Skip to content

Conversation

@drbyte
Copy link
Contributor

@drbyte drbyte commented Dec 1, 2019

This allows the valet FPM pool configuration to stand separately from the default config.

This benefits troubleshooting, makes customization of FPM workers and other settings easer
and allows for easier uninstallation.

(Note: existing Valet installs will have had their PHP /usr/local/etc/php/7.4/php-fpm.d/www.conf files already altered by Valet. It will be best to restore this file to the default version installed by PHP to prevent duplicate FPM pool instances that conflict with each other.
Or, simply delete the www.conf file.)

@drbyte drbyte changed the title Move PHP-FPM config to separate valet-specific file Move PHP-FPM pool config to separate valet-specific file Dec 1, 2019
@mattstauffer
Copy link
Collaborator

@drbyte I love this. Only question is, how should we handle restoring existing PHP 7+ installs' www.conf files?

@drbyte
Copy link
Contributor Author

drbyte commented Dec 2, 2019

@mattstauffer ya, I've been debating how best to handle that.

While I'd prefer not to clutter Valet with a bunch of "upgrade" code, it's probably unavoidable.

Two options:

  1. Delete the "old" www.conf pool file altogether. If Valet has already customized it then "default PHP" isn't going to be offended if we just remove it. I think this is the best option. (And for fresh installs I'm inclined to also rename the www.conf to www.conf.default so that PHP doesn't actually spin up workers that are just wasting memory.)
  2. Restore the old contents. Of primary concern is the listen= line so that it doesn't point to valet.sock. While the other things deserve cleanup too, they're not what's going to "conflict" with Valet.

@mattstauffer
Copy link
Collaborator

@drbyte I 100% agree with you on all points. I don't want the junk upgrade code but not sure what else to do. I prefer the nuclear option, and I love the idea of renaming it on new ones.

@drbyte
Copy link
Contributor Author

drbyte commented Dec 2, 2019

I'll work on an update later this afternoon.

@drbyte
Copy link
Contributor Author

drbyte commented Dec 2, 2019

@mattstauffer I'm toying with just renaming the www.conf to www.conf.old ... regardless of whether Valet has touched it before.

@mattstauffer
Copy link
Collaborator

@drbyte YES. Why not? If someone needs it, they're already in an advanced-enough situation that they can go look it up.

@drbyte
Copy link
Contributor Author

drbyte commented Dec 2, 2019

@mattstauffer Update pushed.

This allows the valet configuration to stand separately from the default PHP config.
This benefits troubleshooting, makes customization of FPM workers and other settings easer
and allows for easier uninstallation.

Also renames any previously-existing `www.conf` pool config so it doesn't conflict with Valet nor run unnecessary additional processes.
@drbyte
Copy link
Contributor Author

drbyte commented Dec 5, 2019

Updated to also make sure the php-fpm.d directory exists, since it was reported today in #865 that a php@7.2 install failed to create it properly for some reason.

@mattstauffer
Copy link
Collaborator

@drbyte <3

@mattstauffer mattstauffer merged commit fea8208 into laravel:master Dec 5, 2019
@drbyte drbyte deleted the phpfpm-simplify branch December 5, 2019 20:46
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