Skip to content

[Console] Added suggestions for missing packages #29865

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2019
Merged

[Console] Added suggestions for missing packages #29865

merged 1 commit into from
Feb 21, 2019

Conversation

przemyslaw-bogusz
Copy link
Contributor

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
Copy link
Member

chalasr 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
Copy link
Contributor

ro0NL 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 console_packages branch 2 times, most recently from 018f91a to 7c80043 Compare January 14, 2019 23:49
chalasr
chalasr requested changes Jan 15, 2019
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

We will need some tests

@chalasr chalasr added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Jan 15, 2019
@przemyslaw-bogusz przemyslaw-bogusz force-pushed the console_packages branch 2 times, most recently from 56f0342 to 3bf0806 Compare January 17, 2019 09:01
@przemyslaw-bogusz
Copy link
Contributor Author

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 console_packages branch 2 times, most recently from 1d72a56 to da6f7ed Compare January 17, 2019 11:29
@przemyslaw-bogusz przemyslaw-bogusz mentioned this pull request Jan 18, 2019
Closed
@chalasr chalasr self-requested a review January 20, 2019 17:52
@przemyslaw-bogusz
Copy link
Contributor Author

@fabpot Done.

nicolas-grekas
nicolas-grekas approved these changes Feb 13, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with minor comment)

@przemyslaw-bogusz przemyslaw-bogusz force-pushed the console_packages branch 2 times, most recently from f3187ec to ba3f404 Compare February 14, 2019 13:55
@przemyslaw-bogusz
Copy link
Contributor Author

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.

@fabpot
Copy link
Member

fabpot commented Feb 21, 2019

Thank you @przemyslaw-bogusz.

@fabpot fabpot merged commit 423a54f into symfony:master Feb 21, 2019
fabpot added a commit that referenced this pull request Feb 21, 2019
@fabpot
feature #29865 [Console] Added suggestions for missing packages (prze…
3560cfd 
…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 mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@mbabker mbabker mbabker left review comments

@ro0NL ro0NL ro0NL left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
7 participants
@przemyslaw-bogusz @chalasr @ro0NL @fabpot @nicolas-grekas @mbabker @carsonbot