search

docker-library / php

Docker Official Image packaging for PHP
https://php.net
MIT License
3.77k stars 2k forks source link

Add environment variable for PHP_FPM_DIR #1452

Open tclavier opened 8 months ago

LaurentGoderre commented 8 months ago

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

LaurentGoderre commented 8 months ago

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

tclavier commented 8 months ago

@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 commented 7 months ago

Issue https://github.com/nextcloud/docker/pull/1766 is waiting for this one :-)

tianon commented 7 months ago

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

tclavier commented 7 months ago

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 :

tianon commented 7 months ago

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

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 commented 7 months ago

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 commented 7 months ago

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). :see_no_evil: