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

Relaxed version constraint to allow v7.3 of oauth2-server #11

Merged
merged 4 commits into from
Jan 11, 2019
Merged

Relaxed version constraint to allow v7.3 of oauth2-server #11

merged 4 commits into from
Jan 11, 2019

Conversation

gschafra
Copy link
Contributor

v7.3.0 of oauth-server does not seem to introduce breaking changes. Call of finalizedScopes() (see ScopeRepository) was moved, see https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#730---released-2018-11-13

Guenter Schafranek added 2 commits December 19, 2018 14:42
- Updated version constraint for
  - league/oauth2-server (^7.2) to allow v7.2
  - symfony/psr-http-message-bridge (^1.0) to allow v1.1
- v7.3.0 of oauth-server does not seem to introduce breaking changes.
  Call of ```finalizedScopes()``` (see ```ScopeRepository```) was moved,
  see https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#730---released-2018-11-13
@X-Coder264
Copy link
Collaborator

league/oauth2-server uses semantic versioning to manage version numbers so is there any reason why we shouldn't write the constraint as just ^7.2? Otherwise a similar PR will have to be sent after each new minor version gets released in the 7.x series.

@gschafra
Copy link
Contributor Author

gschafra commented Jan 7, 2019

In fact I've considered using ^7.2. But since the last build failed with

1) App\Tests\Acceptance\TokenEndpointTest::testSuccessfulClientCredentialsRequest
Failed asserting that 3599 is identical to 3600.

I'm no longer sure, if backward compatibility is really ensured within minor version change from v7.2 to 7.3 in leage/oauth2-server. But, of course, I also prefer your suggestion. Just and still trying to find out this, which minor change in the leage/oauth2-server causes failing this test.

@spideyfusion
Copy link
Contributor

@gschafra The issue you're describing was fixed today. See #13 for more information. :-)

@codecov-io
Copy link

codecov-io commented Jan 7, 2019

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #11   +/-   ##
=========================================
  Coverage     86.18%   86.18%           
  Complexity      201      201           
=========================================
  Files            37       37           
  Lines           608      608           
=========================================
  Hits            524      524           
  Misses           84       84

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2969907...e5459ea. Read the comment docs.

@spideyfusion
Copy link
Contributor

@gschafra The change looks good, but I have one question for you based on your commits:

How come you decided not to relax the version requirement for the symfony/psr-http-message-bridge package to ^1.0?

@gschafra
Copy link
Contributor Author

@spideyfusion Kind of conservative attitude of mine. Do you think, it's save and better to allow minor updates on symfony/psr-http-message-bridge? I know, since it's a minor, it should be... so... as soon as you give me an o.k. I will do it.

@spideyfusion
Copy link
Contributor

@gschafra Go for it. Symfony is pretty good at respecting the semantic versioning convention.

Relaxed version constraint also for symfony/psr-http-message-bridge
@spideyfusion spideyfusion merged commit 26d9c0b into trikoder:master Jan 11, 2019
@spideyfusion
Copy link
Contributor

@gschafra Thank you for your continued contributions. :-)

@gschafra gschafra deleted the oauth2-server-73 branch January 11, 2019 10:33
@gschafra
Copy link
Contributor Author

@spideyfusion No problem, thanks for your effort to start this bundle! I'm thinking about to implement symfony command to manage client, like

  • trikoder:oauth2:add/update-client --identifier BlaBla --secret 13AB424 --grants...
  • trikoder:oauth2:remove-client --identifier BlaBla --secret 13AB424 --grants...

Anyone already planned doing this?

@spideyfusion
Copy link
Contributor

@gschafra Sorry for the late reply. It seems someone had similar thoughts and opened a PR for it (#17). We can continue our efforts there. :-)

@ajgarlag
Copy link
Contributor

Sorry, I started to work with this bundle some days ago and did not see this conversation. I'm happy to contribute with it. I hope your revision on #17.

@spideyfusion Thanks for sharing your great work.

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