Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nicolas-grekas opened this issue Aug 6, 2019 · 35 comments

Comments

@nicolas-grekas
Copy link
Member

commented Aug 6, 2019

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

@vudaltsov

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@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 comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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.

@nicolas-grekas nicolas-grekas referenced this issue Aug 7, 2019
22 of 23 tasks complete
@nikic

This comment has been minimized.

Copy link

commented Aug 7, 2019

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 comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

You can take #32396 as an concrete example.

The class \Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser implements \Symfony\Component\Form\FormTypeGuesserInterface

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 \Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser 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 \Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser would have been expected behaviour.

@pounard

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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 \Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser 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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

See also #32395 (comment)

And the reply to that comment: #32395 (comment)

@pounard

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

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

@deleugpn

This comment has been minimized.

Copy link

commented Aug 22, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

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

image

@terjebraten-certua

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

I think @deleugpn has a point. He pointed out that the example @nikic came with in #32395 (comment) 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

This comment has been minimized.

Copy link

commented Aug 22, 2019

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

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.