8000 [DX] Show a message if server:start is missing pcntl · Issue #12153 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX] Show a message if server:start is missing pcntl #12153

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
Closed
weaverryan opened this issue Oct 6, 2014 · 10 comments
Closed

[DX] Show a message if server:start is missing pcntl #12153

weaverryan opened this issue Oct 6, 2014 · 10 comments
Labels
Console FrameworkBundle Good first issue Ideal for your first contribution! (some Symfony experience may be required)

Comments

@weaverryan
Copy link
Member

Hi guys!

We now have the server:start and server:stop commands, but they require the pcntl extension. If this extension is not available, the isEnabled() function return false, so the commands are completely not available.

I think this may cause "wtf" moments for users - especially if they're following docs that say to use server:start. Instead, I'd propose:

  1. Removing the pctnl check from isEnabled
  2. Checking for pcntl inside execute() and returning a very clear message if it is missing:

This command needs the pcntl extension to run. You can either install this, or use the server:run command instead to run the built-in web server.

Or alternatively we could actually say:

This command needs the pctnl extension to run. Since this extension is not installed, we'll instead start the built-in web server using the server:run command

... and then we would actually call the server:run command for them.

Also, why not print a message at the bottom of server:run that says:

Quick the server with CONTROL+C.

I've "borrowed" the language even from Django (https://docs.djangoproject.com/en/1.7/intro/tutorial01/#the-development-server), so there is some precedence for that :).

ping @xabbuh
@stof stof added Console FrameworkBundle Good first issue Ideal for your first contribution! (some Symfony experience may be required) labels Oct 6, 2014
@xabbuh
Copy link
Member
xabbuh commented Oct 6, 2014

@weaverryan I like your proposal 👍

@stof
Copy link
Member
stof commented Oct 7, 2014

This adds a question about the usefulness of the isEnabled() feature for commands. Is it better to hide commands which cannot be used, or to display them and to fail when trying to use them ? We are already hiding server:run on 5.3 for instance.

and then we would actually call the server:run command for them.

I'm not sure about this, as it would mean that we would block the terminal now, while the user asked to run a command which was not blocking it (thus, running a command from another one is kind of painful).

@hhamon
Copy link
Contributor
hhamon commented Oct 7, 2014

Shall we introduce a new protected method to define an explanation message of why the command cannot be run if you try to run it while isEnabled() returns false? I'm not sure if it's possible either to run of command that is not enabled...

@stof
Copy link
Member
stof commented Oct 8, 2014

@hhamon no you cannot. disabled commands are excluded from the command search in the application. In practice, it looks the same than not registering the command at all (except that the logic can be dynamic)

@xabbuh
Copy link
Member
xabbuh commented Oct 9, 2014

I think it's okay to make an exception for the server commands to be shown if the PCNTL extension. This really improves the developer's experience when they are at least able to execute commands that have an important part in the documentation (not yet but we are working on this).

However, I agree that we shouldn't start the server:run command automatically. The user probably triggered the server:start command because they didn't want to block the current terminal. Executing server:run then would be really unexpected. As long as we point them to the server:run command in the error message that will be enough from my point of view.

@weaverryan weaverryan changed the title Show a message if server:start is missing pcntl [DX] Show a message if server:start is missing pcntl Oct 17, 2014
@weaverryan
Copy link
Member Author

So I think we have some agreement:

  1. Always enable the command, but throw a nice exception message if they're missing the PCNTL extension.

  2. Include directions on that exception message that suggest using server:run instead.

  3. Do not automatically try to server:run.

We'll definitely want this for 2.6 :).

@xabbuh
Copy link
Member
xabbuh commented Oct 17, 2014

@weaverryan I think you meant server:run in 2) and 3), didn't you?

@weaverryan
Copy link
Member Author

@xabbuh Yes thanks - I fixed my comment!

@fabpot
Copy link
Member
fabpot commented Oct 18, 2014

👍

@xabbuh xabbuh mentioned this issue Oct 18, 2014
Merged
@xabbuh
Copy link
Member
xabbuh commented Oct 18, 2014

see #12253

fabpot added a commit that referenced this issue Oct 19, 2014
@fabpot
bug #12253 [FrameworkBundle] improve server commands feedback (xabbuh)
4fe8eda 
This PR was merged into the 2.6-dev branch.

Discussion
----------

[FrameworkBundle] improve server commands feedback

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | kind of
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12153
| License       | MIT
| Doc PR        |

* display a message when `server:start` is executed and the PCNTL
  extension is not loaded
* print instructions about how to terminate the `server:run` command

Commits
-------

bf174cf [FrameworkBundle] improve server commands feedback
@fabpot fabpot closed this as completed Oct 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Console FrameworkBundle Good first issue Ideal for your first contribution! (some Symfony experience may be required)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @weaverryan @hhamon @stof @xabbuh
0