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

Fix fatal error when OPCache preloading is used #13709

Merged

Conversation

mmenozzi
Copy link
Contributor

Q A
Branch? 1.11
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets none
License MIT

While in Symfony 5 the Symfony\Bundle\FrameworkBundle\Templating\EngineInterface was removed, in Sylius there were still references to that interface in docblock comments.

With Sylius 1.11 (with this commit) the type hint has been promoted to a PHP type hint. This normally does not cause any problem because apparently the class loading is lazy and does not try to load the non-existent interface.

But if PHP OPCache preload is used, like suggested here, it appears that PHP switches to eager class loading and you get the following fatal error:

NOTICE: PHP message: PHP Fatal error:  Failed to load class Symfony\Bundle\FrameworkBundle\Templating\EngineInterface used by typed property Sylius\Bundle\ShopBundle\Controller\SecurityWidgetController::$templatingEngine during preloading in Unknown on line 0

With this PR we propose to remove all references to the old interface as Symfony 4 is not supported anymore by Sylius.

@mmenozzi mmenozzi requested a review from a team as a code owner February 28, 2022 11:30
@probot-autolabeler probot-autolabeler bot added Admin AdminBundle related issues and PRs. Maintenance CI configurations, READMEs, releases, etc. Shop ShopBundle related issues and PRs. labels Feb 28, 2022
@mmenozzi
Copy link
Contributor Author

The build is red but I don't think it's related to these changes.

@mmenozzi mmenozzi force-pushed the old-templating-engine-interface-removal branch from 6c52a8e to 1b555f9 Compare February 28, 2022 17:28
@vvasiloi
Copy link
Contributor

Hey @mmenozzi! I finally tried to reproduce this issue and it occurs with PHP 8.0, but not with PHP 8.1.
It's an issue faced by Symfony too and they simply bumped the minimum required PHP version to 8.1 in Symfony 6.1.
Perhaps Sylius should do the same in the next minor release.

@Sylius/core-team I would like to see more opinions on this.

@Ferror
Copy link
Contributor

Ferror commented Mar 25, 2022

I was also able to encounter this problem on ex. on Sylius-Standard.

<b>Fatal error</b>:  Failed to load class Symfony\Bundle\FrameworkBundle\Templating\EngineInterface used by typed property Sylius\Bundle\ShopBundle\Controller\SecurityWidgetController::$templatingEngine during preloading in <b>Unknown</b> on line <b>0</b><br />

From my perspective preloading provide value in production. If Symfony 4 is not supported anymore by Sylius and the class EngineInterface was deleted I don't see a reason why we should still keep a reference to it. Especially that we gain is simple typed classes and less ignored phpstan and psalm errors :)

@mmenozzi
Copy link
Contributor Author

Thank you @Ferror this is exactly what I think.

@vvasiloi
Copy link
Contributor

Removing the reference to that interface will mitigate the problem at hand.
However, if we choose to keep support for PHP 8.0, then we will need a test for preloading to avoid another issue.

@mmenozzi
Copy link
Contributor Author

Yes I agree it could be useful to have a test for that but I think it should be done in a separate PR. Note also that there's already a kind of test: it's PHPStan/psalm.

@vvasiloi
Copy link
Contributor

Sure, I was also thinking of a separate PR. Psalm/PHPStorm will probably be enough for Sylius code, but another issue might be coming from it's 3rd party dependencies.

@lchrusciel
Copy link
Member

@mmenozzi are you willing to do the PR with testing for it?

@lchrusciel lchrusciel added the Bug Confirmed bugs or bugfixes. label Mar 29, 2022
@GSadee GSadee merged commit ad93c74 into Sylius:master Mar 29, 2022
@GSadee
Copy link
Member

GSadee commented Mar 29, 2022

Thank you, Manuele! 🥇

@mmenozzi
Copy link
Contributor Author

@mmenozzi are you willing to do the PR with testing for it?

@lchrusciel do you have any idea on how to test it? AFAIK the preload is done only in the prod environment but we use the test_cached in CI...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin AdminBundle related issues and PRs. Bug Confirmed bugs or bugfixes. Maintenance CI configurations, READMEs, releases, etc. Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants