Skip to content

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?

public function waitUntilReady(Process $process, string $url, bool $ignoreErrors = false): void
public function waitUntilReady(Process $process, string $url): void
{
$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.

\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