Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

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

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[FrameworkBundle] [Console][DX] add a warning when command is not found #26290

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

Conversation

Simperfit
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR

This PR add DX on the the console find() and get() methods when a command is not found because it has not been registered properly.

@carsonbot carsonbot added Status: Needs Review FrameworkBundle Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Feb 23, 2018
@Simperfit Simperfit force-pushed the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch from 52a9c9c to dcfa734 Compare February 23, 2018 18:23
@chalasr chalasr added this to the 3.4 milestone Feb 23, 2018
@chalasr chalasr added the Bug label Feb 23, 2018
@chalasr
Copy link
Member

chalasr commented Feb 23, 2018

Qualifies as a bug since an exception was thrown before 3.4, breaking the whole console with a clear stack trace, currently you have no information at all.

throw new CommandNotRegisteredException(implode('
', $errors), 0, $e);
}
throw $e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should just call renderRegistrationErrors() before callingfind().

@Simperfit
Copy link
Contributor Author

Simperfit commented Feb 23, 2018 via email

@chr-hertel
Copy link
Contributor

if i get this right, wrapping the parent::doRun() call should do the trick:

    public function doRun(...)
    {
        // ...

        try {
            return parent::doRun($input, $output);
        } catch (CommandNotFoundException $e) {
            if ($this->registrationErrors) {
                $this->renderRegistrationErrors($input, $output);
            }

            throw $e;
        }
    }

@Simperfit
Copy link
Contributor Author

Thanks @chr-hertel ! it's clearly better than what I thought.

@Simperfit Simperfit force-pushed the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch 2 times, most recently from e24222f to 0f93cbd Compare February 24, 2018 15:42
return parent::find($name);
try {
return parent::find($name);
} catch (CommandNotFoundException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed anymore when catching in doRun(...), is it?

@Simperfit Simperfit force-pushed the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch from 0f93cbd to ecefeec Compare February 24, 2018 19:23
}

throw $e;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead we can just call $this->registerCommands() before the if above

$container = new ContainerBuilder();
$container->register('event_dispatcher', EventDispatcher::class);
$container->register(ThrowingCommand::class, ThrowingCommand::class);
$container->setParameter('console.command.ids', array(ThrowingCommand::class => ThrowingCommand::class));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the throwing command is not necessary for this test

@@ -165,6 +165,37 @@ public function testRunOnlyWarnsOnUnregistrableCommand()
$this->assertContains('fine', $output);
}

public function testRunOnlyWarnsOnNoName()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testRegistrationErrorsAreDisplayedOnCommandNotFound

$this->assertSame(1, $tester->getStatusCode());
$this->assertContains('Some commands could not be registered:', $output);
$this->assertContains('throwing', $output);
$this->assertContains('fine', $output);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command "fine" is not defined.


$tester = new ApplicationTester($application);
$tester->run(array('command' => 'fine'));
$tester->getOutput();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to revert

$message = sprintf("[WARNING] Some commands could not be registered: \n %s", $message);
parent::__construct($message, $previous instanceof CommandNotFoundException ? $previous->getAlternatives() : array(), $code, $previous);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed anymore

@Simperfit Simperfit force-pushed the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch 2 times, most recently from c26aa09 to 84940c5 Compare February 25, 2018 08:25
@Simperfit Simperfit force-pushed the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch from 84940c5 to efd8f7f Compare February 25, 2018 08:26
@Simperfit
Copy link
Contributor Author

Status: Needs Review

@fabpot
Copy link
Member

fabpot commented Feb 27, 2018

Thank you @Simperfit.

@fabpot fabpot merged commit efd8f7f into symfony:3.4 Feb 27, 2018
fabpot added a commit that referenced this pull request Feb 27, 2018
… is not found (Simperfit)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] [Console][DX] add a warning when command is not found

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none
| License       | MIT
| Doc PR        |

This PR add DX on the the console `find()` and `get()` methods when a command is not found because it has not been registered properly.

Commits
-------

efd8f7f [FrameworkBundle] [Console] add a warning when command is not found
This was referenced Mar 1, 2018
@Simperfit Simperfit deleted the dx/add-warning-when-command-is-not-found-and-registration-errors-exists branch March 2, 2018 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) FrameworkBundle Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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