Support clear_env variable by PHP_CLEAR_ENV

[phracek]

This pull request fixes issue #403.

By parameter PHP_CLEAR_ENV=OFF user can set 'clear_env = false' in /etc/pphp-fpm.d/www.conf file.

[phracek]

[test]

[phracek]

@pkubatrh @hhorak @remicollet Tests passed. Can you please have a look at it and made a review? Your review is more than appropriate. Thanks. The change is valid only for RHEL9 and only in case of PHP_CLEAR_ENV is set to true.

[hhorak]

It would be good to include more info in the commit messages, a summary from https://github.com/sclorg/s2i-php-container/issues/403 might work to explain basic background why clearing environment is not desired sometimes.

[hhorak]

We should likely document this new variable in the README.md, shouldn't we?

[phracek]

e should likely document this new variable in the README.md, shouldn't we?

README.md files are updated. Thanks for the point.

[phracek]

It would be good to include more info in the commit messages, a summary from #403 might work to explain basic background why clearing environment is not desired sometimes.

I have tried to summarize it somehow. If something is still unclear, please let me know.

[phracek]

[test]

[phracek]

@remicollet Hi Remi, once you have free or spare time. Can you please have a look at this pull request? Thank you.

[phracek]

[test]

[xrow]

@phracek @remicollet @hhorak I would love to see this merged... I think it got stuck somewhere in the nirvana... :-)

[phracek]

I am gonna finish this next week, so also tests are passing. Please be patient.

[xrow]

Actually I didn`t see that someone worked on it last week. My bad.

[phracek]

The pull request is ready for review. @xrow @pkubatrh @hhorak

[test]

[phracek]

[test-openshift]

[xrow]

Looks good to me, but i can only test once it is merged.

[phracek]

Let's re-run tests after fixing some issues which are fixed by https://github.com/sclorg/s2i-php-container/pull/406/commits/76d4cf1bfb12e123921ad4cd96ebeefde974658e

[test]

[phracek]

[test-openshift]

[phracek]

@remicollet @pkubatrh @hhorak Please review it. So we can merge it.

[phracek]

Pull request was rebased against master

[test-all]

[phracek]

[test]

[remicollet]

I don't understand the change

• why making FPM configuration word writable?

• about clear_env

We already have in https://github.com/sclorg/s2i-php-container/blob/master/8.1/root/usr/share/container-scripts/php/common.sh#L47

    sed -e 's/^(clear_env)\s+.*/clear_env = no/' -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}

This sed comment probably need to be fixed, as the default line in RPM provided file is

;clear_env = no

[phracek]

fixed

Thanks for the review, I will fix it. You are right.

[phracek]

We already have in https://github.com/sclorg/s2i-php-container/blob/master/8.1/root/usr/share/container-scripts/php/common.sh#L47

    sed -e 's/^(clear_env)\s+.*/clear_env = no/' -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}

This sed comment probably need to be fixed, as the default line in RPM provided file is

;clear_env = no

@remicollet Good comment. In case of PHP_CLEAR_ENV is set to false, than I add to configuration file at the end clear_env = no. There are three posibbilities.

• clear_env is not present in www.conf file and then it should be added
• ;clear_env = no PHP_CLEAR_ENV is set to yes then clear_env = no is added at the end.
• clear_env = no is present and then it will be mentioned twice in configuration file.

Do we want to handle all those states? The condition that will handled all those cases would be a bit complicated. I did not try it yet ;) But can try

[remicollet]

AFAIK;clear_env = no is present in configuration seems exists (PHP 5.6)

IIUC the current code, is that we always expect clear_env to be disabled (but is broken).

Perhaps a simple fix of the sed is enough.

- sed -e 's/^(clear_env)\s+.*/clear_env = no/' -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}
+ sed -e 's/^[;]*\s*clear_env\s*=.*$/clear_env = no/'  -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}

And If you really want a new config, PHP_CLEAR_ENV being true to disable, this name seems confusing Also IMHO it should also allow both value

so:

if [ "${PHP_CLEAR_ENV:-ON}" == "ON" ]; then
    sed -e 's/^[;]*\s*clear_env\s*=.*$/clear_env = yes/'  -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}
else
    sed -e 's/^[;]*\s*clear_env\s*=.*$/clear_env = no/'  -i ${PHP_FPM_CONF_D_PATH}/${PHP_FPM_CONF_FILE}
fi

[phracek]

Rebased and added also to 8.2 version.

[phracek]

[test]

[phracek]

[test-openshift]

[phracek]

Tests are passing. Completely.

[phracek]

[test]

[xrow]

Hi @phracek,

sorry for my late reply, but I just noticed it. It looks like having this functionality only in run script should be sufficent for all use cases. No need for functionality container-setup (unless it is also prt of some run activity) and assemble, but maybe there are reasons for it. Just a comment. No need to fix.