Skip to content
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

Fix command-line arguments & allow using env vars and wildcard IP addresses for server #18

Closed
wants to merge 5 commits into from
Closed

Conversation

Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Apr 15, 2018

Using PHP 7.2.2 on Windows, testing this code in the WebServerManager:

$commandLine = [$binary] + $finder->findArguments() + ['-dvariables_order=EGPCS', '-S', \sprintf('%s:%d', $this->hostname, $this->port)];

$commandLine2 = array_merge([$binary], $finder->findArguments(), ['-dvariables_order=EGPCS', '-S', \sprintf('%s:%d', $this->hostname, $this->port)]);

dump($commandLine);
dump($commandLine2);

And I have this output:

array:3 [
  0 => "E:\dev\php72-nts\php.exe"
  1 => "-S"
  2 => "0.0.0.0:12000"
]
array:4 [
  0 => "E:\dev\php72-nts\php.exe"
  1 => "-dvariables_order=EGPCS"
  2 => "-S"
  3 => "0.0.0.0:12000"
]

The first one is wrong, so I decided to refactor it with a plain array_merge() that will surely be working 👍

This PR also introduces lots of other changes that allow using env vars and ips like 0.0.0.0 for servers listening to any host. I thought it would be a really nice improvement

@Pierstoval Pierstoval changed the title Fixed command-line that was not retrieving all arguments properly Fix command-line arguments & allow using env vars and wildcard IP addresses for server Apr 15, 2018
@dunglas
Copy link
Member

dunglas commented Apr 22, 2018

Can you provide the bug fix in a separate PR please?

{
$context = \stream_context_create(['http' => [
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? It will not work if the / route isn't defined for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it instead uses a socket, I thought it might be a much better idea

Copy link
Member

@dunglas dunglas Jun 11, 2018

Choose a reason for hiding this comment

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

I'm 👎 to use a socket. Using a HTTP client ensure that this a HTTP server. (A server supporting another protocol may already use this port).

while (Process::STATUS_STARTED !== ($status = $process->getStatus()) || false === @\file_get_contents($url, false, $context)) {
$host = parse_url($url, PHP_URL_HOST);

if ($host === '0.0.0.0') {
Copy link
Member

Choose a reason for hiding this comment

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

Please use yoda style.

// block until the web server is ready
\usleep(1000);
} while (Process::STATUS_STARTED !== $status || ++$retries === $maxRetries);

if (count($socketErrors)) {
Copy link
Member

Choose a reason for hiding this comment

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

\count()

@Pierstoval
Copy link
Contributor Author

As #28 is merged, the bugfix part is OK.

For the rest, we discussed with @dunglas on Slack and for now the socket part is not something we really need.

Let's close 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants