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

[13.x] Support OAuth2 Server v9 #1734

Merged
merged 22 commits into from
May 17, 2024
Merged

[13.x] Support OAuth2 Server v9 #1734

merged 22 commits into from
May 17, 2024

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Mar 29, 2024

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@hafezdivandari
Copy link
Contributor Author

@driesvints this repo doesn't have master branch?

@jayan-blutui
Copy link

Great work on creating this PR so quickly seeing v9rc just launched a couple of days ago.

I know this PR is still a WIP but do you plan to include the new Device Authorization?

@hafezdivandari
Copy link
Contributor Author

@jayan-blutui thanks.

I know this PR is still a WIP but do you plan to include the new Device Authorization?

Not in this PR, as it makes this hard to review.

@driesvints
Copy link
Member

Thanks a lot for this PR @hafezdivandari. We should indeed try to support the new Device flow in v13 but let's indeed tackle that in a subsequent PR.

.gitignore Outdated Show resolved Hide resolved
src/Bridge/FormatsScopesForStorage.php Outdated Show resolved Hide resolved
@@ -16,13 +16,9 @@ jobs:
strategy:
fail-fast: true
matrix:
php: ['8.0', 8.1, 8.2, 8.3]
php: [8.1, 8.2, 8.3]
laravel: [9, 10, 11]
Copy link
Member

Choose a reason for hiding this comment

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

In a next PR we may drop Laravel v9 and v10 support as well as PHP 8.1

composer.json Outdated Show resolved Hide resolved
@hafezdivandari
Copy link
Contributor Author

@driesvints This PR is ready, but it should target master, which doesn't exist on this repo.

@driesvints
Copy link
Member

@hafezdivandari that's okay. We're using a different approach these days. I'll talk to the team to prepare a new major version. Ideally we'd also take in the new device flow (separate PR).

Thanks for working on this one!

@hafezdivandari hafezdivandari marked this pull request as ready for review May 15, 2024 14:54
@driesvints driesvints changed the base branch from 12.x to 13.x May 16, 2024 07:51
@driesvints
Copy link
Member

@hafezdivandari I retargeted your PR to 13.x now

@siarheipashkevich
Copy link
Contributor

@hafezdivandari @driesvints why not to 12.x?

@driesvints
Copy link
Member

@siarheipashkevich there's just too many breaking changes in here.

@hafezdivandari reminds me: could you make a list of upgrade steps in UPGRADE.md? I think we only need to reference the OAuth Server client v9 upgrade and a link to its own release notes, telling people to review those as well: https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#900---released-2024-05-13

@taylorotwell
Copy link
Member

Can someone give me an idea of the full upgrade steps and mark as ready for review?

@taylorotwell taylorotwell marked this pull request as draft May 16, 2024 21:55
@driesvints
Copy link
Member

@taylorotwell those are already added to the upgrade guide in this PR. The main reason for this new major version is the oauth server v9 bump and the method signature changes.

@driesvints driesvints marked this pull request as ready for review May 16, 2024 22:44
@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented May 16, 2024

@driesvints We are not going to release 13.x immediately after merging this right? I have 1 or 2 PRs for 13.x before release. I don't want to add too many changes at once, that makes this one hard to review.

@driesvints
Copy link
Member

@hafezdivandari no I'll wait for those

@taylorotwell
Copy link
Member

taylorotwell commented May 17, 2024

@driesvints I guess I'm looking for a general summary of the changes someone will need to make. What changes will a typical Laravel Passport app need to make? Earlier it was mentioned there are too many breaking changes for patch - what are those breaking changes specifically?

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented May 17, 2024

@taylorotwell nothing, just PHP 8.1+ and removed message property from OAuthException HTTP response. Now just use error_description as per the OAuth 2 spec.

The braking change is that most of the methods signatures are changed.

@taylorotwell taylorotwell merged commit fc7e2da into laravel:13.x May 17, 2024
9 checks passed
@hafezdivandari hafezdivandari deleted the master-oauth2-server-v9 branch May 17, 2024 22:05
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