Skip to content

Conversation

@antonioribeiro
Copy link
Contributor

Before restarting nginx it will execute:

sudo nginx -c /usr/local/etc/nginx/nginx.conf -t

which, on success, must return

nginx: the configuration file /usr/local/etc/nginx/nginx.conf syntax is ok
nginx: configuration file /usr/local/etc/nginx/nginx.conf test is successful

And on fail vallet will tell

Nginx cannot start, please check your nginx.conf.

Copy link
Contributor

@adamwathan adamwathan left a comment

Choose a reason for hiding this comment

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

Couple tweaks but happy to merge after that. Thanks!

private function lint()
{
$this->cli->quietly(
'sudo nginx -c '.static::NGINX_CONF.' -t',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this use (&$error) here for? Doesn't look like that variable exists or is used.

$this->cli->quietly(
'sudo nginx -c '.static::NGINX_CONF.' -t',
function() use (&$error) {
info('Nginx cannot start, please check your nginx.conf.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling info and dying here, throw a DomainException like we do elsewhere:

https://github.com/laravel/valet/blob/master/cli/Valet/Brew.php#L103

@antonioribeiro
Copy link
Contributor Author

Done.

@adamwathan adamwathan merged commit 7e1d239 into laravel:master Mar 11, 2017
drbyte added a commit to drbyte/valet that referenced this pull request Nov 30, 2019
While since laravel#268 valet has been checking nginx configs for errors when starting/restarting, the captured errors are never displayed if there is a failure because it's being run using `quietly()`. 
This PR causes the errors to pass through to the console so we can more readily understand why nginx may not be starting or not serving properly.
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