Skip to content

[WebServerBundle] added a way to dump current status host/port/address when getting the status #22316

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Apr 6, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Now that the web server can run on an automatically chosen port, it's more complex to integrate it into tests (I want to run some functional tests locally).

The server:status command gives this information, but not in a machine-readable way.

I propose to add a --filter flag to fix that:

./bin/console server:status --filter=address

@fabpot
Copy link
Member Author

fabpot commented Apr 6, 2017

Here is how I'm using this in my Makefile:

bf-dev:
	blackfire-player --endpoint=http://`bin/console server:status --filter=address` run tests/bkf/all.bkf
.PHONY: bf-dev

@fabpot fabpot force-pushed the server-status-options branch from cf724dd to b417b62 Compare April 6, 2017 16:47
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

Clever!

@@ -35,6 +35,7 @@ protected function configure()
->setName('server:status')
->setDefinition(array(
new InputOption('pidfile', null, InputOption::VALUE_REQUIRED, 'PID file'),
new InputOption('filter', null, InputOption::VALUE_REQUIRED, 'The value to display (one of port, host, or address)'),
Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like --value | --get | --get-value?

if ($server->isRunning($input->getOption('pidfile'))) {
list($host, $port) = explode(':', $address = $server->getAddress($input->getOption('pidfile')));
if ('address' === $filter) {
$output->write($address);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use OUTPUT_RAW to avoid formatting possible malicious input?

Copy link
Member Author

Choose a reason for hiding this comment

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

input comes from the pidfile. If you mess up with it, then you are on your own anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note it can also be messed up by some formatter implementation. But fair enough.. what are the odds anyway :)

@fabpot fabpot merged commit b417b62 into symfony:master Apr 6, 2017
fabpot added a commit that referenced this pull request Apr 6, 2017
…ost/port/address when getting the status (fabpot)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[WebServerBundle] added a way to dump current status host/port/address when getting the status

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Now that the web server can run on an automatically chosen port, it's more complex to integrate it into tests (I want to run some functional tests locally).

The `server:status` command gives this information, but not in a machine-readable way.

I propose to add a `--filter` flag to fix that:

`./bin/console server:status --filter=address`

Commits
-------

b417b62 [WebServerBundle] added a way to dump current status host/port/address when getting the status
@fabpot fabpot deleted the server-status-options branch April 7, 2017 17:17
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants