Symfony: PHP 7.4 breaks resiliency when loading classes with missing parents

[nicolas-grekas]

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 https://github.com/symfony/symfony/issues/32995#issuecomment-519026061

See https://github.com/symfony/symfony/issues/32995#issuecomment-523906657 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 :)

[vudaltsov]

My first silly alternative: a subprocess. If it fatals out, class exists 😂

use Symfony\Component\Process\PhpProcess;

require_once $autoloadScript = __DIR__.'/vendor/autoload.php';

function _class_exists(string $autoloadScript, string $class, bool $autoload = true): bool
{
    $process = new PhpProcess(sprintf(
        '<?php require_once %s; exit(class_exists(%s, %s) ? 0 : 1);',
        var_export($autoloadScript, true),
        var_export($class, true),
        var_export($autoload, true)
    ));

    return 1 !== $process->run();
}

var_dump(_class_exists($autoloadScript, Fatality\InvalidClass::class));

[derrabus]

The problematic part of the current covariance implementation is step 5: Class B in the example is marked as valid, under the premature assumption that class C is valid as well. Afterwards, the assumption turns out to be wrong, which basically means that the php runtime is in an erroneous state it cannot recover from.

To me, this does not sound like a completely unsolvable problem, but I know too little about the php core to actually contribute to a better solution here. 😕

[terjebraten-certua]

One solution could be for php to make a difference between "is_a", "is_subclass_of" kind of functions and the regular "new" operator.

At the start of an "is_a" or ""is_subclass_of" function php could "bookmark" its state (regarding classes loaded) and then revert to that state if it runs into that kind of fatal error, and convert the fatal error into a regular php exception. Just my thoughts about what could be done.

[nicolas-grekas]

@vudaltsov that'd work but would be very slow. An alternative might be to use php -S. Another alternative is pcntl_fork, but that isn't portable.
@derrabus yep, that's what @ausi proposed I think. This looks the most promising, but we definitely need help from someone from php-internals. @jpauli maybe?
@terjebraten-certua looks a lot like https://bugs.php.net/78351, which also requires help from php-internals but looks more restrictive than a generic low-level behavior.

[nikic]

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.

This sounds possible, in principle. I'm a bit wary because it also makes the behavior unreliable -- you may or may not get an error depending on pretty random changes in the class hierarchy (such as swapping the order of implementation of two interfaces).

In case anyone wonders why we built this resiliency, the related failing test cases highlight an interesting variety of use cases.

Can you point to some of the use cases? My gut reaction here is that Symfony is doing something ... not great and should (in the long term at least) move away from doing it.

[nicolas-grekas]

Can you point to some of the use cases?

Sure, thanks for asking!

Here is what I gathered:

