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: set variables_order to EGPCS #10182

Open
wants to merge 1 commit into
base: PHP-8.0
Choose a base branch
from

Conversation

verfriemelt-dot-org
Copy link

@verfriemelt-dot-org verfriemelt-dot-org commented Dec 29, 2022

heyho,

during development i encountered some issues with $_ENV and cli scripts invoked like this:

$ CONFIG_TARGET=qa composer symlinks

which was odd, because on my debian box this worked fine but on a macos install via homebrew it was not working. we noticed that variables_order was set to GPCS and thus ommitting the $_ENV superglobal i required to have the CONFIG_TARGET set.

after some digging around, i found the formular for php on homebrew with its source for the default configuration:

https://github.com/Homebrew/homebrew-core/blob/56299f3dac37eaf9fe0b1e187b889f559a74be26/Formula/php.rb#L219

    config_files = {
      "php.ini-development"   => "php.ini",
      "php.ini-production"    => "php.ini-production",
      "sapi/fpm/php-fpm.conf" => "php-fpm.conf",
      "sapi/fpm/www.conf"     => "php-fpm.d/www.conf",
    }

the default for the variables_order is defined as ECPGS in the documentation but both the php.ini-production and php.ini-development had those set to GPCS which contradicts the docs.

the current dockerhub image also uses EGPCS as a sane default

$ docker run --pull=always -it --rm php php -i 2>/dev/null | grep variables_order
variables_order => EGPCS => EGPCS

so i figured the right thing to do is to update both php.ini files according to the documentation.

the provided defaults contradicts the documentation so we update the
defaults accordingly.
@cmb69
Copy link
Contributor

cmb69 commented Dec 30, 2022

There is a comment in the ini files:

There is a performance penalty paid for the registration of these arrays and because ENV is not as commonly used as the others, ENV is not recommended on productions servers.

So it makes sense to me to not include E.

If you still want to change it, please target the "master" branch, and send email to the internals mailing list for discussion.

@verfriemelt-dot-org verfriemelt-dot-org changed the title php.init: set variables_order to EGPCS php.ini: set variables_order to EGPCS Dec 30, 2022
@verfriemelt-dot-org
Copy link
Author

👍 i sent a mail to the mailing list to discuss this. lets see.
i wonder if the performance impact is still relevant?

@cmb69
Copy link
Contributor

cmb69 commented Dec 30, 2022

i wonder if the performance impact is still relevant?

If not, we should remove that comment. :)

By the way, I'm not pressing "approve and run", since this change wouldn't be checked anyway. The commit message could have been started with [ci skip].

@verfriemelt-dot-org
Copy link
Author

which php.ini is used for the tests? i noticed, that some tests override this setting to include E ?

@cmb69
Copy link
Contributor

cmb69 commented Dec 30, 2022

which php.ini is used for the tests?

That depends on the environment; I don't think our CI uses any particular php.ini files (except for AppVeyor, but that only declares some extensions, and a few OPcache related settings).

@ramsey
Copy link
Member

ramsey commented Sep 20, 2023

because ENV is not as commonly used as the others

This is a strange part of the comment, and I doubt this is the case anymore, since we build applications differently that we did 13 years ago. It's now a common practice to inject ENV vars into your application instead of using local config files for production. Additionally, getenv() and putenv() are not thread safe, so they're discouraged against by libraries such as vlucas/phpdotenv.

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