[2.0] Support for Symfony 5.0

[alcaeus]

This serves as an epic to collect all issues and pull requests arising with Symfony 5 support.

[OneEyedSpaceFish]

@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.

[alcaeus]

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.

[cpjolly]

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

[OneEyedSpaceFish]

@alcaeus
Do you need a hand with the migration?
If you explain what you planned to do, I wouldn't mind taking a look.

[alcaeus]

In a nutshell:

• Update composer constraints to allow Symfony 5
• Ensure Symfony 5 is tested on travis-ci
• Update code to avoid breakages and deprecations due to Symfony
• repeat above until build is stable

If you're interested, please go ahead 👍

[OneEyedSpaceFish]

@alcaeus Are there any pending PRs that you would like to include in this change request?

[alcaeus]

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.

[cpjolly]

@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.

[OneEyedSpaceFish]

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.

[sci3ma]

Any updates when we can use FOSOAuthServerBundle with Symfony 5?

[OneEyedSpaceFish]

@sci3ma none from me yet, I've been too busy with work.
Feel free to have a look

[elchris]

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.

• composer install
• ./vendor/bin/phpunit

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.

[elchris]

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

[elchris]

since we use a deprecated PHPUnit method assertArraySubset, I looked at this sebastianbergmann/phpunit#3494 (comment)

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:

sebastianbergmann/phpunit#3338

[elchris]

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.

[elchris]

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');

[elchris]

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:

1. I could use some guidance from the maintainers of the oauth2-php project as to whether I should send-in a pull-request of my fork, and into which branch
2. I would appreciate if some folks could check out my fork of /this/ project here: https://github.com/elchris/FOSOAuthServerBundle/tree/symfony-5 and guide me as to whether or not I should send-in a pull request, and if so, into which branch. And to also let me know whether there are other tests which I could run beside ./vendor/bin/phpunit

[OneEyedSpaceFish]

@alcaeus fyi

[anboo]

FYI

[patrickbussmann]

@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 🥳👍

[elchris]

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

[patrickbussmann]

Yes please make for both repositories because we need for installation 👍

[elchris]

Yes please make for both repositories because we need for installation 👍

ok sounds good, I'll try to do it later tonight.

[iisisrael]

@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]

@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.

[iisisrael]

@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]

@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.

[elchris]

@iisisrael I merged your PR into my branch, thank you.

[elchris]

• I made a PR for oauth2-php
• I made a PR for this project

Still working on some cleanups on both.

[elchris]

@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 :)

[elchris]

@iisisrael I added "composer commands" in the "scripts" section of composer.json so now we can:

• composer test
  -- ./vendor/bin/phpunit
• composer lint
  -- ./vendor/bin/php-cs-fixer fix .
• composer phpstan
  -- ./vendor/bin/phpstan analyse --configuration phpstan.neon --level 6 .

[elchris]

@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.

[iisisrael]

Made some functional test improvements, will continue over the weekend, and clean up the lint and phpstan results.

[elchris]

@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:

1. switching my getter/setter additions to production code to reflection class references in tests
2. look at the warnings or ignore them for now
3. look at the incomplete tests or just ignore them for now
4. look at phpstan improvements. But if you're busy with the above two I can work on phpstan stuff. What I can do is start from the bottom of the list of the phpstan warnings, and if you do get around to phpstan, you could start from the top of the list. Or I'm happy to hold off altogether and let you run with it. Let me know :)

[elchris]

@iisisrael I'm removing the reference to DefinitionDecorator in OAuthFactory test because it was deprecated as of Symfony 3.3

[elchris]

@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:

• some assertion will always evaluate to false
• result of void method being used, like an assertNull on a "void" process() method
• iterable types needing to be more specified w/ generics-like type hints
• unreachable code

... 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

[iisisrael]

@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.

[elchris]

@iisisrael Thank you for your work, I just merged your pull request into my branch.

[patrickbussmann]

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 😢

[cpjolly]

@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

[elchris]

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.

[elchris]

What would be helpful:

1. 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.
2. If bugs are found, for people to submit pull requests to my fork, and I will review them and merge them
3. To send-in pull requests for "code cleanups" of issues reported by PHPStan, by running composer phpstan, such that we might put their CI script in a position to finally pass. There are a lot of issues which if addressed, would definitely make this framework a lot more robust. I have fixed a bunch of issues, but there are still so many more left, it's very tedious. If you are looking to take this on, let us know in this thread such that we don't duplicate efforts. Personally I'm done for a while, but if I do fix a few more, I'll update this thread.

[sabat24]

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.

[NielsJanssen]

@elchris

What would be helpful:

1. 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.

[tmarly]

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 !

[deguif]

Symfony 5 is now supported in master branch.
An alpha 2 version will be tagged in the coming days.

[elchris]

@deguif - did i still need to rebase my fork, or are you good now that you have fixed it into master?

[deguif]

@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 ;)

[lebadapetru]

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...

[cpjolly]

Hi @elchris, hi @deguif,
I did a comparison of head with the changes in the folk from @elchris. Most seem to have already been implemented in master, so a rebase & smaller PRs of the remaining changes looks like a good idea