-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop support for Symfony 3.4 and 5.3, add support for Symfony 6, drop MongoDB ODM 1.x #284
Conversation
if (!$this->repository instanceof RefreshTokenRepositoryInterface) { | ||
$repository = $om->getRepository($class); | ||
|
||
if (!$repository instanceof RefreshTokenRepositoryInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this tweak after doing a PHPStan analysis, this avoids setting the class property if the type is incorrect (if the property would typed, this would result in a type error instead of the exception below)
|
||
class RefreshAuthenticationFailureEvent extends Event | ||
{ | ||
private AuthenticationException $exception; | ||
|
||
private ?Response $response; | ||
private Response $response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SF6 typehints the response as non-nullable in authentication success and failure handlers, so the events shouldn't allow nulllability either
@@ -14,7 +14,41 @@ | |||
use Symfony\Component\HttpFoundation\JsonResponse; | |||
use Symfony\Component\HttpFoundation\Response; | |||
|
|||
class RefreshAuthenticationFailureResponse extends JsonResponse | |||
if (80000 <= \PHP_VERSION_ID && (new \ReflectionMethod(JsonResponse::class, 'setData'))->hasReturnType()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B/C layer because in SF6 there are PHP 8 types used on the class, so the eval hack is needed for PHP 7.4 compat.
@@ -56,7 +56,7 @@ public static function createForUserWithTtl(string $refreshToken, UserInterface | |||
*/ | |||
public function __toString() | |||
{ | |||
return $this->getRefreshToken(); | |||
return $this->getRefreshToken() ?: ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getRefreshToken()
has a nullable return, __toString()
should always be a string.
README.md
Outdated
@@ -25,6 +25,8 @@ If you want to use this bundle with previous Symfony versions, please use 0.2.x | |||
|
|||
**It's important you manually require either Doctrine's ORM or MongoDB ODM as well, these packages are not required automatically as you can choose between them. Failing to do so may trigger errors on installation** | |||
|
|||
If using Symfony 5.2 or older, you will also need to install the `symfony/security-guard` package, it is only required for the legacy authentication API and is not compatible with Symfony 6.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Guard component is deprecated and dropped in Symfony 6. I made it an optional dependency, folks on Symfony 4.4 will need to manually install that component to keep using this package. Folks on Symfony 5.3 and later won't need to have it installed since the newer API doesn't use it.
@@ -129,6 +129,6 @@ public function supportsClass($class) | |||
return true; | |||
} | |||
|
|||
return User::class === $class; | |||
return class_exists(User::class) && User::class === $class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SF6 drops the User class, so technically the provider shouldn't support the class if it's not available.
composer.json
Outdated
}, | ||
"require-dev": { | ||
"akeneo/phpspec-skip-example-extension": "^4.0|^5.0", | ||
"babdev/phpspec-skip-example-extension": "^1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source package for this looks to be not well maintained and doesn't actually support skipping examples with requirements declared at the example level. So, this changes to my fork that adds the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankfully now not needed :)
58db407
to
dc803e0
Compare
@@ -51,7 +52,8 @@ public function load(array $configs, ContainerBuilder $container): void | |||
$refreshTokenClass = RefreshTokenEntity::class; | |||
$objectManager = 'doctrine.orm.entity_manager'; | |||
|
|||
if (!class_exists(DoctrineOrmMappingsPass::class) || 'mongodb' === strtolower($config['manager_type'])) { | |||
// Change the refresh token and object manager to the MongoDB ODM if the configuration explicitly sets it or if the ORM is not installed and the MongoDB ODM is | |||
if ('mongodb' === strtolower($config['manager_type']) || (!class_exists(EntityManager::class) && class_exists(DocumentManager::class))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to use the object managers instead of the compiler passes in the bundles allows to break the dev dependencies on the bundles (which, were only pulled in for these checks).
$this->authenticator->getCredentials($request), | ||
$this->provider | ||
); | ||
$user = $this->authenticator->getUser($credentials, $this->provider); | ||
|
||
$postAuthenticationToken = $this->authenticator->createAuthenticatedToken($user, $this->providerKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be removed or deprecated too, as PostAuthenticationGuardToken
is deprecated.
For ->onAuthenticationSuccess()
any TokenInterface
(maybe just JWTUserToken
) will work, as it only uses getUser()
.
And for RefreshToken
event getting pre authenticated toeken should probably be deprecated or reworked.. I'm not sure why is it needed at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole class is already deprecated, the only reason I touched it in this PR is there’s no need for it to have the event dispatcher compat check.
bb579b3
to
69042a1
Compare
Any update? |
Blocked by phpspec not supporting Symfony 6 yet. |
merged @webignition #293 |
9efab5b
to
c684d3d
Compare
OK, we're good to go here. |
…support for MongoDB ODM 1.x
Symfony 3.4 and MongoDB ODM 1.x are EOL, there isn't much of a point in keeping support for them now (one can use older versions if they still need it).
Symfony 5.3 goes EOL this month (January 2022), let's drop it now and simplify a bit of the new authenticator API code.
Symfony 6 is here, this adds the support needed for it.