laravel / valet

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

handle deprecated http2_push_preload conf for nginx >= 1.25.1 #1451

Closed OpJePl44tsm4n closed 6 months ago

OpJePl44tsm4n commented 9 months ago

Updated the config so it will work for fresh installs.

removed http2 and http2_push_preload because they are deprecated in latest Nginx versions.

drbyte commented 9 months ago

I agree that this is the correct fix.

Unfortunately it only works if the Nginx version is 1.25.1 or greater, which is quite new. The changed syntax is not backward-compatible. (Ref: nginx docs )

2023-08-15 | nginx-1.25.2 mainline version has been released. 2023-06-13 | nginx-1.25.1 mainline version has been released.

If someone is using (or is forced to use) an older Nginx version, they will get the following anytime they run valet secure or re-secure their sites with valet tld, and Valet will refuse to work until the old syntax is restored to all the "secured" Sites configs.

  Nginx cannot start; please check your nginx.conf [1: nginx: [warn] protocol options redefined for 127.0.0.1:443 in ~/.config/valet/Nginx/foo.test:9
  nginx: [emerg] unknown directive "http2" in ~/.config/valet/Nginx/foo.test:15
  nginx: configuration file /usr/local/etc/nginx/nginx.conf test failed
  ].

@mattstauffer how do you feel about incorporating nginx-version-detection so we can handle version-specific stub file handling?

OpJePl44tsm4n commented 9 months ago

@drbyte thank you for your response, I was thinking about that. Without a mayor version bump, probably a version check would be needed, since installing Valet requires the latest nginx version by default.

I'll take a look and will update the PR.

drbyte commented 9 months ago

At least the old syntax is not throwing Fatal errors and preventing Nginx from starting. Unfortunately the new syntax does throw fatal errors on older Nginx versions. 😢

OpJePl44tsm4n commented 9 months ago

@drbyte true but fresh installs will require lates version of nginx so now it's broken as is. option is to cap the nginx version in the install command and update this part.

$this->brew->installOrFail('nginx', []);

my suggestion is to add a check in Site.php to check the nginx version and require secure.valet.outdated.config for the older versions. This would make it work for all versions.

I'll make a fix so you can check it.

drbyte commented 9 months ago

@mattstauffer @driesvints I think this is ready for merge.

Of course, feel free to test on your local machine before+after upgrading nginx via brew

mattstauffer commented 6 months ago

@drbyte thank you so much for reviews and back-and-forths! Again, apologies for my delay here. @OpJePl44tsm4n thank you for this PR!

Testing now, and will merge if it doesn't bonk things for me.