laravel / valet

A more enjoyable local development experience for Mac.
https://laravel.com/docs/valet
MIT License
2.49k stars 691 forks source link

add server name to valet.conf #1458

Closed Oleg339 closed 2 months ago

Oleg339 commented 7 months ago

Added server name to nginx for more comfortable debugging in PHPStorm. PHPStorm requires not empty server_name

drbyte commented 7 months ago

Can you point to documentation showing how PHPStorm does this, and how it wants to be configured?

drbyte commented 7 months ago

I'm concerned that simply using server_name valet; will cause problems.

As outlined in the nginx docs, the server_name is used for determining which server {} block to use for serving a given incoming request. Calling it a specific valid string that will never match an incoming URI request could lead to unexpected results (like no requests served).

Given that Valet aims to serve anything directed at it (unlike a real live webserver which aims to serve only for the domain names that server is actually servicing), perhaps specifying some of its supported fallbacks may be suitable without breaking the "serve anything" objective:

Thus probably something like: #valet# or @valet@ or !valet! And possibly also add the empty "" to support empty incoming names.

So, I propose: server_name "!valet!" "";

But I'd like to see the difference in PHPStorm's reaction to such a config. Hence requesting more information on their expectations.

drbyte commented 7 months ago

Also: it's only the non-secured "sites" that have had no server_name directive. All valet's "secured" sites name the specific domain that's been secured (to match the SSL cert), so are not nameless.

Oleg339 commented 7 months ago

Also: it's only the non-secured "sites" that have had no server_name directive. All valet's "secured" sites name the specific domain that's been secured (to match the SSL cert), so are not nameless.

I'm concerned that simply using server_name valet; will cause problems.

As outlined in the nginx docs, the server_name is used for determining which server {} block to use for serving a given incoming request. Calling it a specific valid string that will never match an incoming URI request could lead to unexpected results (like no requests served).

Given that Valet aims to serve anything directed at it (unlike a real live webserver which aims to serve only for the domain names that server is actually servicing), perhaps specifying some of its supported fallbacks may be suitable without breaking the "serve anything" objective:

  • I'd suggest avoiding hard-coded things like even the *.test TLD because then it requires updating anytime someone tinkers with that. Leaves it too fragile IMO.
  • "$hostname" uses the PC's machine name ... but probably we're not sending requests for that "domain" to valet
  • "" as a blank value is probably no better than leaving out the server_name directive as we've always done
  • "localhost" might be okay as a secondary directive
  • Since invalid characters (!@#) are treated as a catchall, perhaps #valet# might be the best to satisfy PHPStorm?

Thus probably something like: #valet# or @valet@ or !valet! And possibly also add the empty "" to support empty incoming names.

So, I propose: server_name "!valet!" "";

But I'd like to see the difference in PHPStorm's reaction to such a config. Hence requesting more information on their expectations.

This requirement is not stated in the phpstorm documentation, but I think it will be enough to show a screenshot and a thread on the jetbrains forum. https://intellij-support.jetbrains.com/hc/en-us/community/posts/360009885759-xdebug-not-communicating-with-phpstorm

i think server_name "!valet!" ""; good option

Снимок экрана 2023-11-30 в 20 22 31
drbyte commented 7 months ago

What version of PHPStorm are you using?

Their ticket system indicates that a fix was implemented awhile back.

Oleg339 commented 7 months ago

What version of PHPStorm are you using?

Their ticket system indicates that a fix was implemented awhile back.

I using 2023.2.4 version for macos(last version)

Снимок экрана 2023-11-30 в 21 37 11
mattstauffer commented 6 months ago

I was following the thread until this:

What version of PHPStorm are you using?

Their ticket system indicates that a fix was implemented awhile back.

Is there something we need to consider that I missed here? Otherwise I'm hoping it's good to merge?

drbyte commented 6 months ago

I don't have xdebug set up to explore this with.

jasonvarga commented 2 months ago

Hello 👋

I've just been trying to set up Xdebug in PhpStorm via Valet and I have just stumbled onto this issue.

tl;dr This PR fixes solves the issue and doesn't seem to introduce any problems.

I wanted to chime in and give some info since this PR seems pretty dormant. (Much of this is probably info you already knew). Bear with me!

When PhpStorm is listening for Xdebug connections, since Valet doesn't send a $SERVER['SERVER_NAME'], it comes into PhpStorm blank.

CleanShot 2024-05-09 at 16 40 00

This an issue if you want to customize anything about the server configuration as PhpStorm won't let you save without a host. (e.g. If you're dealing with symlinks, this is something you'll have to do)

CleanShot 2024-05-09 at 16 42 35

By adding the server_name in this PR, even though !valet! is a bit weird to see, at least it's not blank and can be used.

CleanShot 2024-05-09 at 16 45 07

(This blank behavior doesn't happen if you've valet secure'd your site, since that site would get its own nginx config with a server_name and would work fine.)

Browsing other Valet sites, this addition to the config file doesn't seem to have any negative effects.

The only downside I can see is if you were doing some variation of $_SERVER['SERVER_NAME'] in your app code somewhere, it will now output !valet!. However it would have been blank before, which is probably just as unexpected.

Some drivers seem to set this manually (for example, the basic driver). Using $_SERVER['SERVER_NAME'] in your app wouldn't change at all in these cases.

I'm not sure what the reason was for adding that line to BasicDriver etc but it wouldn't help the xdebug situation since PhpStorm would have already picked up the request before Valet sets it.

Hope this is useful! 😅

drbyte commented 2 months ago

Thanks @jasonvarga , great to have an additional set of eyes on this confirming its viability.

I'm satisfied that this PR is both useful and harmless.

/cc @mattstauffer

driesvints commented 2 months ago

Thanks all