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

Bump lcobucci jwt version #248

Merged

Conversation

kellerjmrtn
Copy link

@kellerjmrtn kellerjmrtn commented May 10, 2024

Description

Alternative implementation of #229. Since 2.3.0 dropped support for Laravel < 10, none of the Laravel related changes in #229 are needed.

Upgrade guide can be found here. The two notables are

  1. Removal of Lcobucci\JWT\Signer\Ecdsa::create() done here. This was causing failures in a few tests. Simply not calling this method and using new $signer() like is done with other Signer types fixes this.

  2. Lcobucci\JWT\Builder was updated to be @immutable done here. This change did not seem to cause test failures but did show up in the upgrade guide. I've updated its usage in the Lcobucci provider to reassign return values

Note: I suppose that the addition of the Builder return type could be a BC break if something is extending the class? I can remove that if desired

Checklist:

  • I've added tests for my changes or tests are not applicable
  • I've changed documentations or changes are not required
  • I've added my changes to CHANGELOG.md

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

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

LGTM!

Also checked the change to match in detail.


Note: I suppose that the addition of the Builder return type could be a BC break if something is extending the class? I can remove that if desired

I think we should bump the major version of this lib after merging this PR, nevertheless (and I'm therefore fine keeping it).

I know how composer works and that it does not require a major bump just because a dependency like this was updated: if someone requires lcobucci/jwt:^4 too, the would simply not receive e.g. 2.4.0

But I think in practice, due to the stark change of behaviour how that library works (immutable), even though the changes here are were rather minimal, I think it's safer and raises the awareness level of the user.

Therefore I suggest we:

  • add this also to the changelog
  • and bump the major version

@kellerjmrtn
Copy link
Author

Added a line to CHANGELOG.md. Would you also like me to denote the breaking changes? Something like

- If extending `PHPOpenSourceSaver\JWTAuth\Providers\JWT\Lcobucci`
  - `Builder` return type has been added to `addClaim`
  - The underlying `Builder` instance is now immutable

Let me know if there's anything else I should do

@mfn
Copy link
Contributor

mfn commented May 14, 2024

I like it 👍🏼

@Messhias Messhias merged commit d88bb28 into PHP-Open-Source-Saver:main May 14, 2024
12 checks passed
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.

4 participants