[DependencyInjection] fixed ini file values conversion #20232

fabpot · 2016-10-18T04:04:47Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no-ish
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

When using the ini format to load parameters in the Container, the parameter values were converted by PHP directly ('true' => 1 for instance). But when using the YAML or XML format, the conversions are much broader and more precise ('true' => true for instance). This PR fixed fixes this discrepancy by using the same rules as XML (we could use INI_SCANNER_TYPED for recent versions of PHP but the rules are not exactly the same, so I prefer consistency here).

One might argue that this is a new feature and that this should be merged into master, which I can accept as well. In master, the XmlUtils::phpize() method should be deprecated and replaced by a more generic phpize class.

ping @symfony/deciders

javiereguiluz · 2016-10-18T06:47:04Z

👍

It can be debatable to merge it in 2.7 or master ... but to me this is a bugfix, so it should stay in 2.7.

dunglas · 2016-10-18T06:59:24Z

👍

nicolas-grekas · 2016-10-18T14:55:10Z

👍 for 2.7 also

Tobion · 2016-10-18T15:49:17Z

src/Symfony/Component/DependencyInjection/Tests/Loader/IniFileLoaderTest.php

+            '-10100.1' => -10100.1,
+            '-10,100.1' => '-10,100.1',
+        );
+        $this->assertEquals($expected, $this->container->getParameterBag()->all(), '->load() converts values to PHP types');


AFAIK assertEquals doesn't ensure correct types, e.g. '1' == 1. So the test is not really precise.

ro0NL · 2016-10-18T18:10:29Z

@fabpot can you test with on, off, yes, no... i think this can get problematic ;-) The BC break is real: https://3v4l.org/bW8P0

edit: also constants/nulls are affected

fabpot · 2016-10-18T18:20:28Z

@ro0NL Not sure I understand your concern. Can you be more precise?

ro0NL · 2016-10-18T18:22:31Z

This breaks configuration like

[parameters]
   is_this_enabled = yes ; comment

right? AFAIK this is not covered by XmlUtils::phpize nor this PR...

Before string("1") => bool(true), after string("yes ")

edit: comments are just buggy with INI_SCANNER_RAW :) https://3v4l.org/7pV3u

ro0NL · 2016-10-18T19:39:52Z

@fabpot perhaps more serious, no escaping is applied: https://3v4l.org/oGJqj

nicolas-grekas · 2016-11-02T13:57:41Z

@ro0NL is right, we need to polyfill INI_SCANNER_TYPED...

stof · 2016-11-02T14:02:42Z

src/Symfony/Component/DependencyInjection/Tests/Loader/IniFileLoaderTest.php

