Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 6, 2018
Merged

Fix ini_get() for boolean values #29041

merged 1 commit into from
Nov 6, 2018

Conversation

deguif
Copy link
Contributor

@deguif deguif commented Oct 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

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 31, 2018
@jvasseur
Copy link
Contributor

jvasseur commented Oct 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 (and false if the setting doesn't exists).

@ro0NL
Copy link
Contributor

ro0NL commented Oct 31, 2018

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 31, 2018 •
edited
Loading

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

@deguif
Copy link
Contributor Author

deguif commented Oct 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 nicolas-grekas mentioned this pull request Oct 31, 2018
Merged
@fabpot
Copy link
Member

fabpot commented Oct 31, 2018

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

@nicolas-grekas
Copy link
Member

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
@fabpot
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.ini

Commits
-------

ce53261 [DI] use filter_var() instead of XmlUtils::phpize() in EnvVarProcessor
@nicolas-grekas
Copy link
Member

Thank you @deguif.

@nicolas-grekas nicolas-grekas merged commit 65b34cb into symfony:3.4 Nov 6, 2018
nicolas-grekas added a commit that referenced this pull request Nov 6, 2018
@nicolas-grekas
minor #29041 Fix 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.4

Commits
-------

65b34cb Fix ini_get() for boolean values
@alquerci alquerci mentioned this pull request Nov 10, 2018
Closed
@deguif deguif deleted the 3.4 branch November 12, 2018 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@deguif @jvasseur @ro0NL @nicolas-grekas @fabpot @xabbuh @carsonbot