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.

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

Fix ini_get() for boolean values #29020

Merged
merged 1 commit into from Oct 31, 2018
Merged

Fix ini_get() for boolean values #29020

merged 1 commit into from Oct 31, 2018

Conversation

deguif
Copy link
Contributor

@deguif deguif commented Oct 29, 2018 •
edited

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

Currently setting false or off, ... value to configure some PHP ini directives will make this evaluated to true as this is equal to a non empty string.

ndench
ndench approved these changes Oct 29, 2018
View reviewed changes
Copy link
Contributor

@ndench ndench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Oct 30, 2018
nicolas-grekas
nicolas-grekas reviewed Oct 30, 2018
View reviewed changes
src/Symfony/Component/ClassLoader/Tests/ApcClassLoaderTest.php Outdated
@@ -19,7 +19,7 @@ class ApcClassLoaderTest extends TestCase
{
protected function setUp()
{
if (!(ini_get('apc.enabled') && ini_get('apc.enable_cli'))) {
if (!(\filter_var(ini_get('apc.enabled'), \FILTER_VALIDATE_BOOLEAN) && \filter_var(ini_get('apc.enable_cli'), \FILTER_VALIDATE_BOOLEAN))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove all the \ added in front of filter_var() and FILTER_VALIDATE_BOOLEAN in the PR?
We don't add these except for a specific short list of functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, changed ;)

@ro0NL
Copy link
Contributor

ro0NL commented Oct 30, 2018

See also

symfony/src/Symfony/Bundle/FrameworkBundle/Command/AboutCommand.php

Lines 86 to 87 in 2cd1e11

array('OPcache', \extension_loaded('Zend OPcache') && ini_get('opcache.enable') ? 'true' : 'false'),
array('APCu', \extension_loaded('apcu') && ini_get('apc.enabled') ? 'true' : 'false'),

symfony/src/Symfony/Component/Debug/Debug.php

Line 48 in 2cd1e11

} elseif ($displayErrors && (!ini_get('log_errors') || ini_get('error_log'))) {
also maybe

@ro0NL
Copy link
Contributor

ro0NL commented Oct 30, 2018

oh wait.. i checked master :) AboutCommand would be for 3.4

@deguif
Copy link
Contributor Author

deguif commented Oct 30, 2018

Yes, will do another PR which will target 3.4 after ;)

@deguif
Copy link
Contributor Author

deguif commented Oct 30, 2018

I'm also going to check every ini_get, I just focused on opcache and apc for the moment, going to send them ASAP

@ro0NL
Copy link
Contributor

ro0NL commented Oct 30, 2018 •
edited

Cool :) i was curious if this applies to all boolean typed ini directives. I think it does 👍 https://3v4l.org/aYsiU

@deguif
Copy link
Contributor Author

deguif commented Oct 30, 2018

Ok, just added

  • log_errors
  • xcache.cacher
  • wincache.ocenabled

Not sure about eaccelerator.enable as it doesn't use the macro STD_PHP_INI_BOOLEAN

@fabpot
Copy link
Member

fabpot commented Oct 31, 2018

Thank you @deguif.

@fabpot fabpot merged commit a153869 into symfony:2.8 Oct 31, 2018
fabpot added a commit that referenced this pull request Oct 31, 2018
@fabpot
bug #29020 Fix ini_get() for boolean values (deguif)
5a2969c 
This PR was merged into the 2.8 branch.

Discussion
----------

Fix ini_get() for boolean values

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

Currently setting `false` or `off`, ... value to configure some PHP ini directives will make this evaluated to `true` as this is equal to a non empty string.

Commits
-------

a153869 Fix ini_get() for boolean values
@deguif deguif deleted the 2.8 branch October 31, 2018 08:57
@deguif deguif restored the 2.8 branch October 31, 2018 09:16
@deguif deguif mentioned this pull request Oct 31, 2018
Merged
This was referenced Nov 3, 2018
Merged
Merged
Merged
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
This was referenced Mar 12, 2021
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

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

@ndench ndench ndench approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@deguif @ro0NL @fabpot @nicolas-grekas @ndench @ogizanagi @carsonbot