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 OpenIddict to 5.6.0 #16054

Merged
merged 1 commit into from
May 14, 2024
Merged

Conversation

kevinchalet
Copy link
Member

@kevinchalet kevinchalet added OpenId dependencies Pull requests that update a dependency file labels May 14, 2024
@kevinchalet kevinchalet added this to the 2.0 milestone May 14, 2024
@kevinchalet kevinchalet self-assigned this May 14, 2024
@Piedone
Copy link
Member

Piedone commented May 14, 2024

Does this fix something important/regression since 1.8, or why the 2.0 milestone?

@kevinchalet
Copy link
Member Author

Does this fix something important/regression since 1.8

It indeed includes a few fixes (nothing "critical", tho').

or why the 2.0 milestone?

I simply opted for the earliest release: since only the latest OpenIddict minor version is supported, it's always a good idea to reference it to ensure end users are not accidentally using an old version.

That said, if you prefer postponing that to the next release, no problem.

@MikeAlhayek MikeAlhayek modified the milestones: 2.0, 2.x May 14, 2024
@MikeAlhayek
Copy link
Member

@kevinchalet I don't think we should tag with 2.0 unless we want to hold the release of 2.0 due to this PR.

However, I see no reason not to merge this. I changed the tag to 2.x but this should be merged unless @Piedone sees an issue that I am missing.

@Piedone
Copy link
Member

Piedone commented May 14, 2024

OK, thanks. Just that it's unusual for a PR to have a milestone, but when it's for the upcoming concrete release then that means we really want it to have in it. So, I was confused.

I'm OK with it if Kévin you also tested it and it doesn't break anything obvious and if this point doesn't affect us.

@MikeAlhayek MikeAlhayek merged commit 31528d2 into OrchardCMS:main May 14, 2024
7 checks passed
@kevinchalet
Copy link
Member Author

kevinchalet commented May 14, 2024

OK, thanks. Just that it's unusual for a PR to have a milestone, but when it's for the upcoming concrete release then that means we really want it to have in it. So, I was confused.

Haha, I do that with all my PRs (not just here, but basically everywhere), as it's a very easy way for users to know when the change(s) introduced by a PR ship(s) (otherwise they have to look at the merged commit to see if it's included in a specific tag, which isn't something most users are familiar with).

(in the same vein, I always try to add the relevant tags to my PRs, so they are easier to find by users who're interested in a specific area).

However, I see no reason not to merge this. I changed the tag to 2.x but this should be merged unless @Piedone sees an issue that I am missing.

As a user, I personally find "2.x" milestones very confusing, specially when applied to PRs: okay, the change landed in a 2.x release... but which one? 🤣

I'm OK with it if Kévin you also tested it and it doesn't break anything obvious and if this point doesn't affect us.

I tested the good old authorization code flow with PKCE with Postman it worked fine 👍🏻
Regarding the behavior change affecting GetClaims(), there's no risk, OC doesn't use it at all 😃

https://github.com/search?q=repo%3AOrchardCMS%2FOrchardCore%20GetClaims&type=code

@kevinchalet kevinchalet deleted the openiddict_5.6.0 branch May 14, 2024 18:16
@Piedone
Copy link
Member

Piedone commented May 14, 2024

OK, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file OpenId
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants