Sylius: Test environment broken with php 7.2.20

[JakobTolkemit]

Sylius version affected: >=1.4.2

Description

We encountered this error when we tried to clear the cache:

[Symfony\Component\Debug\Exception\FatalErrorException]
Error: During class fetch: Uncaught ReflectionException: Class HWI\Bundle\OAuthBundle\Connect\AccountConnectorInterface not found
  in /var/www/html/sylius/vendor/sylius/sylius/src/Sylius/Bundle/CoreBundle/OAuth/UserProvider.php:36
Stack trace:
#0 /var/www/html/sylius/vendor/symfony/debug/DebugClassLoader.php(160): require('/var/www/html/s...')
#1 [internal function]: Symfony\Component\Debug\DebugClassLoader->loadClass('Sylius\\Bundle\\C...')
#2 [internal function]: spl_autoload_call('Sylius\\Bundle\\C...')
#3 /var/www/html/sylius/vendor/symfony/config/Resource/ClassExistenceResource.php(78): class_exists('Sylius\\Bundle\\C...')
#4 /var/www/html/sylius/vendor/symfony/dependency-injection/ContainerBuilder.php(353): Symfony\Component\Config\Resource\ClassExistenceResource->isFresh(0)
#5 /var/www/html/sylius/vendor/symfony/dependency-injection/Compiler/AutowirePass.php(336): Symfony\Component\DependencyInjection\ContainerBuilder->getReflectionClass('Sylius\\Bundle\\C...', false)
#6 /var/www/html/sylius/vendor/symfony/dep      

I found out that it's due to Symfony trying to load the Sylius\Bundle\CoreBundle\OAuth\UserProvider, when in the test environment, which implements the HWI\Bundle\OAuthBundle\Connect\AccountConnectorInterface.
This interface comes with the hwi/oauth-bundle which doesn't get installed by Sylius-standard.

The only reason it worked before was a bug in Php that made it possible to define a class that implements a non existent interface.

This bug got fixed in Php 7.2.20.
https://www.php.net/ChangeLog-7.php#7.2.20
https://bugs.php.net/bug.php?id=76980

Steps to reproduce

Install Php 7.2.20 and try to clear the cache in the test environment.

Possible Solution

Two solutions come to mind:

1. Require hwi\oauth-bundle in Sylius-standard
2. Only load the service definition if the hwi\oauth-bundle is installed like here: https://github.com/Sylius/Sylius/blob/defb389775303ec9fffa1dab7bdbd594b3de841e/src/Sylius/Bundle/CoreBundle/DependencyInjection/SyliusCoreExtension.php#L85-L92

[pplotka]

I think this is a bug in Symfony or PHP:
https://github.com/symfony/symfony/issues/32395 and https://bugs.php.net/bug.php?id=76980

(the simple solution is downgrading PHP version to 7.2.19 or waiting for fixed release)

[peterukena]

Colleage of OP here:

PHP 7.2.19 (docker image FROM php:7.2.19-apache-stretch) works in our pipelines with Sylius 1.4.6.

Since we did not get hwi/oauth-bundle installed, because its in require-dev of sylius/sylius I consider this a bug in Sylius with broken dependencies and/or class declarations.

Sady, I did not have the time yet to look into it in more detail yet. :(

PS: On that, dockers php:7.2-apache is also broken, because it switched from stretch to buster on friday night, which broke the php-gd extension with libreetype6-dev.

[mmenozzi]

I think this is a Sylius bug. Sylius makes use of HWI\Bundle\* classes inside implementation classes so it must require the related dependency (hwi/oauth-bundle) inside composer.json. Currently it's required using require-dev and dev dependencies will not be installed in projects using sylius as a dependency. I'll provide a quick PR.

[vvasiloi]

@mmenozzi I think it is a dev-dependency for tests only, because it's an optional dependency and is listed in "suggests".

[mmenozzi]

@vvasiloi yes it is but it’s also a test dependecy in projects using Sylius and in such context you don’t have the hwi bundle installed (require-dev are fetched only for the foot package). And given the new php behavior (which is correct and will remain as is in php 7.4) the dependency is not optional anymore.

It’s also true that symfony is trying to understand what to do for php 7.4 (see https://github.com/symfony/symfony/issues/32395) so maybe it’s better to wait a decision on that issue.

[vvasiloi]

My point is that moving it to "require" is not a solution.

[mmenozzi]

And what’s the solution in your opinion?

[vvasiloi]

To load the tests for HWI oAuth only when the library is installed.

[mmenozzi]

@vvasiloi it's not a test throwing this error. The exception is thrown during the Symfony cache warmup for the test env and this is because the Sylius\Bundle\CoreBundle\OAuth\UserProvider implements an interface that doesn't exists and this service is loaded in the test configuration only. The whole test configuration is always loaded regardless the tests you run. As I said, given the current version of Symfony DI component and PHP 7.2.20 (and also 7.3.7) the hwi/oauth-bundle it's not optional anymore for projects using Sylius.

In my opinion we have 3 choices:

1. Tell people to not use PHP 7.2.20 and 7.3.7 (bugfix for https://bugs.php.net/bug.php?id=76980 will be reverted in 7.2.21 and 7.3.8).
2. Remove the sylius.test.oauth.user_provider service declaration from test configuration and the related tests.
3. Merge https://github.com/Sylius/Sylius/pull/10537 (so the hwi/oauth-bundle will become not optional)

Consider also that the option 1 will not be available with PHP 7.4 but maybe at that point we'll have a solution from Symfony as well.

[vvasiloi]

I did not test it, but as the author of the issue mentioned (and can be seen in the code), that service is only loaded when the bundle is installed: https://github.com/Sylius/Sylius/blob/defb389775303ec9fffa1dab7bdbd594b3de841e/src/Sylius/Bundle/CoreBundle/DependencyInjection/SyliusCoreExtension.php#L85-L92
Will test myself later.

[mmenozzi]

No @vvasiloi that code doesn't matter because in test environment the sylius.test.oauth.user_provider il loaded plain without any conditions here:

https://github.com/Sylius/Sylius/blob/df295e46777266d4d3b94d1726b7cfc01f5c36fb/src/Sylius/Bundle/CoreBundle/Resources/config/test_services.xml#L58-L68

[mmenozzi]

After chatting with @vvasiloi on Slack we basically found that the issue was introduced with this commit: https://github.com/Sylius/Sylius/commit/047df6c7dd7656ad5e333fb75cf8726ce48da731

Here there's the definition of the sylius.test.oauth.user_provider service without the check for hwi/oauth-bundle installation.
This was not a problem before PHP 7.2.20 or 7.3.7 but now it is (and it will be with 7.4).

So I'm going to close my PR https://github.com/Sylius/Sylius/pull/10537.

I think that the right thing to do is to remove sylius.test.oauth.user_provider service definition and change the line
https://github.com/Sylius/Sylius/blob/df295e46777266d4d3b94d1726b7cfc01f5c36fb/tests/Functional/UpdatingUserPasswordEncoderTest.php#L110

to use the sylius.oauth.user_provider service which is already loaded only when the hwi/oauth-bundle is installed and has the same definition.

[peterukena]

@vvasiloi it's not a test throwing this error. The exception is thrown during the Symfony cache warmup for the test env and this is because the Sylius\Bundle\CoreBundle\OAuth\UserProvider implements an interface that doesn't exists and this service is loaded in the test configuration only. The whole test configuration is always loaded regardless the tests you run. As I said, given the current version of Symfony DI component and PHP 7.2.20 (and also 7.3.7) the hwi/oauth-bundle it's not optional anymore for projects using Sylius.

In my opinion we have 3 choices:

1. Tell people to not use PHP 7.2.20 and 7.3.7 (bugfix for https://bugs.php.net/bug.php?id=76980 will be reverted in 7.2.21 and 7.3.8).

I think this is a bad idea, since it only suppresses the symptoms and is not a fix.

2. Remove the `sylius.test.oauth.user_provider` service declaration from test configuration and the related tests.

Either this or #3 - I feel this is the option to go, because after a rough look at that comment I think it was bad test fixing that introduced this error.

3. Merge #10537 (so the `hwi/oauth-bundle` will become not optional)

This is the wooden hammer get-all method. Should work and also properly fix the bug, but having dependencies required that are not _actually_ required is bad practice I guess.

Consider also that the option 1 will not be available with PHP 7.4 but maybe at that point we'll have a solution from Symfony as well.

[mmenozzi]

Yes @kortwotze read my last comment (https://github.com/Sylius/Sylius/issues/10530#issuecomment-513469164) and review the @vvasiloi PR (https://github.com/Sylius/Sylius/pull/10540).

[leonboot]

I found this thread after failing unit tests because of the missing interface. Thanks to @JakobTolkemit for figuring out that the issue is specific to PHP 7.2.20 and PHP 7.3.7! I spent hours figuring out why I didn't experience any problems on my machine and the CI build failed. Turned out my PHP Docker image was still at 7.3.6.

Nonetheless, I think that PR #10537 is the proper solution to the issue. As long as there are classes that implement interfaces from a dependency, that dependency should be required. Or the Oauth\UserProvider class should be removed from the core bundle into its own bundle which has a dependency on hwi/oauth-bundle.

[pamil]

Fixed by #10540.

[tom10271]

I am using Sylius 1.5.2 with PHP 7.3 and still facing this exception. Manually applying the patch fixes the problem.

I have also checked Sylius 1.5.3 has no such fix too.

[mmenozzi]

Hi @tom10271,
https://github.com/Sylius/Sylius/pull/10540 has not been released yet. You have 2 options:

• Upgrade to PHP 7.3.8 (you probably have 7.3.7 if you have this problem)
• Use https://github.com/cweagans/composer-patches and apply the diff patch of https://github.com/Sylius/Sylius/pull/10540 to your project:

        "patches": {
            "sylius/sylius": {
                "Test environment broken with php 7.2.20 (#10530)": "https://patch-diff.githubusercontent.com/raw/Sylius/Sylius/pull/10540.diff"
            }
        }

[tom10271]

Thank you very much for providing solutions