Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

PHP 7.4 breaks resiliency when loading classes with missing parents #32995

Copy link
Copy link
Closed
@nicolas-grekas

Description

@nicolas-grekas
Issue body actions

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:

  1. Autoload C. The class C is registered provisionally.
  2. Autoload the parent B. The class B is registered provisionally.
  3. Autoload the parent A. The class A is registered provisionally.
  4. The class A is verified (trivially) and registered fully.
  5. The class B is verified and registered fully (and may already be used). During the verification it makes use of the fact that C is a subclass of B, otherwise the return types would not be covariant.
  6. 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 :)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.