symfony: [Config] ClassExistenceResource::isFresh() throws fatal error on php 7.2.20/7.3.7 if a parent class is missing
Symfony version(s) affected: 3.4.29, 4.2.10, 4.3.2, 4.4-dev, 5.0-dev
Description
After upgrading to php 7.2.20 or 7.3.7, AutowirePass starts to throw ReflectionExceptions on cache warmup on installations with DoctrineBundle but without Twig and the Form and Validator components. This has been reported to the DoctrineBundle repository, but to me it rather looks like a DI issue.
How to reproduce
- doctrine/DoctrineBundle#990
- https://github.com/derrabus/symfony-32395-reproducer
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 20
- Comments: 82 (52 by maintainers)
Commits related to this issue
- Reproducer for symfony/symfony#32395. — committed to derrabus/symfony-32395-reproducer by derrabus 5 years ago
- Revert "Fixed bug #76980" This reverts commit 35353dc49a73a58c17c7896c4c4c3997ef2c007d. This changes causes issues for Symfony, see https://github.com/symfony/symfony/issues/32395. I'm reverting it ... — committed to php/php-src by nikic 5 years ago
- Fix to php-fpm 7.2.19 Bug: https://github.com/symfony/symfony/issues/32395 — committed to opositatest/php-fpm by ping86 5 years ago
- - use official php images for php 7.1, 7.2 and 7.3 to allow downgrade to older versions - downgrade php 7.2 to 7.2.19 and php 7.3 to 7.3.6 to work around reflection bug https://github.com/symfony/symf... — committed to jost-devops/symfony-app by dnl-jst 5 years ago
- Rollback to PHP 7.3.6 because of 7.3.7 having the breaking change that produces the errors More info: * https://github.com/symfony/symfony/issues/32395 * https://github.com/docker-library/php/issues/... — committed to CodelyTV/php-ddd-example by JavierCane 5 years ago
- minor #32864 Skip tests that fatal-error on PHP 7.4 because of missing parent classes (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- Skip tests that fatal-error on P... — committed to symfony/symfony by nicolas-grekas 5 years ago
That depends on what you’re asking for here. I can revert this change from 7.2 and 7.3. Enforcing this is not critical for those branches and if it causes breakage in Symfony I have no problem with reverting it.
For PHP 7.4 though this change is going to stay in some form. I can invest some effort to avoid throwing a fatal error, but if the request here is to restore the old behavior where you say
Foo implements Barand you can end up with a classFoothat does not actually implementBarby throwing an exception during autoloading, then no, I’m not going to restore that behavior.This topic looks like collection of complaints, but I for one am glad this PHP fix helped us discover this. It uncovered wrongly configured DI, with missing exclude folders in our case, unnecessarily making DI crawl through them. Additionally, these classes shouldn’t have even be available in production composer classmap, since they are working only when no
--no-devis used. So I applaud @nikic for keeping this in some form in PHP 7.4All right, before this evolves into a StackOverflow thread on how to install specific php versions on various platforms:
@derrabus I don’t think so. Let me try to give a specific example of the problem I have in mind, though it’s somewhat convoluted:
What happens now is the following:
C. The classCis registered provisionally.B. The classBis registered provisionally.A. The classAis registered provisionally.Ais verified (trivially) and registered fully.Bis verified and registered fully (and may already be used). During the verification it makes use of the fact thatCis a subclass ofB, otherwise the return types would not be covariant.DoesNotExist, which throws an exception.After these steps have happened … what can we do now? We can’t fully register the class
C, because the interface it implements does not exist. We can’t remove the provisionally registered classCeither, because then a new classCthat is not a subclass ofBcould be registered, and thus violate the variance assumption we made above.I don’t really see a solution here that would allow us to gracefully recover while performing inheritance 😦
@elmariachi111 I don’t know how Heroku handles a situation like this, but there has to be a way to deploy to an older php release. Have you contacted their support yet?
If there really isn’t another way, my suggestion would be:
symfony/form,symfony/validatorandpsr/simple-cacheare hot candidates).That’ll bloat your vendors folder, but you should be able to run your application again. Please remember to revert that change once php 7.2.21 and 7.3.8 are out.
https://github.com/php/php-src/releases 7.2.21 & 7.3.8 released
Remove Entity from exclude list in service.yaml fix for me.
This bug takes 5 hours to get me here ;/
For all people using homebrew (on MacOS), you can use this formula. Copying it to your Formula folder will allow you to install
php@7.2.19(instead of only thephp@7.2option which installs 7.2.20).Simply run this:
For Ubuntu and Debian users you can now update your php binaries, oerdnj fix it : https://github.com/oerdnj/deb.sury.org/issues/1208#issuecomment-515014129
@arderyp The issue is already resolved in the upcoming PHP 7.2 and 7.3 releases. The remaining discussion is about what to do for PHP 7.4, where under my current understanding a change on the Symfony side will be necessary.
Reverted from 7.2 and 7.3 via https://github.com/php/php-src/commit/22ed362810c1b3a5ecb54ebd1d50d804c7fc3159, this change is only in 7.4 now.
Filed a feature request in PHP: https://bugs.php.net/bug.php?id=78351
7.3.8RC1 was tagged today http://git.php.net/?p=php-src.git;a=tag;h=27265a358a4f0d505a192607cbd26d60862ed6cf
FYI: It is caused by https://bugs.php.net/bug.php?id=76980 which was reported because of https://github.com/symfony/symfony/issues/28748
I don’t see why, it’s ok to make DI crash when trying to crawl through invalid classes
@teohhanhui The class
Bcannot be removed at the point where we realize that the interface does not exist, because the class could already be in use. While mixing declarations and code goes against modern coding guidelines, from the language perspective writing code like this is allowed:Of course, we can’t drop the class
Bafter someone has already created an object of it, or similar (a more “typical” use is via class_alias.)Having thought about this for a bit, I think that covariance support in PHP 7.4 effectively makes this impossible. Due to cyclic dependencies, we sometimes have to link classes under the provision that other classes will either also successfully link or else be unusable. This means that we can’t just unregister the class stub if a parent/interface is not found, because then it would be possible to register a class with different methods under the same name and thus violate variance assumptions. The only thing we could do is leave behind a dead class stub, such that the class can neither be used, nor a class using the same name declared, which seems like a pretty bad idea.
When a class implements an interface that can not be found, PHP 7.2.20/7.3.7 throws an exception as well now. See https://github.com/symfony/symfony/issues/32396
I’ve managed to build a minimal reproducer.
https://github.com/derrabus/symfony-32395-reproducer
The problem seems to be
ClassExistenceResourcefrom the Config component. When callingisFresh(), a temporary class loader is registered:https://github.com/symfony/symfony/blob/6811aaa8e087665769dd66c69fb3109d002196ef/src/Symfony/Component/Config/Resource/ClassExistenceResource.php#L71-L73
If a class cannot be loaded because its parent is not found, a
ReflectionExceptionis thrown:https://github.com/symfony/symfony/blob/6811aaa8e087665769dd66c69fb3109d002196ef/src/Symfony/Component/Config/Resource/ClassExistenceResource.php#L121
Previously, this exception could be caught and handled. Since php 7.2.20/7.3.7 however, php seems to catch exceptions thrown during class loading and turns them into a fatal error that cannot be caught.
It’s tagged but not released just yet. Releases tend to happen on Thursdays if I’m not mistaken, so likely tomorrow 😃
For reference, the core logic we need here is a way to make class checks resilient to missing parent classes. This means only these functions are concerned: get_class_methods, get_class_vars, get_parent_class, is_a, is_subclass_of, class_exists, class_implements, class_parents, trait_exists, defined, interface_exists, method_exists, property_exists, is_callable. We need to call them and have them return false in the case that currently crashes.
@nikic regarding your example from above https://github.com/symfony/symfony/issues/32395#issuecomment-509593220
Would it be possible to mark a provisionally registered class as “in use” as soon as it is referenced somewhere?
This way we could determine in an exception case if it is OK to remove the provisionally registered class and gracefully recover or if we have to throw a fatal error.
In your example this would mean that once class
Bis verified it would mark classCas “in use”. When loadingDoesNotExistthrows the exception then, we would throw a fatal error because classCis “in use”.This way we could gracefully recover for most cases while resulting in a fatal error for the impossible cases, which sounds like a perfect trade-off to me.
Instead of using PHP RefelctionClass/class_exists which triggering autoload we might could adapt https://github.com/Roave/BetterReflection. It analyse code without triggering autoload.
The new php bahavior seems fine to me. Can’t we fix our code to not rely on such strange behavior?
@teohhanhui reported that the unit tests of API platform 2.4 are failing because of a similar problem. This is what I get when I run them locally with php 7.3.7:
I’ve ran a quick test with my reproducer on php 7.3.8RC1. Looks like this issue is gone. 🎉
(edit: 7.2.21RC1 is fine as well.)
@arderyp : no it will also roll back php version if you installed 7.2.20 exactly when you execute
sudo apt upgradeit will ask you for downgrade your php version to 7.2.19@karousn this only works if you haven’t already updated to 7.2.20 though, right? In other words, this won’t roll your PHP back, but prevent the 7.2.19 PHP from being upgraded to 7.2.20. Or is that wrong?
@arturslogins, you might have to uninstall and reinstall, which could be a pain if you have lots of complexity in dependencies and configurations. Hopefully 7.2.21/7.3.8 are tagged and published soon (I don’t know how long it typically takes after RCs are tagged)
I try to find a workaround with composer config platform but it dosn’t work for “php” : “7.2.19” with 7.2.20 installed !
And 7.2.21 RC1 as well. Yay. 🎉
@nikic this proposal seems like a nice idea, WDYT? The current situation is pretty scary for 7.4: it’s quite common to check for optional dependencies with a class_exists(), with resiliency relying on the possibility to recover from missing parents.
@ostrolucky this logic exists in Config and in Cache components. Fixing the symptoms in DI still leaves the root issue open, which is what we’re discussing here.
@radyash see https://github.com/symfony/symfony/issues/32395#issuecomment-510185363
I am experiencing this same issue after updating to 7.2.20 this morning. FYI I do have the Twig, Form, and Validator dependencies that are mentioned in the OP. I am seeing the error thrown by a Test class in a custom vendor dependency that my team is responsible for–I am not sure why a dependency’s test files are being parsed for caching.
Anyways, after reading the comments above, I am slightly confused. Do we think solving this requires making a change to Symfony or to PHP itself? If the later, are we really thinking the fix would only be deployed to 7.4.x and not 7.2.x and 7.3.x, or am I misundertsanding your most recent post @smoench? If I’m not, that would be highly unfortunate…
@smoench that would be an option, yes. However, there are a few more such services that were ok previously because they would be removed from the container and were never used. If this can’t be solved in the DI container, I’m open for such a change, but I’d rather not do that preemptively if it can be avoided.
Another solution would be fixing the doctrine bundle with registering the Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser only as a service if the FormTypeGuesserInterface exists.
It contains the revert of the fix that caused the issue. https://github.com/php/php-src/commit/22ed362810c1b3a5ecb54ebd1d50d804c7fc3159
@teohhanhui got to downgrade back to php7.2. < 7.20 😦
api-platform users (im on mac using brew 7.3.7) - I had to add this to my composer.json file and all works after an update
"symfony/form": "4.2.*",Thanks for explanation @bocharsky-bw , so after some search to downgrade the php version to 7.2.19 you have two choices :
As example :
apt-cache policy php7.2# to check if you have php7.2.19 in version tablesudo nano /etc/apt/preferences.d/php72# create a file to add preferencesPut this in
After that you need to update & upgrade
sudo apt update -ysudo apt upgradeit well be the same for 7.3 version.
How to downgrade 7.2.20 to 7.2.19 ? i can’t find any example 😦 i’m not using docker
Hey @karousn , the problem in the PHP version, not in Composer dependencies. Composer config platform version only affects installed dependencies. Just upgrade to 7.2.21 or downgrade to 7.2.19 your PHP version and do nothing in your project
I have this issue when upgrading to php 7.2.20
classLogoutUrlHelper extends Helper <<< class Helper does not exists@teohhanhui it’s not released yet
Just for information, I experience this bug because of
symfony/security-bundlewhich enforce a few services definitions to be loaded for various templating engines. In theSecurityExtensionclass it calls$loader->load('templating_php.xml');which leads the container to have services which references classes from thesymfony/templatingcomponent (which is not installed).Thank you for the detailed explanation, @nikic! Return type contravariance really complicates this. 😕
@nikic Can we find another way to make autoloading failures recoverable somehow? To stay in your example, what the
ClassExistenceResourcetries to do here is to find out if the classFoois available. If the answer is no because the interfaceBaris missing, that’s fine. Nobody wants an incomplete class in that case.No, php itself has always triggered an uncatchable fatal error, see https://3v4l.org/jogPE
The trick in
ClassExistenceResourcewas to register a temporary proxy class loader that throws an exception instead that could be caught and handled. This does not work anymore, see https://3v4l.org/sSSJQ