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

[Console] Added suggestions for missing packages #29865

Merged
merged 1 commit into from Feb 21, 2019

Conversation

@przemyslaw-bogusz
Copy link
Contributor

commented Jan 12, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Currently, when someone runs one of the most common commands, e.g. server:run, but does not have a required package installed, they will get a general 'There are no commands defined...' message.

This commit adds a more useful message, informing the user about a package that might be missing and suggesting a command that should be run in order to install it, e.g. composer require symfony/web-server-bundle --dev.

@chalasr

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

I like the idea.
I think this belongs to the framework, not the Console component.
Components should not be aware of bundles, it's the other way around.
FrameworkBundle looks like a perfect fit for this, as it represents the glue for all Symfony components and bundles, and it has its own console Application.

@chalasr chalasr added this to the next milestone Jan 13, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

i'd prefer something like we did for twig: https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/UndefinedCallableHandler.php

i think its more straightforward to handle it in e.g. a catch-all event listener for unknown commands, opposed to tracking a list on high level (console application).

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

I had some doubts adding it to the main application, since it is widely used as a standalone component, but I wanted the suggestions to be available for some, who started his project with a symfony/skeleton (so without the FrameworkBundle) and tries to use features, that are not installed.

By the way, can you tell me, why my CI checks failed? Did I forget about something in my PR?

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

My mistake. I confused FrameworkBundle with SensioFrameworkExtraBundle. I will rework the PR.

But my other question about the CI checks is still valid :)

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from 268e80f to 4f0d498 Jan 13, 2019

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

I went with the suggestion from @ro0NL and created an event subscriber. I can always switch to a try/catch in FrameworkBundle Console Application.

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch 2 times, most recently from 018f91a to 7c80043 Jan 14, 2019

@chalasr chalasr added the DX label Jan 15, 2019

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch 2 times, most recently from 56f0342 to 3bf0806 Jan 17, 2019

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

I improved the logic. For example, when someone has WebServerBundle and runs server:dump, the message will suggest installing VarDumper (along with alternatives from the original CommandNotFoundException). But on server:foo will add nothing - just show the alternatives.

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch 2 times, most recently from 1d72a56 to da6f7ed Jan 17, 2019

@chalasr chalasr self-requested a review Jan 20, 2019

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from 0217579 to 8149049 Feb 13, 2019

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@fabpot Done.

@nicolas-grekas
Copy link
Member

left a comment

(with minor comment)

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from 8149049 to d178ad5 Feb 13, 2019

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from d178ad5 to 7c6a2d1 Feb 14, 2019

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from 7c6a2d1 to c6b803e Feb 14, 2019

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch 2 times, most recently from f3187ec to ba3f404 Feb 14, 2019

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Too many reviews in a short period of time and things got a little messy for a moment. I've implemented all suggestions the code, fixed the tests. The one Travis CI test that fails, failed before and I guess it has nothing to do with my code directly.

The only thing left to decide is whether to put the back in the exception message by default, or add it to selected messages. I'm in favor of the by default.

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the przemyslaw-bogusz:console_packages branch from ba3f404 to f1cf2f5 Feb 14, 2019

@fabpot

fabpot approved these changes Feb 21, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Thank you @przemyslaw-bogusz.

@fabpot fabpot force-pushed the przemyslaw-bogusz:console_packages branch from f1cf2f5 to 423a54f Feb 21, 2019

@fabpot fabpot merged commit 423a54f into symfony:master Feb 21, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 21, 2019

feature #29865 [Console] Added suggestions for missing packages (prze…
…myslaw-bogusz)

This PR was squashed before being merged into the 4.3-dev branch (closes #29865).

Discussion
----------

[Console] Added suggestions for missing packages

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Currently, when someone runs one of the most common commands, e.g. `server:run`, but does not have a required package installed, they will get a general **'There are no commands defined...'** message.

This commit adds a more useful message, informing the user about a package that might be missing and suggesting a command that should be run in order to install it, e.g. `composer require symfony/web-server-bundle --dev`.

Commits
-------

423a54f [Console] Added suggestions for missing packages

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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