Skip to content

Conversation

@lorisleiva
Copy link
Contributor

Hi there 👋

Context:
Last year, support for Lazy loading commands was added in Laravel 9 (See #34873).

This means, if the command we're trying to resolve is a valid class, we add it to a commandMap using the getDefaultName static method as a key. Otherwise, we fall back to the previous behaviour which resolves the command from the container.

public function resolve($command)
{
    if (class_exists($command) && ($commandName = $command::getDefaultName())) {
        $this->commandMap[$commandName] = $command;

        return null;
    }

    return $this->add($this->laravel->make($command));
}

Problem:
The issue here is that the above code assumes $command will always refer to the class name of a Symfony Command — i.e. Symfony\Component\Console\Command\Command.

However, the previous behaviour allowed us to use $command as an abstract that would, later on, be resolved as a valid Symfony or Laravel Command class.

The new behaviour prevents this by throwing an exception saying the getDefaultName static method does not exist on the abstract class provided.

Concrete example:
The Laravel Actions package leverages the previous behaviour to allow any PHP class to be registered as a command. It is only after the provided abstract is resolved from the container that we get a Laravel Command that decorates the provided PHP class.

Solution:
Fortunately, continuing to support the previous behaviour is very simple. We just need a stronger assertion before caching the provided command.

Instead of checking that the class exists — using class_exists — we can check that the provided class is a subclass of the Symfony Command — using is_subclass_of which also checks that the class exists.

@taylorotwell taylorotwell changed the base branch from master to 9.x January 25, 2022 17:28
@taylorotwell
Copy link
Member

Hey @lorisleiva! Can you resubmit this on the 9.x branch? I tried to retarget it but it caused issues.

@lorisleiva
Copy link
Contributor Author

🤦‍♂️ Sorry about that I forgot master was 10.x now. 😅 I'll re-submit right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants