Web server bundle #21039

Merged
merged 10 commits into from Jan 6, 2017
@fabpot
Member
fabpot commented Dec 23, 2016 edited
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/a

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:

bin/console server:run

To manage a background server:

bin/console server:start
bin/console server:stop
bin/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:8888
bin/console server:stop # no need to pass the address again
bin/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.

+ {
+ $this
+ ->setDefinition(array(
+ new InputArgument('addressport', InputArgument::OPTIONAL, 'The address to listen to (can be address:port, address, or port)', '127.0.0.1:8000'),
@javiereguiluz
javiereguiluz Dec 24, 2016 Member

Minor: instead of addressport, we could call this uri because we accept schemes or not, IP address or hostnames, port numbers or not, etc. So this looks like a URI.

@xabbuh
xabbuh Dec 24, 2016 Member

I think I would name this socket.

@ro0NL
ro0NL Dec 24, 2016 Contributor

What about address? Like WebServer does?

+
+ $callback = null;
+ $disableOutput = false;
+ if (OutputInterface::VERBOSITY_NORMAL > $output->getVerbosity()) {
@stof
stof Dec 24, 2016 Member

this means === QUIET, right ? If yes, it may be much more readable

@lyrixx
lyrixx Dec 24, 2016 Member

->isQuiet() ;)

+ '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)) {
@stof
stof Dec 24, 2016 Member

by making true 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)

@stof
stof Jan 5, 2017 Member

@fabpot what about this ?

@fabpot
fabpot Jan 5, 2017 Member

It is like this on the current commands. So, I'm not sure.

@fabpot
fabpot Jan 5, 2017 Member

ok, I've made a new commit to change the default value to false

->setDefinition(array(
- new InputArgument('address', InputArgument::OPTIONAL, 'Address:port', '127.0.0.1:8000'),
- new InputOption('port', 'p', InputOption::VALUE_REQUIRED, 'Address port number', '8000'),
@stof
stof Dec 24, 2016 Member

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)

@ro0NL
ro0NL Dec 26, 2016 Contributor

Introducing a new composer package with a deprecation by default, is probably a bad idea? It should be done from the framework bundle.

@fabpot
fabpot Dec 30, 2016 Member

We never kept BC on CLI commands AFAIK.

+ $this->env = $env;
+ }
+
+ public function run($router = null, $disableOutput = true, $callback = null)
@stof
stof Dec 24, 2016 Member

should it be typehinted as callable ?

+ }
+ }
+
+ return false;
@stof
stof Dec 24, 2016 Member

I prefer a string|null API than string|false. The absence of value is null, not a boolean

@@ -0,0 +1,35 @@
+{
+ "name": "symfony/web-server-bundle",
@stof
stof Dec 24, 2016 Member

you forgot the replace rule for this package in the root

+ "name": "symfony/web-server-bundle",
+ "type": "web-server-bundle",
+ "description": "Symfony WebServerBundle",
+ "keywords": [],
@stof
stof Dec 24, 2016 Member

It would be good to add some keywords for discoverability through the Packagist search

@mickaelandrieu

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 :)

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Dec 26, 2016
@ro0NL
Contributor
ro0NL commented Dec 26, 2016

I would not need really the commands by default, i guess.

What about moving WebServer to symfony/process. As it already uses that, there's no real dependency issue or so. PhpServerProcess?

