Description
This issue is a follow up of #32395, so that we can focus on the underlying cause and skip the forum it has become. The closest related PHP bug report is https://bugs.php.net/78351, but it has been described as a feature request, while this is really a blocker for supporting PHP 7.4 as of now in Symfony.
PHP 7.4 support is almost done - all remaining issues have pending PRs. This one currently triggers phpunit warnings so that we can spot the affected test cases easily. Here is an example job that has the warnings: https://travis-ci.org/symfony/symfony/jobs/568248854, search for PHP 7.4 breaks this test, see https://bugs.php.net/78351
.
In case anyone wonders why we built this resiliency, the related failing test cases highlight an interesting variety of use cases, summarized in #32995 (comment)
See #32995 (comment) for a reproducer.
@nikic described what happens in PHP7.4 with this example:
<quote>
// A.php
class A {
public function foo($x): B {}
}
// B.php
class B extends A {
public function foo($x): C {}
}
// C.php
class C extends B implements DoesNotExist {
}
// main.php
new C;
What happens now is the following:
- Autoload
C
. The classC
is registered provisionally. - Autoload the parent
B
. The classB
is registered provisionally. - Autoload the parent
A
. The classA
is registered provisionally. - The class
A
is verified (trivially) and registered fully. - The class
B
is verified and registered fully (and may already be used). During the verification it makes use of the fact thatC
is a subclass ofB
, otherwise the return types would not be covariant. - Autoload the interface
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 class C
either, because then a new class C
that is not a subclass of B
could be registered, and thus violate the variance assumption we made above.
</quote>
This leaves little room but @ausi suggested this:
<quote>
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 B is verified it would mark class C as “in use”. When loading DoesNotExist throws the exception then, we would throw a fatal error because class C is “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.
</quote>
Alternatively, @smoench proposed to use https://github.com/Roave/BetterReflection to do the check we need. But I fear this would have an unacceptable performance impact. Compiling the container is already slow enough, we cannot add a preflight parsing of every service class I fear.
I'm stuck, help needed :)