Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Additional helper commands to control PHP's built-in web server#11311
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Without this change I wasn't able to prefix thePHP_BINARY call withexec. Do you have any idea how to handle it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@romainneutron To be more precise: The command that should be executed leading to a "Command not found error" actually is something like:
'exec''/usr/bin/php' ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Either we could implement a new feature in the process builder, either you can build the command manually instead of using the ProcessBuilder, but you can not decide to unescape the first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Of course, you're right. I first thought that it doesn't make sense to escape the command name. But that's obviously not always true. What do you think about adding a method likeuseExec to theProcessBuilder class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That would be indeed a nice feature. Please open a new PR for this feature as it would be easier to review this new feature in a separate thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I created a pull request for this (see#11335).
xabbuh commentedJul 6, 2014
Both the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The server process could have crashed. You could check if the server sends HTTP responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea. That could be done relatively easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The lock file is now removed if the server process terminated before.
xabbuh commentedJul 8, 2014
I modified this a bit. The pull request is no longer based off#11335. Instead the script that is executed by the |
xabbuh commentedAug 15, 2014
Any thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This var is unused in the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed it.
fabpot commentedAug 28, 2014
I've not tried it but it looks good to me. |
fabpot commentedSep 2, 2014
@romainneutron should give you some feedback soon... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
3000 seconds is very long timeout, I rather suggest a short one, 1 second should be enough.
Moreover, this method should return a boolean and close the socket resource once it has been created
romainneutron commentedSep 3, 2014
It would be nice to add correct exitcodes (0 on success, other codes in case of failures) |
4fbf128 to459a97eComparexabbuh commentedSep 3, 2014
@romainneutron I added 1 where necessary. 0 is the default behaviour. Do you think it makes sense to return 0 explicitly? |
romainneutron commentedSep 3, 2014
No need to return 0 in case the command was successful |
lyrixx commentedSep 3, 2014
The lock management could/should use#10475 (but it's not yet merged) |
xabbuh commentedSep 3, 2014
You can then have another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If it failed, this call would fail. You should use instead:
if (false !==$fp = @fsockopen($hostname,$port,$errno,$errstr,1)) {fclose($fp);returntrue;}returnfalse;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yes, that's true.
bde1b5e toe796bacCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's add a check: If the server could not be started, let's display an error
if (!$process->isRunning()) {$output->writeln('<error>Unable to start the server process</error>');unlink($lockfile);return1;}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this would trigger an error in case the address is not in the right format (for examplerandomstring would trigger anUndefined index notice
To avoid that, you can use:
explode(':',$address) +array(8000);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
or better, ensure the passed argument matches a regular expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not really, there wouldn't be any server running with this address. So, there is no lock file andisServerRunning() would never be called (see line 48).
xabbuh commentedSep 3, 2014
I addressed your other suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$pid argument does not seem to be used withsprintf
28afe29 to74c993cCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should be good to add a call toposix_setsid() here to avoid zombie process.
if (posix_setsid() <0) {return1; }
xabbuh commentedSep 9, 2014
Thanks for the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
might be nice to display a nice message in case it happens likeUnable to set the child process as session leader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
romainneutron commentedSep 9, 2014
I'm 👍 on this one (once my last comment would be addressed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
justif as the previousif returns
8464681 tobd58bcbComparexabbuh commentedSep 21, 2014
Any other opinion on this? |
fabpot commentedSep 21, 2014
👍 |
fabpot commentedSep 22, 2014
Thank you@xabbuh. |
…l PHP's built-in web server (xabbuh)This PR was merged into the 2.6-dev branch.Discussion----------[FrameworkBundle] Additional helper commands to control PHP's built-in web server| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#10827| License | MIT| Doc PR |symfony/symfony-docs#4005Basically, both the ``server:status`` and ``server:stop`` wouldn't be really reliable if you had stopped the web server by, for example, killing the process. But honestly I don't know how to platform-independently determine if a process is still running given its PID. Maybe such a way could be a good improvement for the Process component.Commits-------b601454 new helper commands for PHP's built-in server
…mands (xabbuh)This PR was merged into the 2.6-dev branch.Discussion----------[FrameworkBundle] add changelog entry for web server commands| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |I just noticed that I forgot to add a changelog entry in#11311.Commits-------292088f add changelog entry for web server commands
…ilt-in web server in the background (xabbuh)This PR was merged into the master branch.Discussion----------[Cookbook][Web server] description for running PHP's built-in web server in the background| Q | A| ------------- | ---| Doc fix? | no| New docs? | yes (symfony/symfony#11311)| Applies to | 2.6+| Fixed tickets |Commits-------3caec21 description for running PHP's built-in web server in the background
Basically, both the
server:statusandserver:stopwouldn't be really reliable if you had stopped the web server by, for example, killing the process. But honestly I don't know how to platform-independently determine if a process is still running given its PID. Maybe such a way could be a good improvement for the Process component.