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

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

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.

@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.

Wow, thanks a lot, @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.

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?

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.

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.

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 😃

@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.

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.

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

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…

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!

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.

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.

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.

@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).

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

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.

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.

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.

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?

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.

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.

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.

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

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

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.

@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

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.

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++, …

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.)

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.

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.

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.

@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.

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();
}

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));