Skip to content
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

Add environment variable for PHP_FPM_DIR #1452

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tclavier
Copy link

No description provided.

@LaurentGoderre LaurentGoderre changed the title Expose PHP_FPM_DIR Add environment variable for PHP_FPM_DIR Oct 27, 2023
@LaurentGoderre
Copy link
Member

Changed the title because "expose" could mean make accessible outside the container

@LaurentGoderre
Copy link
Member

@tclavier to get the change in, you would need to run ./apply-template for the changes to be applied to the image themselves.

@tclavier
Copy link
Author

tclavier commented Nov 1, 2023

@tclavier to get the change in, you would need to run ./apply-template for the changes to be applied to the image themselves.

@LaurentGoderre it's done

@tclavier
Copy link
Author

Issue nextcloud/docker#1766 is waiting for this one :-)

@tianon
Copy link
Member

tianon commented Nov 27, 2023

Sorry, I think I'm missing some context here (just got off some long PTO) -- what problem is this solving?

@tclavier
Copy link
Author

Sorry, I think I'm missing some context here (just got off some long PTO) -- what problem is this solving?

There is some fpm configuration that goes directly into the PHP_FPM_DIR folder, and :

  • if the path of this folder changes, it would be good to drop the configuration in the right folder.
  • It's more explicit to write $PHP_FPM_DIR/myfile.ini than /usr/local/etc/php-fpm.d/myfile.ini.

@tianon
Copy link
Member

tianon commented Nov 29, 2023

I don't think it's changed in the roughly nine years since FPM was added 😅

My concern with adding it as an explicit ENV like this is that it implies that it might be something we could change at runtime, but the change won't actually apply, and it doesn't really save many characters for users either.

@tclavier
Copy link
Author

I'm not trying to save characters, but to make it explicit and readable. And I find it really simple to find out where the fpm conf is by exploring the environment variables. There's a PHP_INI_DIR variable and the first instinct is to put all the php configuration in PHP_INI_DIR ... but for fpm it's the wrong folder.

If I have to choose between these different options :

  1. $PHP_FPM_DIR/myfile.ini
  2. /usr/local/etc/php-fpm.d/myfile.ini.
  3. $PHP_INI_DIR/../php-fpm.d/myfile.ini

I have a strong preference for option 1, I find it easier to read and more explicit.

@tianon
Copy link
Member

tianon commented Nov 29, 2023

I guess I'm trying to say that if I wrote this Dockerfile fresh today, I probably wouldn't even include ENV PHP_INI_DIR in it, and breaking users is the only reason I'm not considering removing it retroactively (but might consider doing so in a PHP-version conditional for future releases). 🙈

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.

None yet

3 participants