@mnapoli opened this Pull Request on January 7th 2015 Member

This PR is a suggestion for a new Piwik component to read and write INI configurations.

The new component contains 2 classes:

  • IniReader: content extracted from the shim in upgrade.php that makes it work even if the native PHP function is disabled
  • IniWriter: content extracted from Config (so it clears up this class a little bit)

Just like Symfony Yaml, I think this INI component could be interesting to be shipped as a standalone component. It's a good example of a small reusable one.

Here is the advantages of this component that I would write in its README:

  • allows to read and write (no native PHP function to write INI files)
  • the classes are mockable in unit tests
  • works even if parse_ini_file() is disabled
  • throws exceptions instead of errors (can be easily catched, less if ( === false)…)
  • boolean support when parsing: true, yes, on are actually converted to true instead of 1, same for false, no, off

Here is the advantage for us:

Thoughts?

@mattab commented on January 7th 2015 Owner

Great idea, this change makes our code less cluttered and cleaner, and also will benefit the community, it is a great move :+1:

(fyi didn't review the code in detail, but I love that we will have tests now!)

@Globulopolis commented on January 7th 2015 Contributor

@mnapoli you say "works even if parse_ini_file() is disabled" - maybe we need to add some checks if function exists instead of writing own function, because native function much faster.

And $array = @parse_ini_string($ini, true, INI_SCANNER_RAW); what is it? Suppress an error output is bad idea I'm think.

@mnapoli commented on January 7th 2015 Member

@Globulopolis thanks for the feedback.

It does use the native PHP function, the shim is just used if the function is disabled.

Regarding <a class='mention' href='https://github.com/parse_ini_string'>@parse_ini_string</a>(...) suppressing an error would be bad if it was just suppressed, but:

$array = <a class='mention' href='https://github.com/parse_ini_string'>@parse_ini_string</a>($ini, true, INI_SCANNER_RAW);

if ($array === false) {
    $e = error_get_last();
    throw new IniReadingException('Syntax error in INI configuration: ' . $e['message']);
}

I'm actually checking for the error just afterwards and turning it into an exception. So the error is not silenced, it's turned into an exception.

@mnapoli commented on January 8th 2015 Member

The new component is here: https://github.com/piwik/component-ini

@mattab commented on January 9th 2015 Owner

Nice, gotta love new small reusable components keeping the core lean :+1:

This Pull Request was closed on January 9th 2015
Powered by GitHub Issue Mirror