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

PHP ini overrides are not working #8

Closed
jessedobbelaere opened this issue Apr 20, 2024 · 8 comments
Closed

PHP ini overrides are not working #8

jessedobbelaere opened this issue Apr 20, 2024 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@jessedobbelaere
Copy link

jessedobbelaere commented Apr 20, 2024

Description

Overriding PHP settings with env variables such as PHP_MEMORY_LIMIT does not seem to work at the moment.

Issue 1: not loading the craftcms.ini

The 60-craftcms.ini file does not get loaded by PHP:
image

The Dockerfile might not be using the right location to put 60-craftcms.ini in?

- COPY etc/php.d/60-craftcms.ini /etc/php.d/60-craftcms.ini
+ COPY etc/php.d/60-craftcms.ini /etc/php/8.3/fpm/conf.d/60-craftcms.ini

Issue 2: settings cannot get overridden because of php_admin_value

After fixing the issue where 60-craftcms.ini did not load, I could see the ini file loaded in phpinfo ✅ .
However, the memory_limit still did not change ⚠️

I figured out that memory_limit is set in the php-fpm settings using:

php_admin_value[memory_limit] = "256M"

I looked it up and found:

Unlike php_value, php_admin_value stands immune to local overwrites. Regardless of the attempts made in individual directories or files to alter the same configuration setting, the global directive set via php_admin_value remains supreme. This characteristic renders it an excellent choice for safeguarding vital configurations against inadvertent alterations.

So in fact, there's no way that any .ini file can override settings like memory_limit. Could this get fixed?


Feature request: allow to override open_basedir

Should there be an ENV var to modify open_basedir? Or at least, also stop using php_admin_value here and switch to php_value so I can copy a custom .ini to override open_basedir?

php_admin_value[open_basedir] = /app:/dev/stdout:/tmp

👉 I have installed /usr/bin/cwebp, /usr/bin/cavif, /usr/bin/jpegoptim and /usr/bin/optipng that Imager-X needs to use. The open_basedir feature of PHP prevents PHP from using files in the /usr/bin directory. So I need to modify it with a .ini file, but that requires me to use sed magic to uncomment the open_basedir in the php-fpm config, unfortunately.

Quite the hack, but using this I could proof the steps to override the open_basedir setting:

RUN sed -i 's/php_admin_value\[open_basedir\] = \/app:\/dev\/stdout:\/tmp/;php_admin_value\[open_basedir\] = \/app:\/dev\/stdout:\/tmp/' /etc/php-fpm.conf
RUN echo "open_basedir=/app:/dev/stdout:/tmp:/usr/bin/jpegoptim:/usr/bin/optipng:/usr/bin/cwebp:/usr/bin/cavif" > /etc/php/8.3/fpm/conf.d/70-custom.ini

Steps to reproduce

  1. Go to examples/nginx and add an environment variable as mentioned in the README.md
version: "3.8"
services:
  web:
    image: craftcms/web:local
    build:
      context: .
      dockerfile: Dockerfile
      args:
        - php_version=8.3
    ports:
      - "8080:8080"
+   environment:
+     PHP_MEMORY_LIMIT: 512M
    volumes:
      - ./local:/app
  1. Run docker compose up
  2. Visit http://localhost:8080 and look in the phpinfo for memory_limit --> it will be set at 256M instead of 512M!. You will also see that it did not load 60-craftcms.ini at all.
@jessedobbelaere jessedobbelaere added the bug Something isn't working label Apr 20, 2024
@jessedobbelaere
Copy link
Author

Currently worked around using:

RUN mv /etc/php.d/60-craftcms.ini /etc/php/8.3/fpm/conf.d/60-craftcms.ini && \
    sed -i '/^php_admin_value\[open_basedir\]/s/^/;/' /etc/php-fpm.conf && \
    sed -i '/^php_admin_value\[memory_limit\]/s/^/;/' /etc/php-fpm.conf && \
    echo "open_basedir=/app:/dev/stdout:/tmp:/usr/bin/jpegoptim:/usr/bin/optipng:/usr/bin/cwebp:/usr/bin/cavif" >> /etc/php/8.3/fpm/conf.d/70-custom.ini

@jasonmccallister jasonmccallister self-assigned this Jun 7, 2024
@jasonmccallister
Copy link
Member

@jessedobbelaere thank you for the report, I will look into this and let you know.

@internetztube
Copy link

I have the same problem. Are there any plans for timing? @jasonmccallister

@carlcs
Copy link

carlcs commented Aug 30, 2024

I have to say that the situation with the Docker images is pretty disappointing. The old image is deprecated, and the new one isn’t really usable. It’s frustrating that issues like this one, or another one I opened #14, aren’t getting any attention.

@jasonmccallister
Copy link
Member

Hi all, appreciate your patience. Now that Cloud has launched and things have gone back to some semblance of normal here I'm circling back to our public container images.

@jessedobbelaere, @internetztube I'm going to try and reproduce this. None of the clients that use these images have had a need to tweak the PHP configuration.

@carlcs do you mind elaborating on how you consider these unusable? We have received a lot of feedback over the years about the other images saying they are too opinionated and difficult to customize (e.g. people wanting to swap Nginx for Caddy). We opted for a base image with requirements and less opinions and are using these for some enterprise customers with really good success. I'm happy to discuss if you feel things are missing or we could do things to make it easier to use these images.

As far as #14, I did need to follow up on it but saw that it was closed.

Again, happy to discuss ways we can make these images easier to use.

@jasonmccallister
Copy link
Member

Just resolved this and verified on PHP 8.3 with the following command:

$ docker run --rm -it -e PHP_UPLOAD_MAX_FILESIZE=100M --pull=always --entrypoint php-fpm ghcr.io/craftcms/image:8.3 -i | grep upload
8.3: Pulling from craftcms/image
Digest: sha256:41d989b8e7c0ead233cb825e2b76caec12342fdfbebc6bbcfb396572cd933b09
Status: Image is up to date for ghcr.io/craftcms/image:8.3
file_uploads => On => On
max_file_uploads => 20 => 20
upload_max_filesize => 100M => 100M
upload_tmp_dir => no value => no value
session.upload_progress.cleanup => On => On
session.upload_progress.enabled => On => On
session.upload_progress.freq => 1% => 1%
session.upload_progress.min_freq => 1 => 1
session.upload_progress.name => PHP_SESSION_UPLOAD_PROGRESS => PHP_SESSION_UPLOAD_PROGRESS
session.upload_progress.prefix => upload_progress_ => upload_progress_

I'm going to add this to the other images and also ensure the php CLI will also pick up the config changes.

@jasonmccallister
Copy link
Member

Marking this as resolved, these actions will update the 8.2 and 8.1 images with the changes:

https://github.com/craftcms/image/actions/runs/10725422755
https://github.com/craftcms/image/actions/runs/10725359700

@jessedobbelaere
Copy link
Author

jessedobbelaere commented Sep 9, 2024

Hi @jasonmccallister thanks for looking into it! That should do it

However my post also had a Issue 2 part to it, I'll make a new issue for that #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants