NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Fix ini_get() for boolean values#29041

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:3.4fromdeguif:3.4

Nov 6, 2018

Merged

Fix ini_get() for boolean values#29041

nicolas-grekas merged 1 commit intosymfony:3.4fromdeguif:3.4

Nov 6, 2018

Conversation

Copy link

Contributor

deguif commentedOct 31, 2018

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

This follows#29020 for branch 3.4

carsonbot added Status: Needs Review Bug labels

Oct 31, 2018

nicolas-grekas added this to the3.4 milestone

Oct 31, 2018

Copy link

Contributor

jvasseur commentedOct 31, 2018•
edited
Loading

This shouldn't be needed,ini_get already returns a normalized value for boolean configs :"" for false and"1" for true (andfalse if the setting doesn't exists).

Copy link

Contributor

ro0NL commentedOct 31, 2018

maybe it only happens due ini_set()?https://3v4l.org/flvAL

Copy link

Member

nicolas-grekas commentedOct 31, 2018•
edited
Loading

ini_set() and ENV-defined ini settings, seecomposer/composer#7760 (comment)

Copy link

ContributorAuthor

deguif commentedOct 31, 2018

To add more details, the issue is encountered when using environment variables to configure some PHP ini directives.

#php.ini[opcache]opcache.enable_cli = ${PHP_INI_OPCACHE_ENABLE_CLI}

PHP_INI_APC_ENABLE_CLI=true php -r "var_dump(ini_get('apc.enable_cli')); new APCUIterator();" will enable apcu and outputs:

string(4) "true"

PHP_INI_APC_ENABLE_CLI=false php -r "var_dump(ini_get('apc.enable_cli')) ; new APCUIterator();"
will disable apcu and outputs:

string(5) "false"Fatal error: APCuIterator::__construct(): APC must be enabled to use APCuIterator in Command line code on line 1

nicolas-grekas mentioned this pull request

Oct 31, 2018

[DI] use filter_var() instead of XmlUtils::phpize() in EnvVarProcessor#29042

Merged

Fix ini_get() for boolean values

65b34cb

deguif force-pushed the3.4 branch from04f5cea to65b34cbCompare

October 31, 2018 13:11

carsonbot removed the Status: Needs Review label

Oct 31, 2018

nicolas-grekas approved these changes

Oct 31, 2018

View reviewed changes

carsonbot added the Status: Reviewed label

Oct 31, 2018

Copy link

Member

fabpot commentedOct 31, 2018

I'm not sure we want to do this dance for all calls toini_get. Most of those settings should not be set via env vars anyway (or is there another case where we need this?).

Copy link

Member

nicolas-grekas commentedNov 1, 2018

Let's do it IMHO: that prevents us from wondering about which settings are legit changing via env vars. There is no easy reasoning so we will be wrong at some point in the future. There are only a few calls in the code base also...

fabpot added a commit that referenced this pull request

Nov 2, 2018

feature#29042[DI] use filter_var() instead of XmlUtils::phpize() in…

d699036

… EnvVarProcessor (nicolas-grekas)This PR was submitted for the 3.4 branch but it was merged into the 4.2-dev branch instead (closes#29042).Discussion----------[DI] use filter_var() instead of XmlUtils::phpize() in EnvVarProcessor| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -#29041 made me realize that we don't need this dependency on the Config component: `filter_var()` is just fine. This allows using a few more legit values for boolean styles, which are already accepted in php.iniCommits-------ce53261 [DI] use filter_var() instead of XmlUtils::phpize() in EnvVarProcessor

xabbuh approved these changes

Nov 6, 2018

View reviewed changes

Copy link

Member

nicolas-grekas commentedNov 6, 2018

Thank you@deguif.

nicolas-grekas merged commit65b34cb intosymfony:3.4

Nov 6, 2018

nicolas-grekas added a commit that referenced this pull request

Nov 6, 2018

minor#29041Fix ini_get() for boolean values (deguif)

6cfd3de

This PR was merged into the 3.4 branch.Discussion----------Fix ini_get() for boolean values| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |This follows#29020 for branch 3.4Commits-------65b34cb Fix ini_get() for boolean values