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 simple environment-based PHP configuration #167

Merged
merged 1 commit into from
Mar 18, 2023

Conversation

andrewnicols
Copy link
Contributor

This adds a layer of common sense on top of the other changes I've worked on to allow easy configuration variable setting.

This is currently based on top of #162 and #163 together, but can be refactored easily.

The tl;dr is:

docker run \
    --name web0 \
    -p 8080:80 \
    -v $PWD/moodle:/var/www/html
    -e INI-upload_max_filesize=200M \
    -e INI-post_max_size=210M \
    moodle-php-apache:latest

Will cause a new php.ini file to be created at `/usr/local/etc/php/conf.d/10-local.ini containing:

upload_max_filesize = 200M
post_max_size = 210M

@andrewnicols andrewnicols force-pushed the phpini branch 2 times, most recently from 9266002 to 08592a3 Compare March 16, 2023 01:35
@andrewnicols
Copy link
Contributor Author

Note: This is intended to co-exist with the entrypoint changes - the rationale being that we should make it as easy as possible to make small changes, but still allow larger ones.

Adding an environment variable is very easy.
Specifying an entrypoint directory with appropriate .ini configuration is significantly harder.

They are not mutually exclusive, and both are optional. We can provide neither, either, or both.

@scara
Copy link
Contributor

scara commented Mar 16, 2023

Hi @andrewnicols,
this is really a nice feature! 💪

My trivial comment: why the INI- prefix?
At first glance I would use PHP_INI- (or PHP_INI.) to better cope with the domain of those ENV VAR settings.

The official image is actually using PHP_INI_* (e.g. PHP_INI_DIR) too to provide read-only info.

HTH,
Matteo

@andrewnicols
Copy link
Contributor Author

I'm happy to go with either if anyone has a preference.

@stronk7
Copy link
Member

stronk7 commented Mar 16, 2023

Fun, I was to comment also about the "INI" prefix when saw @scara already did.

My point was more about future expansions we could do, like adding stuff to apache conf or git conf... or whatever.

In any case, for me PHP_INI/php_ini looks perfect.

This adds a layer of common sense on top of the other changes I've
worked on to allow easy configuration variable setting.

Fixes moodlehq#166
@andrewnicols
Copy link
Contributor Author

I've updated the patch to change it to use PHP_INI instead.

@stronk7 stronk7 merged commit ff023e2 into moodlehq:master Mar 18, 2023
@stronk7
Copy link
Member

stronk7 commented Mar 18, 2023

Sold, thanks!

I'll be backporting this to php >= 74 images (like entrypoints @ #162).

@stronk7
Copy link
Member

stronk7 commented Mar 19, 2023

Done, all the php >= 74 images have been rebuilt to include this.

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.

3 participants