@@ -40,6 +33,34 @@ public function testIniFileCanBeLoaded()
    }

    /**
+     * @requires PHP 5.3


useless, as we cannot run on PHP < 5.3 anyway

ro0NL · 2016-11-02T20:12:55Z

@nicolas-grekas we basically already doing that with INI_SCANNER_NORMAL + XmlUtils::phpize.

Imo a

more generic phpize class

would be sufficient.

or simply patch the current ini loader on numerics, etc.

ro0NL · 2016-11-03T20:33:43Z

src/Symfony/Component/DependencyInjection/Loader/IniFileLoader.php

+                ("'" === $value[0] && "'" === $value[strlen($value) - 1]) ||
+                ('"' === $value[0] && '"' === $value[strlen($value) - 1])
+            ):
+                // quoted string


Perhaps throw if 1 === strlen($value) (invalid syntax)?

the php.ini file would not be valid in that case.

It was fatal before :/ https://3v4l.org/VgDvJ

With SCANNER_RAW it parses though.. but that doesnt mean it's valid, it's raw :)

fabpot · 2016-11-03T20:38:39Z

I've updated this PR to take into account how the scanner type convert values, and added some more tests. As for the failure on PHP 7.1, not sure what changed. Is it better now?

fabpot · 2016-11-03T20:52:01Z

I propose to merge it into master instead of 2.7 as it's more complex than anticipated.

nicolas-grekas · 2016-11-03T20:53:41Z

Using INI_SCANNER_TYPED when possible would be a great way to ensure the polyfill behaves as expected

fabpot · 2016-11-03T20:55:42Z

@nicolas-grekas Not really possible as our behavior is not exactly the same (we must be as close as possible to it but also be closed to what XmlUtils does to be consistent with the other formats).

nicolas-grekas · 2016-11-03T21:53:25Z

src/Symfony/Component/DependencyInjection/Loader/IniFileLoader.php

+                return true;
+            case 'no' === $value || 'off' === $value || 'none' === $value:
+                return false;
+            case isset($value[0]) && (


I propose this patch:

--- a/src/Symfony/Component/DependencyInjection/Loader/IniFileLoader.php +++ b/src/Symfony/Component/DependencyInjection/Loader/IniFileLoader.php @@ -64,15 +64,16 @@ class IniFileLoader extends FileLoader { // trim on the right as a comment leaves the whitespaces in place $value = rtrim($value); + $lowercaseValue = strtolower($value); switch (true) { case defined($value): return constant($value); - case 'yes' === $value || 'on' === $value: + case 'yes' === $lowercaseValue || 'on' === $lowercaseValue: return true; - case 'no' === $value || 'off' === $value || 'none' === $value: + case 'no' === $lowercaseValue || 'off' === $lowercaseValue || 'none' === $lowercaseValue: return false; - case isset($value[0]) && ( + case isset($value[1]) && ( ("'" === $value[0] && "'" === $value[strlen($value) - 1]) || ('"' === $value[0] && '"' === $value[strlen($value) - 1]) ): diff --git a/src/Symfony/Component/DependencyInjection/composer.json b/src/Symfony/Component/DependencyInjection/composer.json index 9c12d11..e24187f 100644 --- a/src/Symfony/Component/DependencyInjection/composer.json +++ b/src/Symfony/Component/DependencyInjection/composer.json @@ -20,7 +20,7 @@ }, "require-dev": { "symfony/yaml": "~2.3.42|~2.7.14|~2.8.7", - "symfony/config": "~2.2", + "symfony/config": "~2.7", "symfony/expression-language": "~2.6" }, "conflict": {

fabpot · 2016-11-03T23:22:56Z

Rebased on master

… (fabpot) This PR was merged into the 3.2-dev branch. Discussion ---------- [DependencyInjection] fixed ini file values conversion | Q | A | | --- | --- | | Branch? | master | | Bug fix? | yes | | New feature? | no | | BC breaks? | no-ish | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | n/a | | License | MIT | | Doc PR | n/a | When using the ini format to load parameters in the Container, the parameter values were converted by PHP directly (`'true'` => `1` for instance). But when using the YAML or XML format, the conversions are much broader and more precise (`'true'` => `true` for instance). This PR fixed fixes this discrepancy by using the same rules as XML (we could use `INI_SCANNER_TYPED` for recent versions of PHP but the rules are not exactly the same, so I prefer consistency here). One might argue that this is a new feature and that this should be merged into master, which I can accept as well. In master, the `XmlUtils::phpize()` method should be deprecated and replaced by a more generic phpize class. ping @symfony/deciders Commits ------- 4ccfce6 [DependencyInjection] fixed ini file values conversion

…Cat) This PR was merged into the 4.4 branch. Discussion ---------- [Config] Remove obsolete warning about INI files Spotted thanks to #17232 (comment) Since symfony/symfony#20232 the claim that “you can only set parameters to string values” is not valid anymore. Commits ------- 7a67d8f Remove obsolete warning

carsonbot added Status: Needs Review DependencyInjection Bug labels Oct 18, 2016

fabpot force-pushed the ini-type-conversion-fix branch 3 times, most recently from ea41f76 to db988f3 Compare October 18, 2016 04:30

fabpot force-pushed the ini-type-conversion-fix branch from db988f3 to ead4617 Compare October 18, 2016 15:44

Tobion reviewed Oct 18, 2016

View reviewed changes

fabpot force-pushed the ini-type-conversion-fix branch 3 times, most recently from f77cdda to 4110de6 Compare October 18, 2016 17:52

stof reviewed Nov 2, 2016

View reviewed changes

fabpot force-pushed the ini-type-conversion-fix branch 6 times, most recently from 4accc8f to 8a3e741 Compare November 3, 2016 20:03

ro0NL reviewed Nov 3, 2016

View reviewed changes

fabpot force-pushed the ini-type-conversion-fix branch from 8a3e741 to 5026d27 Compare November 3, 2016 20:50

fabpot force-pushed the ini-type-conversion-fix branch from 5026d27 to 824520d Compare November 3, 2016 20:50

nicolas-grekas reviewed Nov 3, 2016

View reviewed changes

fabpot force-pushed the ini-type-conversion-fix branch from 824520d to 7e315ac Compare November 3, 2016 23:16

fabpot changed the base branch from 2.7 to master November 3, 2016 23:16

fabpot force-pushed the ini-type-conversion-fix branch from 7e315ac to 61b4c4a Compare November 3, 2016 23:17

[DependencyInjection] fixed ini file values conversion

4ccfce6

fabpot force-pushed the ini-type-conversion-fix branch from 61b4c4a to 4ccfce6 Compare November 3, 2016 23:43

fabpot merged commit 4ccfce6 into symfony:master Nov 3, 2016

fabpot mentioned this pull request Nov 17, 2016

Release v3.2.0-RC1 #20552

Merged

fabpot deleted the ini-type-conversion-fix branch January 7, 2017 00:34

lpd-au mentioned this pull request Aug 22, 2022

IniFileLoader throws when parsing arrays #47357

Closed

MatTheCat mentioned this pull request Sep 2, 2022

[Config] Remove obsolete warning about INI files symfony/symfony-docs#17247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] fixed ini file values conversion #20232

[DependencyInjection] fixed ini file values conversion #20232

fabpot commented Oct 18, 2016 •

edited

Loading

javiereguiluz commented Oct 18, 2016

dunglas commented Oct 18, 2016

nicolas-grekas commented Oct 18, 2016

Tobion Oct 18, 2016 •

edited

Loading

ro0NL commented Oct 18, 2016 •

edited

Loading

fabpot commented Oct 18, 2016

ro0NL commented Oct 18, 2016 •

edited

Loading

ro0NL commented Oct 18, 2016 •

edited

Loading

nicolas-grekas commented Nov 2, 2016

stof Nov 2, 2016

ro0NL commented Nov 2, 2016 •

edited

Loading

ro0NL Nov 3, 2016

fabpot Nov 3, 2016

ro0NL Nov 3, 2016

fabpot Nov 3, 2016

fabpot commented Nov 3, 2016

fabpot commented Nov 3, 2016

nicolas-grekas commented Nov 3, 2016

fabpot commented Nov 3, 2016 •

edited

Loading

nicolas-grekas Nov 3, 2016

fabpot commented Nov 3, 2016

[DependencyInjection] fixed ini file values conversion #20232

[DependencyInjection] fixed ini file values conversion #20232

Conversation

fabpot commented Oct 18, 2016 • edited Loading

javiereguiluz commented Oct 18, 2016

dunglas commented Oct 18, 2016

nicolas-grekas commented Oct 18, 2016

Tobion Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL commented Oct 18, 2016 • edited Loading

fabpot commented Oct 18, 2016

ro0NL commented Oct 18, 2016 • edited Loading

ro0NL commented Oct 18, 2016 • edited Loading

nicolas-grekas commented Nov 2, 2016

stof Nov 2, 2016

Choose a reason for hiding this comment

ro0NL commented Nov 2, 2016 • edited Loading

ro0NL Nov 3, 2016

Choose a reason for hiding this comment

fabpot Nov 3, 2016

Choose a reason for hiding this comment

ro0NL Nov 3, 2016

Choose a reason for hiding this comment

fabpot Nov 3, 2016

Choose a reason for hiding this comment

fabpot commented Nov 3, 2016

fabpot commented Nov 3, 2016

nicolas-grekas commented Nov 3, 2016

fabpot commented Nov 3, 2016 • edited Loading

nicolas-grekas Nov 3, 2016

Choose a reason for hiding this comment

fabpot commented Nov 3, 2016

fabpot commented Oct 18, 2016 •

edited

Loading

Tobion Oct 18, 2016 •

edited

Loading

ro0NL commented Oct 18, 2016 •

edited

Loading

ro0NL commented Oct 18, 2016 •

edited

Loading

ro0NL commented Oct 18, 2016 •

edited

Loading

ro0NL commented Nov 2, 2016 •

edited

Loading

fabpot commented Nov 3, 2016 •

edited

Loading