New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/phpfpm: fix apply global phpOptions #72767
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we renaming? I like the names the way they are. If the documentation isn't clear enough can we please just update the documentation instead?
The name of the option immediately shows that it applies to all pools.
After:
And here it is easy to deteсt which parameters belong to global:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as @aanderse.
I don't see the point in this rename and I think we shouldn't do it.
Maybe it's possible to do if we do a big refactor or something at the same time so we actually gain some more benefits from the module. But as far as I can see from reading the diff here... we don't really change anything that much really...
Ok. Updated PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Izorkin please try this instead:
~/nixpkgs> git diff
diff --git a/nixos/modules/services/web-servers/phpfpm/default.nix b/nixos/modules/services/web-servers/phpfpm/default.nix
index 4ab7e3f0c0a..f33a740abeb 100644
--- a/nixos/modules/services/web-servers/phpfpm/default.nix
+++ b/nixos/modules/services/web-servers/phpfpm/default.nix
@@ -69,8 +69,6 @@ let
phpOptions = mkOption {
type = types.lines;
- default = cfg.phpOptions;
- defaultText = "config.services.phpfpm.phpOptions";
description = ''
"Options appended to the PHP configuration file <filename>php.ini</filename> used for this PHP-FPM pool."
'';
@@ -137,6 +135,7 @@ let
config = {
socket = if poolOpts.listen == "" then "${runtimeDir}/${name}.sock" else poolOpts.listen;
group = mkDefault poolOpts.user;
+ phpOptions = mkBefore cfg.phpOptions;
settings = mapAttrs (name: mkDefault){
listen = poolOpts.socket;
This would allow you to customize much further, more flexibility.
services.phpfpm.phpOptions = ''
; changes to apply to every php.ini file
'';
services.phpfpm.pools.pool1.phpOptions = ''
; changes which will be appended onto services.phpfpm.phpOptions for this pool
'';
services.phpfpm.pools.pool2.phpOptions = mkForce ''
; changes which will overwrite services.phpfpm.phpOptions for this pool
'';
Thanks for this variant. Updated PR. |
This code:
generates a file in this form
How to make lines displayed first:
|
For future reference: Attribute sets in Nix are sorted alphabetically and there's no nice and easy way to change that for something like this. |
Motivation for this change
Fixed apply of parameter
config.services.phpfpm.phpOptions
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @