[make:auth] handle session deprecations in 5.3 #801
Conversation
- fixed deprecated ReflectionType::__toString() (MakeAuthenticatorTest) Deprecated in PHP 7.1
- removes unneeded optional param - triggers required param order deprecation in PHP8 (GeneratorTest)
- handles Session deprecation's introduced in Symfony 5.3 in test suites
|
I suspect that the failing tests are caused by - symfony/symfony#38616 - the timing is pretty much perfect. What we'll need to do is run that test locally (using the flags so we get the If we are able to reproduce all of this, then we could try to create a tiny, reproducer project that shows the (potential) bug and open an issue in symfony. I don't want to ping the author Thanks! |
a5303fb to
78674a9
Compare
|
Most of the session related test issues on the // login a user in WebTestCase
$usageTrackingTokenStorage = static::$container->get('security.token_storage');
$usageTrackingTokenStorage->disableUsageTracking(); // <- bypasses trying to get the session from the request stack
$token = $usageTrackingTokenStorage->getToken();
$this->assertNotNull($token); // passesor by adding a event listener on the request with a callable to the test as done in https://github.com/symfony/symfony/pull/38616/files#diff-c1a4581fd182f7bd35bbb4f0297fd62d1df49fb0b2a4b4210707f3a8131766d1R53-R55 |
|
With see #825 |
|
splitting up the test fixes into individual PR's - there are too many issues across different parts of maker to fix everything under one roof. |
|
|
||
| // Handle Session deprecations in Symfony 5.3+ | ||
| if (Kernel::VERSION_ID >= 50300) { | ||
| $tokenStorage = static::$container->get('security.token_storage'); |
There was a problem hiding this comment.
Why is the way that we fetch the service different in the two cases? This new way seems more correct.
And, would it be possible to use a method_exists() instead of the Symfony version?
$tokenStorage = static::$container->get('security.token_storage');
if (method_exists($tokenStorage, 'disableUsageTracking')) {
$tokenStorage->disableUsageTracking();
}
$token = $tokenStorage->getToken();Checking the kernel is sort of a "last resort". If we can rely on something hard in the code, that's even better.
73c30e4 to
4f96eb5
Compare
| $typeHint .= ' '; | ||
| $type = $reflectionMethod->getParameters()[2]->getType(); | ||
|
|
||
| if (!$type instanceof \ReflectionNamedType) { |
There was a problem hiding this comment.
getType() returns \ReflectionNamedType | null since 7.1. As maker is not used in production, I opted to check instanceOf vs the more performant null === for better DX.
| { | ||
| $tokenStorage = static::$container->get('security.token_storage'); | ||
|
|
||
| if (class_exists(SessionNotFoundException::class)) { |
There was a problem hiding this comment.
And, would it be possible to use a method_exists() instead of the Symfony version?
Yes, kinda. The disableUsageTracking() method has always existed in the UsageTrackingTokenStorage::class since it was introduced in Symfony 4.4
For context,
8000
PR 38616 changed how the token is retrieved when calling UsageTrackingTokenStorage->getToken() if usage tracking is enabled (which the kernel enables).
Although I'm not sure we need to, but to support Symfony 4.0 - 4.3 we can't rely on disableUsageTracking() to exist. And for Symfony 4.4 - 5.2 I don't think we should disable usage tracking as thats not realistic from the users perspective although we could. E.g. class_exists(UsageTrackingTokenStorage)
The SessionNotFoundException::class was introduced in 38616 (Symfony 5.3) and was the only class introduced that didn't look like it would be removed anytime soon.
There was a problem hiding this comment.
let's keep the kernel check then - thanks for the explantion
d39be33 to
cf0a27d
Compare
|
Thanks Jesse! |