• During cache warm-ups, we load all classes from some folders and look for annotations.
• During autowiring, we load all classes from some folders and either report missing parents with a user-friendly error message or skip the error when the related services happen to be not used (we cannot know beforehand which services are going to be used).
• When configuring the application, we do feature-testing to discover which of the optional dependencies are installed or not. Doing so typically means calling class_exists and not fail hard in the case it has a missing parent (which we don't know anything about when doing the call).
• During dev, we run some logic at every request that auto-triggers a cache clear when the container is out of sync. Not being in sync sometimes involve a parent class that goes missing for whatever reasons.
• When unserializing a cache payload, we want to be able to generate a miss instead of a fatal error when the payload is obsolete because the code itself changed.

All these cases must be resilient to missing parents since it's just fine ignoring the failure and skipping the class. Missing parents happen all the time when optional dependencies are not installed in the vendor/ folder. It also happens regularly during development, when you're not done yet. Being able to provide an accurate and contextual error message is critical for the developer experience.

[fancyweb]

If the behavior stays in PHP itself, I think we can resolve our usage in Symfony by actually creating the missing classes / interfaces / traits on the fly in an autoload function, mark them with a special interface / special trait, and then check through reflection if the loaded class / interface / trait implements or use the marker.

Something like that (use nikic/php-parser) :

spl_autoload_register(function ($class): void {
    /*if ($class === $this->resource) {
        return;
    }*/

    $needingClass = null;
    foreach (debug_backtrace() as $backtrace) {
        if ('spl_autoload_call' === $backtrace['function'] && $backtrace['args'][0] !== $class) {
            $needingClass = $backtrace['args'][0];
        }
    }

    if (!$needingClass) {
        return;
    }

    $traverser = new NodeTraverser();
    $traverser->addVisitor(new class($class) extends NodeVisitorAbstract {
        private $class;

        public function __construct(string $class)
        {
            $this->class = $class;
        }

        public function enterNode(Node $node) {
            if ($node instanceof Class_) {
                foreach ([
                    $node->implements,
                    $node->extends ?: [],
                ] as $i => $items) {
                    foreach ($items as $item) {
                        if ((string) $item !== $this->class) {
                            continue;
                        }

                        if (0 === $i) {
                            $code = $this->getInterfaceCode();
                        } else {
                            $code = 'class %s implements \Symfony\Component\Config\Resource\MarkerInterface { }';
                        }

                        break;
                    }
                }

                // If the needing class does not implements or extends the wanted class, it has to be a trait.
                $code = $this->getTraitCode();
            } elseif ($node instanceof Interface_) {
                $code = $this->getInterfaceCode();
            } elseif ($node instanceof Trait_) {
                $code = $this->getTraitCode();
            } else {
                return;
            }

            if (false !== $len = strrpos($this->class, '\\')) {
                $code = 'namespace '.substr($this->class, 0, $len).'; '.$code;
                $class = substr($this->class, $len + 1);
            }

            eval(sprintf($code, $class));

            return NodeTraverser::STOP_TRAVERSAL;
        }

        private function getInterfaceCode(): string
        {
            return 'interface %s extends \Symfony\Component\Config\Resource\MarkerInterface { }';
        }

        private function getTraitCode(): string
        {
            return 'trait %s { use \Symfony\Component\Config\Resource\MarkerTrait; }';
        }
    });

    $traverser->traverse((new ParserFactory())->create(ParserFactory::ONLY_PHP7)->parse(file_get_contents((new \ReflectionClass($needingClass))->getFileName())));
});

$hasMarkerTrait = static function (\ReflectionClass $refl) use (&$hasMarkerTrait): bool {
    foreach ($refl->getTraits() as $trait) {
        if (MarkerTrait::class === $trait->getName()) {
            return true;
        }

        return $hasMarkerTrait($trait);
    }

    return false;
};

class_exists(ClassWithNotExistingParentOrInterfaceOrTrait::class);

$refl = new \ReflectionClass(ClassWithNotExistingParentOrInterfaceOrTrait::class);
if ($refl->implementsInterface(MarkerInterface::class) || $hasMarkerTrait($refl)) {
    throw new \ReflectionException();
}

[vudaltsov]

Let me abstract away from the PHP language and look from a logical point of view.

To me there is a contradiction between the fact that class B is valid and usable (so new B works fine) and the fact that class_exists(C::class) throws an error.

B::foo requires "C is a subclass of B" and that's true. But that also implies that C exists, right? Otherwise how can it be a subclass of smth if it does not exist? At the same time class_exist does not return true for C. Thus, a contradiction.

If we get back to PHP, I understand that internally class_exists is not only a conjunction of "is class" and "exists" predicates, but also "is valid", since it needs to load the class by design. But to me this should not be unrecoverable due to the existing contradiction.

[nicolas-grekas]

@fancyweb that would be very fragile, because then the related behavior of standalone components (eg cache miss instead of fatal error) would be tightly coupled to this very special autoloader.
@vudaltsov that's https://bugs.php.net/78351 again, but that's asking php-internals for doing more work than strictly required to build the target behavior in PHP. I prefer having the engine provide us low-level primitives ("possible resiliency" here) and then it's up to frameworks to package that into useful abstractions. Let's reduce the burden on C-authors please.

[pounard]

I think there's a few steps that could be improved to lower the bug from happening.

During cache warm-ups, we load all classes from some folders and look for annotations.

OK this one is tricky, and this is why I don't love annotations and where I work we don't use them (at all).

During auto-wiring, we load all classes from some folders and either report missing parents with a user-friendly error message or skip the error when the related services happen to be not used (we cannot know beforehand which services are going to be used).

The human friendly reporting is useful and nice, but it's not necessary. Developers are developers, they can debug themselves. Moreover, auto-wiring tends to create false positives with errors and enforce to strict file naming which sometime can be a pain. From my perspective, auto-wiring should only be used for final application code, not within bundles (they never be using it, especially to be much more strict and resilient to this king of magic-triggered errors), considering that opinion, if a parent class is missing, just let it crash.

When configuring the application, we do feature-testing to discover which of the optional dependencies are installed or not. Doing so typically means calling class_exists and not fail hard in the case it has a missing parent (which we don't know anything about when doing the call).

In that case, class_exists() can be called on prior classes in the hierarchy, and most of this bug occurrences could be solved that way, I think.

During dev, we run some logic at every request that auto-triggers a cache clear when the container is out of sync. Not being in sync sometimes involve a parent class that goes missing for whatever reasons.

Isn't this failing because of the two previous points ?

When unserializing a cache payload, we want to be able to generate a miss instead of a fatal error when the payload is obsolete because the code itself changed (typically when deploying to prod.)

A manual cache clear when delivering should actually solve that. We do force manual cache clear on every delivery on every project and I think that most people also do it. Even if Symfony is very resilient with cache, we still experience bad bugs or edge case behaviours quite often not doing so. Trying to let the framework auto-obsoleting its cache payload on delivery seems a very dangerous thing to do. In any case, if you are attempting a hotfix or critical prod bugfix and don't want to interfere with runtime performance during that time, or avoid unnecessary downtime, your developer must write a proper, finer deploy procedure that manually drops what needs to be dropped.

[nicolas-grekas]

@pounard that'd mean undoing the progress of many years of contributions. We built all these behaviors because users reported they would be desired. We should try harder to me.

[terjebraten-certua]

The problem (in a nutshell) is that you can't safely call class_exists() on a class without in some special cases risk running into a fatal error.

That happens in this case if the class you are testing does kind of exist, but is invalid because it is missing an interface. It would be much better to be able to get a false result without getting a fatal error. You cannot catch fatal errors.

[terjebraten-certua]

@vudaltsov that's https://bugs.php.net/78351 again, but that's asking php-internals for doing more work than strictly required to build the target behavior in PHP. I prefer having the engine provide us low-level primitives ("possible resiliency" here) and then it's up to frameworks to package that into useful abstractions. Let's reduce the burden on C-authors please.

What are you asking for then, that is less than https://bugs.php.net/78351?

I am afraid I missed that.

[linaori]

This sounds very much like an issue that php should be solving, as it's also php changing this behavior. Until then, it sounds semi-reasonable to have the container first collect the class definitions and in a sub-process and tries to autoload them. While not an ideal solution, I can't think of another sandboxed way of attempting to load it without bringing php in a state where it can't continue.

If php could solve it, class unloading would probably what it needs, but I'm no expert on this.

[pounard]

that'd mean undoing the progress of many years of contributions

I think you're exaggerating a bit, it's not about removing everything of auto-wiring or caching, just refining a bit a few bits to be more resilient.

I think doing more specialized class_exists() calls that start with classes prior in the hierarchy wouldn't hurt at all usability, it would just need bundle and core developers to be a bit more careful, and I guess this would get rid 90% of sides effects due to this PHP change. All that happened on my projects actually could have been avoided doing so.

[nikic]

Possibly a stupid suggestion: Instead of trying to catch missing interfaces during inheritance, why not avoid them in the first place?

<?php

if (!interface_exists(OptionalComponent\Bar::class))
    return;

class Foo implements OptionalComponent\Bar {
}

This seems like a more robust, clearer and faster solution than trying to declare the class and then recovering from a failure. It clearly shows that the class is optional and will correctly work with a vanilla autoloader and class_exists().

I think that independently of the use of the class loading resiliency mechanism for specific purposes (like cache warmup), what Symfony imho does wrong and should preferably stop doing as soon as feasible, is to bake this functionality into the general class_exists() functionality. class_exists() should not implicitly and silently recover from a missing interface, for much the same reason it should not silently recover from a parse error in the loaded file. A priori, a missing interface is just a missing interface (say a typo) and should never be silently ignored. Symfony is hijacking basic language behavior here, in a way that creates incorrect expectations that do not hold anywhere outside the Symfony ecosystem. Using something like this internally is fine, but exposing it to users in such a manner is not.

[pounard]

This seems like a more robust, clearer and faster solution than trying to declare the class and then recovering from a failure

@nikic I'm not a native english speaker, I think that it is exactly what I was suggesting.

I think that independently of the use of the class loading resiliency mechanism for specific purposes (like cache warmup), what Symfony imho does wrong and should preferably stop doing as soon as feasible

Actually it does bring a lot of goodness for lazy developers, it allows very quick Symfony app setup and it's very nice to use. But I agree, this is black magic. Symfony dependency-injection component should be more resilient and more careful, I'm sure there are solutions that would allow the same level of comfort for users with less magic.

@nicolas-grekas I agree that having PHP be a bit more robust regarding this behaviour would be much easier for Symfony, and probably some other magic-based frameworks, but it doesn't prevent you from preparing for the case where PHP developers would mark this as a wont-fix. To be honest, I'm glad that PHP doesn't allow broken code to be loaded, and VM kept in a broken state.

[nicolas-grekas]

@nikic

Instead of trying to catch missing interfaces during inheritance, why not avoid them in the first place?

Because we don't know the parents. Tight coupling to the class hierarchy is fragile and doesn't work across all versions of dependencies since hierarchies change. And even if we were, that wouldn't fix the other use cases.

not hold anywhere outside the Symfony ecosystem

The Symfony ecosystem is the PHP ecosystem, see how many rely on the components. Many packages rely on the behavior seamlessly and would learn about the topic in a bad way.

Having the engine kill itself on a class_exists check is equally wrong to me and that's why we're asking for help here.

@pounard

Symfony dependency-injection component should be more resilient and more careful

Resiliency when a fatal error occurs is not possible.

Nobody is asking to load broken code in the engine btw.

[mmarton]

What about something similar than https://www.php.net/manual/en/reflectionclass.isinstantiable.php?
A native php function that tells if the class and all of its parents/interfaces/traits are exists without running to fatal error.

[derrabus]

Possibly a stupid suggestion: Instead of trying to catch missing interfaces during inheritance, why not avoid them in the first place?

<?php

if (!interface_exists(OptionalComponent\Bar::class))
    return;

class Foo implements OptionalComponent\Bar {
}

That would be doable, however that would decrease the DX as well.

In this example, when attempting to use the Foo class directly, the developer would not get any feedback anymore on why the class is not available.

The code

new Foo();

would normally result in

Fatal error: Interface 'OptionalComponent\Bar' not found

and with your change applied, the error would be

Fatal error: Uncaught Error: Class 'Foo' not found

That error message would rather confuse people, I'm afraid.

[nikic]

Because we don't know the parents. Tight coupling to the class hierarchy is fragile and doesn't work across all versions of dependencies since hierarchies change. And even if we were, that wouldn't fix the other use cases.

I don't understand what you mean here. To be clear, what I have in mind is a check when the class is declared, not when someone is trying to use it.

If a class has optional dependencies, then it is an optional class itself -- it should only be declared if the dependencies exist.

not hold anywhere outside the Symfony ecosystem

The Symfony ecosystem is the PHP ecosystem, see how many rely on the components. Many packages rely on the behavior seamlessly and would learn about the topic in a bad way.

That sounds worse: People should not be implicitly pulling in non-standard behavior for class_exists() just because they happen to use a Symfony component.

[derrabus]

That sounds worse: People should not be implicitly pulling in non-standard behavior for class_exists() just because they happen to use a Symfony component.

To be clear: We're still talking about internal logic of the DI component that is executed during cache warmup. Userland code still gets the vanilla behavior of class_exists.

[pounard]

Because we don't know the parents. Tight coupling to the class hierarchy is fragile and doesn't work across all versions of dependencies since hierarchies change. And even if we were, that wouldn't fix the other use cases.

When a bundle auto-configures itself to optionally use classes from another packages, if the case arise, it's a good thing to check deeper in the class hierarchy. It seems to be a sane approach. Moreover, by checking some interface or class higher in the class hierarchy, if it disappear since a version, or was introduced at a specific version, it also covers the versions you didn't anticipated and deactivate gracefully the feature in non anticipated version constraints usages.

[nicolas-grekas]

To be clear, what I have in mind is a check when the class is declared, not when someone is trying to use it.

Oh sorry I read too fast. This could work only for code we have control over. But there are many third-party packages that will never accept to add these checks. Also, from my experience, PHP (maybe opcache?) ignores such early return statements sometimes and loads the declarations before executing the code itself. That's why we always wrap such conditional classes inside "if"s. Anyway, doing this for every single class would be ugly and would be a workaround for an issue that lies in the engine really.

Ideally, the engine should never "fatal error", because that means taking control away from userland. That's exactly why Error has been introduced. Adding a new fatal error when some control was still possible before is what breaks fundamentally...

[pounard]

I don't understand what you mean here. To be clear, what I have in mind is a check when the class is declared, not when someone is trying to use it.

I didn't understood this as well, sorry.

[pounard]

Adding a new fatal error when some control was still possible before is what breaks fundamentally...

To be fair, I don't think avoiding all panics is possible, no matter the language, the VM, nor how hard you try. It's sad Symfony actually stumbles upon this one.

[terjebraten-certua]

You can take https://github.com/symfony/symfony/issues/32396 as an concrete example.

The class SymfonyBridgeDoctrineFormDoctrineOrmTypeGuesser implements SymfonyComponentFormFormTypeGuesserInterface

But if you in your composer.json do not have symfony/form and it is not pulled in by any other package you use, then the Form classes and interfaces will not exist in the vendor directory.

Then the symfony DI compenent needs to make sure that SymfonyBridgeDoctrineFormDoctrineOrmTypeGuesser is also not available as a service. But the problem is that it cannot do so without stumbling into a fatal error.

I myself got this error when using ApiBundle, where we have no use of Forms. Having Symfony just remove the redundant service SymfonyBridgeDoctrineFormDoctrineOrmTypeGuesser would have been expected behaviour.

[pounard]

@terjebraten-certua this is a real question, I don't understand at which point you really cannot check for the interface existence before registering the service. Can't you do a if (interface_exists('\Symfony\Component\Form\FormTypeGuesserInterface')) prior to attempt to check if DoctrineOrmTypeGuesser exists during bundle configuration ?

[terjebraten-certua]

The error came from the doctrine-bridge code, not my own application code.

The doctrine-bridge depends on the Symfony autowiring to remove the unused service SymfonyBridgeDoctrineFormDoctrineOrmTypeGuesser if it is not needed.

For me the error came out of the blue, because I was not using any of that code in my app.

[terjebraten-certua]

See also https://github.com/symfony/symfony/issues/32395#issuecomment-509748876

And the reply to that comment: https://github.com/symfony/symfony/issues/32395#issuecomment-509749529

[pounard]

@terjebraten-certua I see, thanks for the answer.

[deleugpn]

Sorry my intrusion, I'm just a curious person that landed here accidentally and is trying to learn. I failed to reproduce Nikita's code as it seems to be invalid on PHP 7.4.0beta4 (https://3v4l.org/4bHiF).

I was trying to understand vudaltsov's comment:

B::foo requires "C is a subclass of B" and that's true. But that also implies that C exists, right? Otherwise how can it be a subclass of smth if it does not exist? At the same time class_exist does not return true for C. Thus, a contradiction.

Nicolas' reply was that this is asking for https://bugs.php.net/bug.php?id=78351 but I fail to see that. The ticket requests for a possibility to check if a class exists without fatal error while vudaltsov's comment ask the engine to not allow for class B to be valid and it appear as if that is already the case. We cannot load class B because it requires C to exist and we cannot declare class C first because it extends B.

My question now is: if vudaltsov is right and B should not be valid in the first place and that appears to be the case already due to circular dependency, does that nulify the entire premise of the problem? Does the fact that Nikita's code throw a different fatal error changes the course of the discussion at all since it starts with that snippet of code or am I dead wrong?

[nicolas-grekas]

@deleugpn here is the code that highlights the BC break:

spl_autoload_register(function ($c) {
    if ('ChildClass' === $c) {
        class ChildClass extends ParentClass {}
    }
    if ('ParentClass' === $c) {
        throw new \Exception('ParentClass not found.');
    }
});

try {
    new ChildClass();
} catch (\Exception $e) {
    echo $e->getMessage();
}

As you can see on https://3v4l.org/uh8ae, PHP 7.4 takes control away from userland, by throwing a fatal error and thus forbidding to implement any resiliency for now (7.2.20 too, but 7.2.21 is fixed).

[terjebraten-certua]

I think @deleugpn has a point. He pointed out that the example @nikic came with in https://github.com/symfony/symfony/issues/32395#issuecomment-509593220 is invalid on PHP 7.4.0beta4.

Since that was the example @nikic used to argue that the php engine must throw a fatal error, I guess it is relevant to ask, if @nikic was wrong about this, if this would change the discussion.

[nikic]

@deleugpn This was a simplified example, here is a working example with the autoload boilerplate: https://3v4l.org/AhD6U

spl_autoload_register(function($class) {
    if ($class === 'A') {
        class A {
            public function foo($x): B {
                return new B;
            }
        }
    } else if ($class === 'B') {
        class B extends A {
            public function foo($x): C {
                return new C;
            }
        }
    } else if ($class === 'C') {
        class C extends B {
        }
    }
});

(new C)->foo('blah');

[terjebraten-certua]

If you look at https://3v4l.org/jMYYP it is not possible to instantiate class B. So why does it have to be a fatal error then? No objects of class B can be in use, because they cannot be instantiated.

[nikic]

https://3v4l.org/oUoG2

[terjebraten-certua]

It means that it is only when you are in the autoloader you can instantiate those half baked objects of class B.

Could not the PHP engine take a snapshot of the valid classes before it enters the autoloader, and then if an impossible situation like this arises, go back to that known good state and exit the autoloader with a catchable exeption instead of throwing a fatal error?

[deleugpn]

I think that would cause a bigger mess at the internals side since it has to cater for a rollback of not only class definition, but also removal of instantiated and partially valid objects that could have been invoked (https://3v4l.org/IrGfU). To recover from that and have the instance of B magically vanished would allow for so many weird possibilities.

[nikic]

Could not the PHP engine take a snapshot of the valid classes before it enters the autoloader, and then if an impossible situation like this arises, go back to that known good state and exit the autoloader with a catchable exeption instead of throwing a fatal error?

I don't understand the suggestion. In the above example you already have a live object of the class -- you can't roll back the class state, otherwise you have an object of a class that no longer exists.

Independently of that though: Anything that involves "unregistering" classes past a very early point (prior to any inheritance) is technically infeasible, at least for the 7.4 timeframe. Class registration is a destructive process, which converts a class template into a registered class. If the class is unregistered, we would need to restore that template though, because there may be an attempt to re-register the class at a later point in time (e.g. after loading the missing dependency). Supporting this is principally possible, but it does require a full rewrite of the class inheritance system, which is not going to happen for 7.4.

[terjebraten-certua]

When you are in the autoloader you should be defining classes, not do other things. Why not simply forbid instatiation of objects like B in the autoloader. I think that is the real bug here.

Why is it difficult to detect in the autoloader that B might be invalid? It should stop there before any objects of class B is made. Then it is possible to recover.

[deleugpn]

That doesn't seem a reasonable request either. The autoloader is a way to delegate the responsibility of loading classes to PHP by internals. It's providing a chance of doing whatever we want to place the engine in a stable state and if not reached successfully, the engine will crash as it was already about to crash anyway with a class not found. Saying the autoloader can do 'whatever it wants but not really' sounds like a control nightmare.

[terjebraten-certua]

You cannot instantiate B outside the autoloader. I am just asking for the same to be the case inside the autoloader.

It think it is better that the autolader is a bit more strict in what it considers to be valid classes, than allowing this mess.

[nicolas-grekas]

If that can help, I'm mostly concerned by the BC break for existing apps. If we can figure out a way that still breaks for newly written code that requires covariance, but doesn't for code compatible with < 7.4, that'd fix the issue on my side. What @ausi describes would work for me IIUC.

[nikic]

When you are in the autoloader you should be defining classes, not do other things. Why not simply forbid instatiation of objects like B in the autoloader. I think that is the real bug here.

If a class declaration succeeds, it stands to reason that the class will be usable subsequently -- an earlier prototype did delay availability of classes inside autoloaders, but we decided that this behavior is unacceptable. If you don't like the new B example, think of it as class_alias('B', 'Foo') or class_exists('B') instead, things that you are more likely to find inside an autoloader context, but uses of the class nonetheless.

You cannot instantiate B outside the autoloader. I am just asking for the same to be the case inside the autoloader.

The only reason you "can't" instantiate it, is because a fatal error occurred in the meantime. For example, you can instantiate it in a shutdown handler. The class is either there or it isn't, we don't make it available and then drop it again.

If that can help, I'm mostly concerned by the BC break for existing apps. If we can figure out a way that still breaks for newly written code that requires covariance, but doesn't for code compatible with < 7.4, that'd fix the issue on my side. What @ausi describes would work for me IIUC.

As a pure BC solution, yes, I think doing something like this is reasonable. But I haven't seen any movement here to treat it as such, i.e. there doesn't seem to be any movement towards being able to remove this functionality in a future version of Symfony. As I wrote before, I believe that https://github.com/symfony/symfony/issues/32995#issuecomment-519063995 is the correct long-term solution to your problem. (It may help to step away from the whole "autoloading" issue and consider how you would have to write your code if the whole library was in one file -- the need to declare classes conditionally would become obvious then.)

[teohhanhui]

consider how you would have to write your code if the whole library was in one file -- the need to declare classes conditionally would become obvious then

PHP's autoloading system is very broken if users have to write conditional classes. You don't have to do it in Java, you don't have to do it in C++, ...

[linaori]

In java or C++ it won't compile either if you extend a class or implement an interface that doesn't exist. The problem here is that certain classes are being autoloaded for inspection while they are optional and should only be loaded if in specific scenarios (e.g. when the other class is present). Or at least, this is what I understand from the issue and how Symfony works.

[nikic]

you don't have to do it in C++

Huh...? In C++ optional declarations are handled either using preprocessor conditions or build system conditions. Code that references a non-existing dependency will fail to compile even if it is itself unused.

[nicolas-grekas]

As a pure BC solution, yes, I think doing something like this is reasonable.

Cool, I'm looking forward to it :) Sorry I can't help with code...

But I haven't seen any movement here to treat it as such, i.e. there doesn't seem to be any movement towards being able to remove this functionality in a future version of Symfony.

If we're forced to do so thanks to the fatal error, we will :)
But right now we can recover from the situation and that saves us from writing boilerplate conditionals, which is nice as it makes maintenance a bit easier.

[stof]

also note that the need for conditional classes everywhere would not be limited to Symfony core. Any library doing something similar would also be affected.

And it would be great if a simple if (!interface_exists(OptionalComponent\Bar::class)) return; guard could work reliably instead of having to wrap the whole class inside a if to avoid OPCache issues

[teohhanhui]

So, if I understand correctly, there are 2 suggestions on what the right thing to do is:

1. It is the calling code's responsibility to check whether a class should be loaded or not, before triggering the autoloader. But this doesn't work as typically the calling code is not supposed to be aware of such implementation details.
2. Conditionally declare a class. But it's ugly, breaks the 1:1 mapping of PSR-4, and is probably not feasible as all existing code would have to be changed.

Which is why I still think it should be handled at the language level.

[deleugpn]

Since this is a major issue, I'm going to feel free to throw a bad idea here in case it leads to someone else recycling it into a good one.

What about a C extension? Maybe Symfony could try and find a C developer willing to write a small php extension in C code to change the autoload process to be compatible with the previous behavior and carry the extension forward?

[lisachenko]

@nikic is it possible on engine level to temporary put a pointer to the __PHP_Incomplete_Class class entry during the autoload? Then, if autoload fails, we will have a partially reconstructed class which will be valid for the engine, but restricted from being used on userland level (throw an exception about incomplete class during parsing).

[someniatko]

@nikic the problem with solution in the comment https://github.com/symfony/symfony/issues/32995#issuecomment-519063995 you suggest is that it requires to know about whole inheritance tree before attempting to use the class in any way. The symfony dependency injection component is abstract and does not have control over all possible classes that it may be fed with so it cannot know the inheritance tree beforehand.

UPD: well, the position in https://github.com/symfony/symfony/issues/32995#issuecomment-519074577 seems reasonable as well. Indeed it seems like the classes (and bundles' services) should be declared conditionally.

Conditionally declare a class. But it's ugly, breaks the 1:1 mapping of PSR-4, and is probably not feasible as all existing code would have to be changed.

@teohhanhui If autoloader is requested with an non-existing class (say without any interfaces, just non-existing), the 1:1 mapping will be also broken.

I think nikic is right saying that using a non-existing interface is the same level as having a syntax error in the file - it is just not permitted by the language itself.

[TerjeBr]

When you are in the autoloader you should be defining classes, not do other things. Why not simply forbid instatiation of objects like B in the autoloader. I think that is the real bug here.

If a class declaration succeeds, it stands to reason that the class will be usable subsequently -- an earlier prototype did delay availability of classes inside autoloaders, but we decided that this behavior is unacceptable. If you don't like the new B example, think of it as class_alias('B', 'Foo') or class_exists('B') instead, things that you are more likely to find inside an autoloader context, but uses of the class nonetheless.

But my whole point is that B should never be considered a valid class (since it makes use of class C in its signature)
The autoloader should fail using class B in the same way that the normal code cannot use class B.

You cannot instantiate B outside the autoloader. I am just asking for the same to be the case inside the autoloader.

The only reason you "can't" instantiate it, is because a fatal error occurred in the meantime. For example, you can instantiate it in a shutdown handler. The class is either there or it isn't, we don't make it available and then drop it again.

This is the problem, class B should not have been allowed to be available if the first place. That is why you have to throw a fatal error, right?

What I am saying, is that if in the autoloader you cannot yet be sure if class B is valid, (because you need to run the autoloader for class C first), then treat class B as not yet valid, and do not allow any use of it until it has been confirmed that it is a valid class.

If that can help, I'm mostly concerned by the BC break for existing apps. If we can figure out a way that still breaks for newly written code that requires covariance, but doesn't for code compatible with < 7.4, that'd fix the issue on my side. What @ausi describes would work for me IIUC.

As a pure BC solution, yes, I think doing something like this is reasonable. But I haven't seen any movement here to treat it as such, i.e. there doesn't seem to be any movement towards being able to remove this functionality in a future version of Symfony.

This is core functionality of the dependency container in Symfony. It needs to test if a class exists, and then not fatal error if that class has been made non-functional because of a missing interface.

It is a common thing when you install things with composer, that some piece of code will not be there if the dependency is not installed. Then symfony tries to work out how to configure the system without that dependency. In the process it needs to inspect code with classes that no longer can be instantiated, and remove those services from the container. But the problem is that in PHP 7.4 it cannot inspect code without risking running into a fatal error.

[pounard]

This is core functionality of the dependency container in Symfony

Working on a real project, our CI started to explode because of this behaviours as soon as PHP reached the version that triggered the bug (silly us, we didn't put version constraints and had a too bleeding edge package source on this env) and I had to explore and debug those failures. It's a fullstack Symfony project (we just don't use Doctrine ORM, but we have Doctrine DBAL nonetheless).

What we experienced is only two occurrences of the bug, two that could have been avoided pretty easily, shame on me I don't remember the actual compiler classes that failed, but the two were from Symfony itself, not external bundles, and writing if (interface_exists('foo') && class_exists('bar')) { in those compiler pass instead of if (class_exists('bar')) { has actually solved the bugs we had (we had to hotfix it until our admins could fix the env to propagate non-buggy PHP versions).

I don't think this worth a global, nor generic solution for Symfony. As soon as a bundle conditionally configures a component (where the only condition is a bar class existence), even in the case it comes from an arbitrary unmanaged external composer dependency, the developer that writes this compiler pass should know that this class itself has another conditional foo dependency and be extra careful when writing its extension/compiler class. It's not that much a burden, being careful, it's our job actually.

I think Symfony can survive this and adapt by putting a few extra class_exists() or interface_exists() whenever it reaches this point while remaining fully functional. Problem will happen for bundle developers, but end-users, people that writes applications and not bundles, should not have any problems with this, I mean, who on earth will write a business application that uses Schrödinger classes ? If anyone does, this means he/she's writing a product, but a product needs to be extra careful and, IMHO, should not use autowire but remain in control of every service it spawns.

It's of course, just an opinion and it is very subjective; I still think that the way to go is to open a new issue every time someone identifies an occurrence of this PHP behaviour trigger in Symfony, and fix it independently from the others. From my perspective, this issue is a non issue, it should be a coding convention in Symfony: be extra careful when conditionally checking for class existence.

[TerjeBr]

Yes, I am sure the symfony framwork could work around it somehow.

But I still think it is a language issue in PHP, that there is no way to safely check if a class exists or do other inspections on that class, if it may have missing interfaces. And with composer being a common standard to manage PHP packages, it will not be that unusal that if a package is not installed, it might leave some other classes outside the package with a missing interface. From PHP 7.4 this creates a mine-trap in the code, and it will be hard/impossible to write general code around it, but you have to make a special case each time.

[pounard]

I'm not sure this is a PHP global problem, if you're not writing an auto-configurable dependency injection container, you should probably never trigger this behaviour otherwise than when your code is actually bugguy or when you forgot a dependency. But in the end, a missing class exception is as easy to resolve than type composer req missing/package.

[teohhanhui]

How does the consumer check for optional dependencies? By calling class_exists, interface_exists, etc. And PHP will throw an uncatchable fatal error if you do that to anything that's intermediate, forcing you to trace things all the way up the inheritance chain. That means it's not feasible to do the checks from the consumer.

[TerjeBr]

I think you have misunderstood the common use case scenario. You do not want to type composer req missing/package, because you do not want that package. Your app does fine without it.

The problem is that either the framework itself, or some other package (that you do need) has defined some services, that depends on an interface from missing/package. Those services will never be used, because you do not have installed missing/package, and the dependency container code should normally be able to see that these services are not being used anywhere and remove them from the compiled service container. The problem is that it becomes very hard for the service container to do this job when it has to look out for a mine field of fatal errors because of missing interfaces.

[pounard]

I do understand, actually, I think, since I had to debug it along the way quite a few time. If A extends B, A is optional for You and B is optional for A, when you call class_exists('A') and B is not defined but A is, PHP fatals, I'm right I think ? So if instead you just call class_exists('B') && class_exists('A'), statements are run in this order, so it returns false for B, doesn't load incomplete A, and does not fatal.

If you're not autoconfiguring things, you don't call class_exists('A'), you just new A(); If it fatals, you add the required dependency to your project or you do not new A(), end of story.

[TerjeBr]

@pounard it is the code for the symfony container that has a hard time, not the packages themself or the user code that uses the packages.

The symfony container is supposed to be general, and has no knowledge of the class hierachy of the services it manages.

[teohhanhui]

If A extends B, A is optional for You and B is optional for A, when you call class_exists('A') and B is not defined but A is, PHP fatals, I'm right I think ? So if instead you just call class_exists('B') && class_exists('A'), statements are run in this order, so it returns false for B, doesn't load incomplete A, and does not fatal.

That is not feasible. As a consumer of A, you should not need to know its inheritance chain. It's simply not your responsibility. The "correct" alternative of declaring classes conditionally is also not feasible because of the amount of existing code that was not written that way. This change would break the PHP ecosystem and really should be reconsidered.

[pounard]

it is the code for the symfony container that has a hard time

Only when autowiring, otherwise the container only loads the services that were defined in the configuration. When you use widely used bundles, most of them don't autowire, they explicitely register their services using YAML or XML, case in which there is actually no problem otherwise than the double class_exists() to implement in certain edge cases.

Considering autowiring, I do think it's a mispractice to use it in bundles, bundles must remain in control, IMHO the autowiring feature should only be used only in your final application code. And once again, you never write conditional class includes in a final business application. If you do, I have hard conceiving how you could end up doing this.

[pounard]

That is not feasible. As a consumer of A, you should not need to know its inheritance chain. It's simply not your responsibility

I simply do not agree with this statement. When you start writing compiler classes with conditional includes and auto-configuration, it is seriously your responsibility to understand the code you're actually using, even though you didn't write it.

Moreover, it's PHP, you don't use opaque binary library files, you always the code alongside with you and your editor can navigate into it.

And to finish on this topic, if it was actually opaque, it would be the library you're using responsibility to NOT define a incomplete class. If you can't guess that's broken, it just shouldn't be, it's not Symfony's container responsibility to try to fix broken code out there. It only brings an extra layer of complexity in the dependency injection component, which is already complex and hard to debug.

[TerjeBr]

Moreover, it's PHP, you don't use opaque binary library files, you always the code alongside with you and your editor can navigate into it.

That you can do if you are a human and trying to put together your code. But the matter is different if you are a dependency container, and trying to collect services you know nothing about into a compiled and dumped to file service container.

[pounard]

I think that you are misunderstanding responsibilities, the container does nothing more than collect what's under the auto-wired declared namespaces and files, for the rest, it's explicit registration done by the bundles. In case of any explicit registration, the container does nothing at all, except running class_exists() during compilation maybe then die if class is not there (whether or not the behaviour triggers) to alert the developer he mis-defined a service. At least it was prior to 3.4, the whole container functioning was very straight-forward, even thought now it does more drastic optimisations, and added auto-wiring, it's not much more complex functionaly.

The bug triggered when bundles explicitly registered incomplete classes once the compiler check for the services definitions sanity, it was breaking. At least in the use cases I stumbled upon, it was always an explicit service registration problem with bundles, not with the compiler itself.

[TerjeBr]

it is the code for the symfony container that has a hard time

Only when autowiring, otherwise the container only loads the services that were defined in the configuration.

This is not true. You should study #32396 as an example.

Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser is declared as a normal service, and does not depend on any autowiring. The class implementing that service does depend on the interface Symfony\Component\Form\FormTypeGuesserInterface that is in the package symfony/form.

The service Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser is only intended to be used by the symfony/form package, and will be picked up by a compiler pass in the symfony/form package based on its tag. This was working fine long before autowiring was introduced in the container.

When the package symfony/form is not needed, that service is not needed neither. But the doctrine bridge did not need to do anything special to make that service "optional". It just expected that when no compiler pass picked up the service, it would just automatically fall out of the compiled container.

There are many other examples of packages providing optional services to other packages that may or may not be there. No auto-wire logic is involved.

[pounard]

The service SymfonyBridgeDoctrineFormDoctrineOrmTypeGuesser is only intended to be used by the symfony/form package, and will be picked up by a compiler pass in the symfony/form package based on its tag. This was working fine long before autowiring was introduced in the container.

Really, if the bundle lets the service be in the configuration when the class does not exists, it seems to be the bundle responsibility to remove it before compiling. You cannot just let an undefined service in the container and expect it to clean up your mess.

In my opinion, the doctrine bundle plays a dangerous game letting incomplete or non existing definitions in the container. The container cannot be held responsible for it if it breaks. The bundle lacks robustness.

[TerjeBr]

You cannot just let an undefined service in the container and expect it to clean up your mess.

Yes, that was a beautiful feature of the dependency container that worked for a long time.
It seems that coming the next version of PHP we must do without this feature.

It is a sad thing that a resilient and good dependency container can no longer have a feature it used to have, because of a BC break in the way PHP works.

[pounard]

Yes, that was a beautiful feature of the dependency container that worked for a long time.

I think that in this example, the doctrine bundle, it was more like a nice side effect.

As for fixing the doctrine bundle, it would be extremely simple: put every service form related within a Resources/config/form.xml file, and load it conditionally in the extension class. It would be a single xml file and a two-liner patch, easy, and backward compatible with older versions of Symfony.

[TerjeBr]

To fix the doctrine bundle, you will have to split the form related services out from doctrine/doctrine-bundle/Resources/config/orm.xml into a new file and add new logic to the class Doctrine\Bundle\DoctrineBundle\DependencyInjection\DoctrineExtension.

And this is just one example of many. When/if the dependency container must do without its above mentioned resilience feature, there will be many places and bundles that will need to be updated.

[pounard]

I guess that they'll need to be fixed. I'm not sure this bug will trigger that often, considering that it's conceptually extremely dangerous to have multiple level of optional class in a single inheritance tree, when it happens, it highlight, in my opinion, very complex and out of control code. Sometime, it takes a tiny change a behaviour in PHP to give an opportunity to developers to make their code more robust :)

[pounard]

@TerjeBr I am sorry if sometime I can be rude, I do not mean too, I'm not a native english speaker and sometime can be a bit rough in my words. I never meant to be insulting or mean. I know that, for Doctrine maintainers and all others, that contributing to open source is not always easy and maintaining packages is hard.

That said I did a PoC for Doctrine bundle: https://github.com/doctrine/DoctrineBundle/pull/1013

[linaori]

Wouldn't all problems be solved if services (classes) are only added to the container if you intend on initializing them? If you try to initialize them with missing dependencies, it rightfully crashes. The error message is a bit confusing for this though.

In the end the responsibility of the service registration lies with bundles (or your application), which means it can all be solved in bundles by doing feature checks, such as class or interface exists. For optional dependencies, you have to check whether the optionally present class or interface is present and that should be enough.

I don't exactly like the way PHP handles it, yet in the end it's an error that is caused by us forgetting to check the existence of the optional dependency, not PHP.

[pounard]

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 did a few benchmarks recently, and I was honestly surprised by the results on container compile phase, it was, on the project I benched, Twig warmup taking up to 50% of the time, and property access component doing whatever it does taking another 20% to 30%, in conclusion, container compilation was decently fast (I didn't suspected those results honestly).

Considering https://github.com/Roave/BetterReflection it would actually be a legit use case for it, and I expect it to be decently fast as well, hoping there's some kind of cache mechanism within (and if there's not, it would still be possible in last resort implementing one around it). There's one thing to consider thought, it brings a few (usually dev only) dependencies to production releases, amongst them nikic/php-parser, phpdocumentor/reflection-docblock and phpdocumentor/type-resolver, all with relatively recent versions. I personally experienced unsolvable problems while pulling code using nikic/php-parser, which may very quickly cause unsolvable dependency version conflicts between your framework pulling it at version 4 and some other libraries pulling it at another version. Since that symfony/dependency-injection is supposed to be a low level library, in use by other frameworks, it may create serious dependency resolution problems. That is not taking into account that phpdocumentor/* are also in use by the property access component, which at some point might also require a divergent version.

[stof]

That is not taking into account that phpdocumentor/* are also in use by the property access component, which at some point might also require a divergent version.

For cross-component conflicts, that's not an issue. We can easily ensure that both components support a common version. External conflicts are a much bigger issue for the ecosystem (as we control only part of the conflicting packages).

[pounard]

@stof you're absolutely right. It still would add one complexity further in maintenance then it needs to coordinate property-info active maintainers with the container one in case of any change. As it exists, I could install the 4.3 version of the property-info component and any version from 2.x to actual of the container component, without going fullstack, and without creating a dependency problem, they don't share common dependencies (at least not in their actual composer.json files, except maybe for dev dependencies). But that's probably a negligible problem.

[teohhanhui]

Sorry, I see I've misunderstood the issue. If a class is defined with a missing parent, we've always got an error of this nature during autoload:

Error: Class 'Symfony\Component\BrowserKit\AbstractBrowser' not found

Which is great.

This issue only concerns the special case of throwing an exception from the autoloader, in order to be able to "recover" from the above error, which I now see is a very dubious loophole indeed. Everything I've said before is irrelevant.

[derrabus]

What can we do to move forward here? php 7.4 has reached RC stage and the behavior described above is still present. I would assume that 7.4.0 will ship with it.

[nicolas-grekas]

As a pure BC solution, yes, I think doing something like this is reasonable. ^

@nikic can we hope for a patch from your side on php-src to implement this?

Because on ours, we're out of luck. Yes, we can check the parent on classes we've control over, but that's never going to solve all items listed in https://github.com/symfony/symfony/issues/32995#issuecomment-519026061

Loosely related: as explained, if ([...]) return; doesn't work at the top of a file with opcache. We must nest the full declaration inside the if. Is this fixable? Should we open a bug report about it?

My main concern is BC of course. Thanks for your understanding and help!

[nicolas-grekas]

See https://github.com/symfony/symfony/pull/33539/files#diff-4d4aa4c7bcd6435d73fdfc6f6c010e4b for what we could do on code we have control over.

[nicolas-grekas]

Closed by php/php-src#4697, thank you @nikic!

[pounard]

That's good news, at least it shuts down this discussion and everyone is happy.

[derrabus]

Wow, thanks a lot, @nikic!

[alcaeus]

Thanks @nikic, your work on this is highly appreciated! ❤️