Closed alcaeus closed 3 years ago
@alcaeus Would that be possible to merge all outstanding PRs for 2.0 into a feature branch? That way it would be possible for people to start testing their own application with symfony 5.0 beta before it's release.
I'll do a session to take care of Symfony 5 support next week. Sorry for the delay here, but I've been quite busy with @doctrine packages.
I don't see it mentioned elsewhere so please note that the base server library FriendsOfSymfony/oauth2-php will also need a few minor incremental changes to support Symfony 5. For example, it's composer.json requires "symfony/http-foundation": "~3.0|~4.0"
Also, in it's master branch there is a new Interface for OAuth2 but there is no dot tagged release with this important improvement
@alcaeus Do you need a hand with the migration? If you explain what you planned to do, I wouldn't mind taking a look.
In a nutshell:
If you're interested, please go ahead 👍
@alcaeus Are there any pending PRs that you would like to include in this change request?
Taking a quick look, you want to check out #599, #610, #617, #619, and #623 - they sound like they could affect this. I'd have to take a closer look to know for sure.
@alcaeus I can confirm that the issues described and the proposed fixes in #599, #610, #617, #619, and #623 are the same for Symfony 5.0. Also note #501.
Just an FYI; I started to have a look at this, but there's no much progress made so far, as I'm extremely busy nowadays and have very little time.
So if someone would like to have a look before I can sort this out, feel free to do so.
Any updates when we can use FOSOAuthServerBundle with Symfony 5?
@sci3ma none from me yet, I've been too busy with work. Feel free to have a look
I've started some work on my fork here: https://github.com/elchris/FOSOAuthServerBundle/tree/symfony-5
it also references another fork of mine for the oauth2-php project
i just got the tests to "compile", resolved composer dependencies, etc.
Errors: 21, Failures: 6, Warnings: 18, Incomplete: 2.
If anyone wants to submit to me a pull request against my "symfony-5" branch with some incremental fixes to the tests, i'd be happy to merge them. @OneEyedSpaceFish fyi :)
This article will be relevant for OAuthListenerTest which references Symfony\Component\Security\Core\SecurityContextInterface which was deprecated.
Also this is relevant: https://stackoverflow.com/questions/56438177/deprecated-the-listenerinterface-turn-your-listeners-into-callables-instead in OAuthListener.php as Symfony\Component\Security\Http\Firewall\ListenerInterface was deprecated.
I let Nicolas Grekas know about this article which still references it. Issue I filed here.
Also relevant: https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching
since we use a deprecated PHPUnit method assertArraySubset, I looked at this https://github.com/sebastianbergmann/phpunit/issues/3494#issuecomment-464464423
and added a reference to https://packagist.org/packages/dms/phpunit-arraysubset-asserts
There are a lot more PHPUnit deprecations which we should look into:
Now down to: Tests: 186, Assertions: 446, Warnings: 5, Incomplete: 3.
Still need to deal with deprecated reference to SecurityContextInterface ... not sure why tests aren't failing against that though.
Before I forget:: two tests had already been marked as incomplete but I added one more as incomplete: testProcessWithoutExistingTokenStorage:
$this->markTestIncomplete('Find a graceful way to handle what happens when no token storage is available');
ok i'm down to 2 warnings which i can't really find a way to fix so I'll leave them be.
1) FOS\OAuthServerBundle\Tests\Document\AuthCodeManagerTest::testDeleteExpired Method getQuery may not return value of type Mock_AbstractQuery_84acf880, its return declaration is ": Doctrine\ODM\MongoDB\Query\Query"
2) FOS\OAuthServerBundle\Tests\Document\TokenManagerTest::testDeleteExpired Class "Doctrine\ODM\MongoDB\Query\Query" is declared "final" and cannot be mocked.
as far as SecurityContextInterface goes, it looks like it's only referenced in a test as a fall-back for when a TokenStorageInterface was not available, and I imagine it was a backward-compatibility thing, and from what I can tell that part of the test doesn't get executed because the TokenStorage is there.
Ok so with this last commit to my branch, I could in theory perpetrate a pull request ...
The tests all pass as far as ./vendor/bin/phpunit goes. Are there some other tests I should run?
Also I would need to send-in a pull-request for the other project "oauth2-php" https://github.com/elchris/oauth2-php/tree/symfony-5 which would also be backward-incompatible and would need a separate version branch in the original project.
So ...... to recap:
@alcaeus fyi
FYI
@elchris can you make a pull request? 👍 Looks great.
I'm using it via:
$ composer.json
"repositories": [
{
"type": "git",
"url": "https://github.com/elchris/FOSOAuthServerBundle.git"
},
{
"type": "git",
"url": "https://github.com/elchris/oauth2-php.git"
}
]
And then:
$ composer req friendsofsymfony/oauth2-php:dev-symfony-5
$ composer req friendsofsymfony/oauth-server-bundle:dev-symfony-5
Thanks very much for your work 🥳👍
ok I'll make a pull request shortly, and ya'll can let me know later what you want me to do for oauth2-php
Yes please make for both repositories because we need for installation 👍
Yes please make for both repositories because we need for installation 👍
ok sounds good, I'll try to do it later tonight.
@elchris I'm working through some clean-up forked from your fork, and I'm wondering why the Symfony templating component was removed from the AuthorizeController
service configuration? The component itself is still supported in v5, only deprecated from framework integration in 4.3.
This could be changed to depend on Twig\Environment
instead, but that's a BC break not required for Symfony 5 support, right? Or am I missing something?
@elchris I'm working through some clean-up forked from your fork, and I'm wondering why the Symfony templating component was removed from the
AuthorizeController
service configuration? The component itself is still supported in v5, only deprecated from framework integration in 4.3.This could be changed to depend on
Twig\Environment
instead, but that's a BC break not required for Symfony 5 support, right? Or am I missing something?
I can look at it, and of course, I can't speak for the maintainers of this package but I imagine that the goal would be to arrive at something that is compatible and deprecation-warning-free in a 4.4/5.0.* context, such that a new release might be cut from it, which is backward-incompatible with anything earlier than 4.4?
But I'm definitely open to suggestions because I'm more or less "flying blind" as I'm making a lot of assumptions.
Also if at some point you wish to do so, I'd be happy to merge a pull request into my branch from you.
@elchris I poked around a bit for a solution, but it seems that trying to hand-roll a templating service would be a bit beyond the scope of S5 compatibility. That could be a future improvement if someone wanted to do that or otherwise add a third-party templating service. Twig it is.
I also clarified the authentication listener handling, which shouldn't ever have been expected to return anything.
Some other minor clean-up, dev environment improvements.
There's a little more polish that can be done, noted on the PR on your fork. I can look at that tomorrow.
@elchris I poked around a bit for a solution, but it seems that trying to hand-roll a templating service would be a bit beyond the scope of S5 compatibility. That could be a future improvement if someone wanted to do that or otherwise add a third-party templating service. Twig it is.
I also clarified the authentication listener handling, which shouldn't ever have been expected to return anything.
Some other minor clean-up, dev environment improvements.
There's a little more polish that can be done, noted on the PR on your fork. I can look at that tomorrow.
Oh that's very cool. I'll try to look at it and merge it in tonight, thank you so much for doing this.
@iisisrael I merged your PR into my branch, thank you.
Still working on some cleanups on both.
@iisisrael I just realized they had phpcs and phpstan hooked-up in their CI so I made some updates to travisci config file, applied php-cs-fixer to fix all phpcs issues, but now i'm working thru a flurry of phpstan fails and it's going to take me a while but i'll be committing incrementals.
=> TL;DR: be sure to pull again from my origin into your branch :)
@iisisrael I added "composer commands" in the "scripts" section of composer.json so now we can:
@iisisrael I'll try to tackle as many of the phpstan issues as possible over the weekend. It would appear that all those issues have been around for quite some time, but I'd like to get their CI to pass again, which does rely upon phpstan level 6.
Made some functional test improvements, will continue over the weekend, and clean up the lint and phpstan results.
@iisisrael that's great I merged your pull request.
lint requests should already be clean. (should) (lol) But basically when you run "composer lint" it should automagically fix all issues for you, so then you only need to commit the changes that it made. Because it's using php-cs-fixer
Priority-wise you might consider:
@iisisrael I'm removing the reference to DefinitionDecorator in OAuthFactory test because it was deprecated as of Symfony 3.3
@iisisrael Updates:
Please be sure to pull from my branch
I've cleaned-up a lot of obvious "return type missing" phpstan issues starting from the bottom up, mostly in the tests, and there are still a bunch of those starting from the top in the production code, but overall, in the tests we're still left with a ton of issues like:
... still overall 200 phpstan errors and 2 warnings. lol.
@patrickbussmann if you can, please try to re-run some tests in your project. The unit tests in here still all pass
@elchris, reflection property assertions implemented, removing the getters.
Up next, will fix MongoDB test configurations, where tests are failing with warnings on mocked query builder return value, which can't be mocked (ODM query is a final class) and can't return an ORM query object (return type constraint). I'm pretty sure mongo can be configured in the Symfony test environment to run with the in-memory storage engine. I've only ever tested functionally with an actual collection written to disk, but if in-memory is possible, it will have the same advantages as using sqlite for ORM tests.
@iisisrael Thank you for your work, I just merged your pull request into my branch.
Thanks @elchris but I found a issue with anonymous users. I fixed it here and added a test case but I'm not able to create pull requests. I already informed GitHub about this issue.
https://github.com/patrickbussmann/FOSOAuthServerBundle/tree/symfony-5-anonymous-access And now I cant upload pictures because of error message. I will create a pull request as soon as GitHub fixes the issue.
Else you could merge it directly when you checkout the branch.
Thanks in Advance and in general for your work 👍
Ah lol ok, "You cant comment at this time". Seems to be a bigger GitHub problem 😢
@elchris - I see all the excellent work done by yourself and the other guys, but what's the status of your fork ? Any idea when it will be "ready" and your changes will show up here ?
For now I'm using your fork as described above by @patrickbussmann
Hi @cpjolly :) As mentioned here I did make a pull request from my fork, but I have not yet heard from the maintainers of this project. So I have no idea if and when this will ever get mainstreamed.
And yes for now I would say that it is a good idea to keep using my fork as you currently are, I will continue to review and accept any and all pull requests to my fork.
Big thanks to @iisisrael & @patrickbussmann for their contributions.
What would be helpful:
I found one issue in the architecture of OAauth2.php lib. I think that GrantAccessToken method shouldn't return the whole response object (HTTP response). It should return a newly created token and there should be another method which could build the response.
If I want to override a tokenAction inside my own controller and make some other stuff with the token I need to parse a JSON response from GrantAccessToken method. It's not a big problem but I generally expect from model to get a raw data not HTTP response. That response should be made inside controller with maybe additional help of model.
@elchris
What would be helpful:
- For more people to test my fork in their projects, and report back in this thread to tell us what works and what doesn't.
I've installed your fork and have been using it in a very basic OAuth implementation, so far without issues.
Just for information, I am using this fork since one month (not in production yet), using the authorization code flow, and I found no issue. Great job @elchris !
Symfony 5 is now supported in master branch. An alpha 2 version will be tagged in the coming days.
@deguif - did i still need to rebase my fork, or are you good now that you have fixed it into master?
@elchris would be nice if you can rebase as that would allow to see other changes you introduced. But the better would be that you open small PRs with specific changes ;)
Hey guys, using symfony 5 and i tried to install this bundle but i get the following error:
The child node "db_driver" at path "fos_oauth_server" must be configured.
I've added this into my composer.json:
{
"friendsofsymfony/oauth-server-bundle": "dev-master"
}
EDIT: I managed to fix it by creating a config/packages/fos_oauth_server.yaml with:
fos_oauth_server:
db_driver: orm # Drivers available: orm, mongodb, or propel
client_class: Entity\Client
access_token_class: Entity\AccessToken
refresh_token_class: Entity\RefreshToken
auth_code_class: Entity\AuthCode
But i've came across a different error:
No authentication listener registered for firewall "oauth_authorize".
In my security.yaml:
oauth_token:
pattern: ^/oauth/v2/token
security: false
oauth_authorize:
pattern: ^/oauth/v2/auth
# Add your favorite authentication process here
api:
pattern: ^/api
fos_oauth: true
stateless: true
anonymous: false # can be omitted as its default value
Gotta say, this bundle ain't easy to setup for a beginner...
This serves as an epic to collect all issues and pull requests arising with Symfony 5 support.