Skip to content
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

Fix SF 7 compat #366

Merged
merged 1 commit into from
Dec 26, 2023
Merged

Fix SF 7 compat #366

merged 1 commit into from
Dec 26, 2023

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Nov 13, 2023

The Symfony 7 compat PR was merged and released too quickly, given that:

  • lexik/jwt-authentication-bundle itself has a pending PR for Symfony 7 support Merged
  • The original PR added symfony/security-guard versions that do not exist to the Composer manifest
  • The original PR didn't add CI coverage for Symfony 7 (or 6.4), so the passing CI is a false positive since that combination is untested

This PR is a draft PR until the lexik/jwt-authentication-bundle PR is merged, as that will block the Symfony 7 build from running until it's merged/released. Issues fixed and ready for review/merge.

@@ -298,6 +298,10 @@ public function testAttachesTheTokenToTheResponseBodyOnCredentialsAuth()

public function testDoesNothingWhenThereIsNotAUser()
{
if ((new \ReflectionClass(AuthenticationSuccessEvent::class))->getMethod('getUser')->hasReturnType()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is necessary because the WIP lexik/jwt-authentication-bundle 3.x branch adds return types on this event class, and its getUser() method isn't documented as nullable in the first place. For now I just skip the test when that branch gets installed as it's the only failure when I force all dev dependencies to be pulled in (merging #338 as early as it was might've been premature given we don't look to be anywhere near a stable release yet).

@chalasr
Copy link
Contributor

chalasr commented Nov 25, 2023

lexik/LexikJWTAuthenticationBundle#1165 has just been merged 👍
I'll tag a new release under one week

@mbabker mbabker force-pushed the fix-sf-7 branch 5 times, most recently from a005f48 to 8222d03 Compare November 25, 2023 22:22
@mbabker mbabker marked this pull request as ready for review November 25, 2023 22:27
@mbabker
Copy link
Contributor Author

mbabker commented Nov 25, 2023

Rebased, issues fixed, ready to go.

@mbabker mbabker changed the title [WIP] Fix SF 7 compat Fix SF 7 compat Dec 1, 2023
@webignition
Copy link
Contributor

👍 Nice one. Will be nice to see this merged!

@markitosgv markitosgv merged commit 9cc564b into markitosgv:master Dec 26, 2023
22 checks passed
@mbabker mbabker deleted the fix-sf-7 branch December 26, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants