Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Web server bundle#21039
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
Web server bundle#21039
Uh oh!
There was an error while loading.Please reload this page.
Conversation
21475e2 to1a796c0Compare| { | ||
| $this | ||
| ->setDefinition(array( | ||
| newInputArgument('addressport', InputArgument::OPTIONAL,'The address to listen to (can be address:port, address, or port)','127.0.0.1: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.
Minor: instead ofaddressport, we could call thisuri because we accept schemes or not, IP address or hostnames, port numbers or not, etc. So this looks like a URI.
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 think I would name thissocket.
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.
What aboutaddress? LikeWebServer does?
| $callback =null; | ||
| $disableOutput =false; | ||
| if (OutputInterface::VERBOSITY_NORMAL >$output->getVerbosity()) { |
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 means=== QUIET, right ? If yes, it may be much more readable
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.
->isQuiet() ;)
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.
fixed
| 'You can either install it or use the "server:run" command instead.', | ||
| )); | ||
| if ($io->ask('Do you want to execute <info>server:run</info> immediately? [Yn]',true)) { |
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.
by makingtrue the default, it means that a non-interactive process will automatically fallback to server:run. Is it expected ? I don't think it is a good idea, as it switches to a long-lived command. I think this should be done only for interactive inputs (returning the failure exit code otherwise)
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.
@fabpot what about 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.
It is like this on the current commands. So, I'm not sure.
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.
ok, I've made a new commit to change the default value tofalse
| ->setDefinition(array( | ||
| newInputArgument('address', InputArgument::OPTIONAL,'Address:port','127.0.0.1:8000'), | ||
| newInputOption('port','p', InputOption::VALUE_REQUIRED,'Address port number','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.
removing this option is a BC break. We should deprecate it instead (even though the class is moved elsewhere, it is the same API for people using the CLI)
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.
Introducing a new composer package with a deprecation by default, is probably a bad idea? It should be done from the framework bundle.
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.
We never kept BC on CLI commands AFAIK.
| } | ||
| } | ||
| 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.
I prefer astring|null API thanstring|false. The absence of value is null, not a boolean
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
| $this->env =$env; | ||
| } | ||
| publicfunctionrun($router =null,$disableOutput =true,$callback =null) |
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.
should it be typehinted ascallable ?
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
| @@ -0,0 +1,35 @@ | |||
| { | |||
| "name":"symfony/web-server-bundle", | |||
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.
you forgot thereplace rule for this package in the root
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
| "name":"symfony/web-server-bundle", | ||
| "type":"web-server-bundle", | ||
| "description":"Symfony WebServerBundle", | ||
| "keywords": [], |
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.
It would be good to add some keywords for discoverability through the Packagist search
mickaelandrieu left a comment
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.
Great move, with hope it's a start for a more readable console in full edition.
2 questions : will this bundle be installed by default on Symfony 4?
How about a Symfony/web-server package and a bundle? A lot of frameworks and cmses can be interested to reuse this tiny but useful piece of code.
Merry Christmas Fabien :)
ro0NL commentedDec 26, 2016
I would not need really the commands by default, i guess. What about moving |
| publicfunction__construct($address ='127.0.0.1:8000') | ||
| { | ||
| if (false !==strpos($address,':')) { | ||
| list($this->hostname,$this->port) =explode(':',$address); |
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 will not work when IPv6 is used.
The format for IPv6 should actually be[address]:port, eg.[::1]: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.
fixed
66b1ee3 to2a31515Comparefabpot commentedJan 4, 2017
Auto-port detection has just been added. I've also updated the description to describe usage and differences with current commands. |
fabpot commentedJan 5, 2017
ping @symfony/deciders |
nicolas-grekas commentedJan 5, 2017
👍 |
mickaelandrieu commentedJan 5, 2017
Also, as the behavior have changed this need - at least - an issue to document it :) |
| $io =newSymfonyStyle($input,$output); | ||
| $server =newWebServer(); | ||
| if ($server->isRunning($input->getOption('pidfile'))) { | ||
| $io->success('Web server still listening.'); |
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.
Is it possible to show the address and port where the port is listening? Otherwise this command doesn't provide much help:
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
| newInputArgument('addressport', InputArgument::OPTIONAL,'The address to listen to (can be address:port, address, or port)',null), | ||
| newInputOption('docroot','d', InputOption::VALUE_REQUIRED,'Document root',null), | ||
| newInputOption('router','r', InputOption::VALUE_REQUIRED,'Path to custom router script'), | ||
| newInputOption('pidfile','', InputOption::VALUE_REQUIRED,'PID file',null), |
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.
second argument should benull
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.
fixed everywhere
| newInputArgument('addressport', InputArgument::OPTIONAL,'The address to listen to (can be address:port, address, or port)',null), | ||
| newInputOption('docroot','d', InputOption::VALUE_REQUIRED,'Document root',null), | ||
| newInputOption('router','r', InputOption::VALUE_REQUIRED,'Path to custom router script'), | ||
| newInputOption('pidfile','', InputOption::VALUE_REQUIRED,'PID file',null), |
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 help of the comment should explain the PID file IMO
| @@ -0,0 +1,19 @@ | |||
| Copyright (c) 2004-2016 Fabien Potencier | |||
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.
2017 now
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.
fixed
| $this->port =$address; | ||
| }else { | ||
| $this->hostname =$address; | ||
| $this->port =80; |
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.
Shouldn't this also find the best port ?
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, done
| if (false !==$pos =strrpos($address,':')) { | ||
| $this->hostname =substr($address,0,$pos); | ||
| $this->port =substr($address,$pos +1); | ||
| }elseif (ctype_digit($address)) { |
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.
Should we check if the user is trying to use a privileged port (i.e. < 1024) ?
Otherwise, you'll end up with this error:
| if (null ===$address) { | ||
| $this->address ='127.0.0.1'; | ||
| $this->port =$this->findBestPort(); | ||
| $address =$this->address.':'.$this->port; |
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.
you should return here, or put thisif in the same chain than others, to avoid parsing the address and port again
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.
fixed
| if (false !==$pos =strrpos($address,':')) { | ||
| $this->hostname =substr($address,0,$pos); | ||
| $this->port =substr($address,$pos +1); |
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 sure if we should care about this ... but negative ports are wrongly allowed too:
| $this->port =$this->findBestPort(); | ||
| }elseif (false !==$pos =strrpos($address,':')) { | ||
| $this->hostname =substr($address,0,$pos); | ||
| $this->port =substr($address,$pos +1); |
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.
Again I don't know if we should care, but here the port can also be non-numeric:
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've fixed that to only allow numeric values.
javiereguiluz commentedJan 5, 2017
👍 Status: reviewed |
fabpot commentedJan 5, 2017
@javiereguiluz |
fabpot commentedJan 5, 2017
All comments taken into account now. |
This PR was squashed before being merged into the 3.3-dev branch (closes#21039).Discussion----------Web server bundle| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21040| License | MIT| Doc PR | n/aMoved the `server:*` commands to a new bundle. It makes them more easily discoverable and more decoupled. Discoverability is important when not using symfony/symfony. In that case, the commands are not available unless you have the symfony/process component installed. With a dedicated bundle, installing the bundle also installs the dependency, making the whole process easier.Usage is the same as the current commands for basic usage:To start a web server in the foreground:```bin/console server:run```To manage a background server:```bin/console server:startbin/console server:stopbin/console server:status```The big difference is that port is auto-determined if something is already listening on port 8000.Usage is **different** if you pass options:```bin/console server:start 127.0.0.1:8888bin/console server:stop # no need to pass the address againbin/console server:status # no need to pass the address again```That's possible as the web server now stores its address in a pid file stored in the current directory.Commits-------f39b327 [WebServerBundle] switched auto-run of server:start to off by default961d1ce [WebServerBundle] fixed server:start when already running126f4d9 [WebServerBundle] added support for port auto-detection6f689d6 [WebServerBundle] changed the way we keep track of the web server585d445 [WebServerBundle] tweaked command docsfa7ebc5 [WebServerBundle] moved most of the logic in a new class951a1a2 [WebServerBundle] changed wordingac1ba77 made the router configurable via env vars48dd2b0 removed obsolete check132902c moved server:* command to a new bundle
…ands (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] fix IPv6 address handling in server commands| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21039 (comment)| License | MIT| Doc PR |Thisfixes#21039 (comment) as reported by@sstok for the existing commands by backporting@fabpot's patch from#21039.Commits-------2bb4713 fix IPv6 address handling in server commands
…ands (xabbuh)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] fix IPv6 address handling in server commands| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#21039 (comment)| License | MIT| Doc PR |Thisfixessymfony/symfony#21039 (comment) as reported by@sstok for the existing commands by backporting@fabpot's patch from #21039.Commits-------2bb4713 fix IPv6 address handling in server commands
ben29 commentedMay 31, 2017
what about add support for https ? |
stof commentedMay 31, 2017
I don't think the PHP built-in webserver supports HTTPS |
Uh oh!
There was an error while loading.Please reload this page.
Moved the
server:*commands to a new bundle. It makes them more easily discoverable and more decoupled. Discoverability is important when not using symfony/symfony. In that case, the commands are not available unless you have the symfony/process component installed. With a dedicated bundle, installing the bundle also installs the dependency, making the whole process easier.Usage is the same as the current commands for basic usage:
To start a web server in the foreground:
To manage a background server:
The big difference is that port is auto-determined if something is already listening on port 8000.
Usage isdifferent if you pass options:
That's possible as the web server now stores its address in a pid file stored in the current directory.