[FrameworkBundle][Config] Ignore exceptions thrown during reflection classes autoload

[fancyweb]

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32499 with PHP 7.3+
License	MIT
Doc PR	-

The behavior when an exception is thrown in a class loader changed in PHP 7.3 (cf https://3v4l.org/OQPk9). That means that the throwOnRequiredClass trick that is done in the parent class of these cache warmers (AbstractPhpFileCacheWarmer) does not work anymore with PHP7.3+.

[nicolas-grekas]

I'm not sure we need to work around since the new behavior has been reverted on <7.4
See #32395
For 7.4, there are more places that need to be changed, and I'd like we really understand the best way to fix the root issue.

[nicolas-grekas]

Hmm, this is different and discussed in https://bugs.php.net/74372
@nikic can we consider fixing the BC break by having the new behavior only for child classes of Error and let Exception behave as before?

[nikic]

@nicolas-grekas I don't think introducing an Error/Exception distinction here makes sense. If you want to ignore exceptions, please ignore them explicitly.

We could revert from 7.3 and introduce in a later version, but given that we're at 7.3.8 already and this change is explicitly called out in the migration guide, I'm not sure doing that is a good idea. You'll have to deal with it at some point in either case...

[nicolas-grekas]

@nikic thanks for the feedback, OK.

@fancyweb I think the patch should be only this one:

--- a/src/Symfony/Component/Config/Resource/ClassExistenceResource.php
+++ b/src/Symfony/Component/Config/Resource/ClassExistenceResource.php
@@ -76,11 +76,12 @@ class ClassExistenceResource implements SelfCheckingResourceInterface, \Serializ
 
             try {
                 $exists = class_exists($this->resource) || interface_exists($this->resource, false) || trait_exists($this->resource, false);
-            } catch (\ReflectionException $e) {
+            } catch (\Throwable $e) {
                 if (0 >= $timestamp) {
                     unset(self::$existsCache[1][$this->resource]);
                     throw $e;
                 }
+                $exists = class_exists($this->resource, false) || interface_exists($this->resource, false) || trait_exists($this->resource, false);
             } finally {
                 self::$autoloadedClass = $autoloadedClass;
                 if (!--self::$autoloadLevel) {

The other places should not be changed (according to my current understanding at least.)

[nicolas-grekas]

OK, I missed the } catch (\ReflectionException $e) { in warmers, I understand how the issue propagates here. We should factorize this logic now, inside throwOnRequiredClass I believe.

[fancyweb]

@nicolas-grekas Just pushed all tests + a deduplication of the code. I don't think it was what you wanted so I'm waiting for your feedback 😁

[nicolas-grekas]

ClassExistenceResource::isFresh() is also affected + any direct user of PhpArrayAdapter::throwOnRequiredClass() would too.
The plan could be:

• factorize in ClassExistenceResource
• find a way to use ClassExistenceResource instead of PhpArrayAdapter in FrameworkBundle
• don't patch PhpArrayAdapter but deprecate its throwOnRequiredClass() in 4.4

[fancyweb]

@nicolas-grekas

factorize in ClassExistenceResource

Done.

find a way to use ClassExistenceResource instead of PhpArrayAdapter in FrameworkBundle

symfony/config is a requirement of the FrameworkBundle so it's alright.

don't patch PhpArrayAdapter but deprecate its throwOnRequiredClass() in 4.4

Actually this method was introduced directly as @internal, so I just removed it.

[fancyweb]

This is another fix / improvement. Moving this catch on every calls allows to not fail totally on first fail but just on each failing cases.

[nicolas-grekas]

Thank you @fancyweb.