+ public function __construct($address = '127.0.0.1:8000')
+ {
+ if (false !== strpos($address, ':')) {
+ list($this->hostname, $this->port) = explode(':', $address);
@sstok
sstok Dec 27, 2016 edited Contributor

This will not work when IPv6 is used.

The format for IPv6 should actually be [address]:port, eg. [::1]:8000

fabpot added some commits Dec 23, 2016
@fabpot fabpot moved server:* command to a new bundle 132902c
@fabpot fabpot removed obsolete check 48dd2b0
@fabpot fabpot made the router configurable via env vars ac1ba77
@fabpot fabpot [WebServerBundle] changed wording 951a1a2
@fabpot fabpot [WebServerBundle] moved most of the logic in a new class fa7ebc5
@fabpot fabpot [WebServerBundle] tweaked command docs
585d445
@fabpot
Member
fabpot commented Jan 4, 2017

Auto-port detection has just been added. I've also updated the description to describe usage and differences with current commands.

@fabpot
Member
fabpot commented Jan 5, 2017
@nicolas-grekas
Member

👍

@mickaelandrieu
Contributor

Also, as the behavior have changed this need - at least - an issue to document it :)

@javiereguiluz
Member

I've found a minor error when using server:start repeatedly in the same project:

server-start-error

The error message says that the server is listening on 8001 instead of 8000.

+ $io = new SymfonyStyle($input, $output);
+ $server = new WebServer();
+ if ($server->isRunning($input->getOption('pidfile'))) {
+ $io->success('Web server still listening.');
@javiereguiluz
javiereguiluz Jan 5, 2017 Member

Is it possible to show the address and port where the port is listening? Otherwise this command doesn't provide much help:

server-status

+ new InputArgument('addressport', InputArgument::OPTIONAL, 'The address to listen to (can be address:port, address, or port)', null),
+ new InputOption('docroot', 'd', InputOption::VALUE_REQUIRED, 'Document root', null),
+ new InputOption('router', 'r', InputOption::VALUE_REQUIRED, 'Path to custom router script'),
+ new InputOption('pidfile', '', InputOption::VALUE_REQUIRED, 'PID file', null),
@stof
stof Jan 5, 2017 Member

second argument should be null

@fabpot
fabpot Jan 5, 2017 Member

fixed everywhere

+ new InputArgument('addressport', InputArgument::OPTIONAL, 'The address to listen to (can be address:port, address, or port)', null),
+ new InputOption('docroot', 'd', InputOption::VALUE_REQUIRED, 'Document root', null),
+ new InputOption('router', 'r', InputOption::VALUE_REQUIRED, 'Path to custom router script'),
+ new InputOption('pidfile', '', InputOption::VALUE_REQUIRED, 'PID file', null),
@stof
stof Jan 5, 2017 Member

The help of the comment should explain the PID file IMO

@@ -0,0 +1,19 @@
+Copyright (c) 2004-2016 Fabien Potencier
@stof
stof Jan 5, 2017 Member

2017 now

+ $this->port = $address;
+ } else {
+ $this->hostname = $address;
+ $this->port = 80;
@stof
stof Jan 5, 2017 Member

Shouldn't this also find the best port ?

@fabpot
fabpot Jan 5, 2017 Member

good idea, done

+ if (false !== $pos = strrpos($address, ':')) {
+ $this->hostname = substr($address, 0, $pos);
+ $this->port = substr($address, $pos + 1);
+ } elseif (ctype_digit($address)) {
@javiereguiluz
javiereguiluz Jan 5, 2017 Member

Should we check if the user is trying to use a privileged port (i.e. < 1024) ?

Otherwise, you'll end up with this error:

command-port-1

browser-port-1

+ if (null === $address) {
+ $this->address = '127.0.0.1';
+ $this->port = $this->findBestPort();
+ $address = $this->address.':'.$this->port;
@stof
stof Jan 5, 2017 Member

you should return here, or put this if in the same chain than others, to avoid parsing the address and port again

+
+ if (false !== $pos = strrpos($address, ':')) {
+ $this->hostname = substr($address, 0, $pos);
+ $this->port = substr($address, $pos + 1);
@javiereguiluz
javiereguiluz Jan 5, 2017 Member

Not sure if we should care about this ... but negative ports are wrongly allowed too:

negative-port-number

+ $this->port = $this->findBestPort();
+ } elseif (false !== $pos = strrpos($address, ':')) {
+ $this->hostname = substr($address, 0, $pos);
+ $this->port = substr($address, $pos + 1);
@javiereguiluz
javiereguiluz Jan 5, 2017 Member

Again I don't know if we should care, but here the port can also be non-numeric:

non-numeric-port

@fabpot
fabpot Jan 5, 2017 Member

I've fixed that to only allow numeric values.

@javiereguiluz
Member

👍

Status: reviewed

@fabpot
Member
fabpot commented Jan 5, 2017

@javiereguiluz server:start command fixed when the web server is already running.

fabpot added some commits Jan 4, 2017
@fabpot fabpot [WebServerBundle] added support for port auto-detection 126f4d9
@fabpot fabpot [WebServerBundle] fixed server:start when already running
961d1ce
@fabpot
Member
fabpot commented Jan 5, 2017

All comments taken into account now.

@fabpot fabpot [WebServerBundle] switched auto-run of server:start to off by default
f39b327
@fabpot fabpot merged commit f39b327 into symfony:master Jan 6, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Jan 6, 2017
@fabpot fabpot feature #21039 Web server bundle (fabpot)
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/a

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:

```
bin/console server:run
```

To manage a background server:

```
bin/console server:start
bin/console server:stop
bin/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:8888
bin/console server:stop # no need to pass the address again
bin/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 default
961d1ce [WebServerBundle] fixed server:start when already running
126f4d9 [WebServerBundle] added support for port auto-detection
6f689d6 [WebServerBundle] changed the way we keep track of the web server
585d445 [WebServerBundle] tweaked command docs
fa7ebc5 [WebServerBundle] moved most of the logic in a new class
951a1a2 [WebServerBundle] changed wording
ac1ba77 made the router configurable via env vars
48dd2b0 removed obsolete check
132902c moved server:* command to a new bundle
be85fcc
@javiereguiluz javiereguiluz referenced this pull request in symfony/symfony-docs Jan 6, 2017
Open

Document WebServerBundle #7325

@fabpot fabpot deleted the fabpot:web-server-bundle branch Jan 7, 2017
@nicolas-grekas nicolas-grekas added a commit that referenced this pull request Jan 12, 2017
@nicolas-grekas nicolas-grekas bug #21104 [FrameworkBundle] fix IPv6 address handling in server comm…
…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        |

This fixes #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
d7bc68a
@sensiolabs-splitter sensiolabs-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jan 12, 2017
@nicolas-grekas nicolas-grekas bug #21104 [FrameworkBundle] fix IPv6 address handling in server comm…
…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        |

This fixes symfony/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
62bbde7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment