matomo-org / component-ini

Read and write INI configurations.
GNU Lesser General Public License v3.0
50 stars 25 forks source link

Add filename property to IniReadingException... #3

Closed diosmosis closed 9 years ago

diosmosis commented 9 years ago

...so file w/ error can be determined when multiple INI files are processed sequentially.

Necessary for https://github.com/piwik/piwik/pull/7312

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.9%) to 73.78% when pulling 2c4acaa8a3a70386bc3acb1d3965d03ae811c4dd on ini_read_exception_filename into 2a2179e56e6e906364e7e978305a2b75b7b34eb0 on master.

mnapoli commented 9 years ago

Having the pseudo-file "<string>" for errors while reading a string doesn't really make sense IMO. Instead I would have a IniFileReadingException extends IniReadingException with a getFilename() method.

That way you can do:

try {
    ...
} catch (IniFileReadingException $e) {
    $file = $e->getFilename();
    // do something with the file
} catch (IniReadingException $e) {
    // do something else?
}
mnapoli commented 9 years ago

Actually I'm not even sure it belongs here?

If the problem is this part:

    public function reload($defaultSettingsFiles = array(), $userSettingsFile = null)
    {
        // ...
        foreach ($this->settingsChain as $file => $ignore) {
            if (is_readable($file)) {
                $this->settingsChain[$file] = $reader->readFile($file);
            }
        }
        // ...

and the calling code:

try {
    $this->settings->reload(array($this->pathGlobal, $this->pathCommon), $this->pathLocal);
} catch (IniReadingException ...

I would simply change the calling code:

    public function reload($defaultSettingsFiles = array(), $userSettingsFile = null)
    {
        // ...
        foreach ($this->settingsChain as $file => $ignore) {
            if (is_readable($file)) {
                try {
                    $this->settingsChain[$file] = $reader->readFile($file);
                } catch (IniReadingException) {
                    throw new Exception(Piwik::translate('General_ExceptionUnreadableFileDisabledMethod', array($file, "parse_ini_file()")));
                }
            }
        }
        // ...

I.e. no need to change the library here, if there's an exception in readFile(), it's pretty obvious which file is causing the exception.

Also with the current code you will be showing the error:

The INI file is not readable or parse_ini_file() is disabled

That's not correct:

I'm afraid it will only confuse people.

diosmosis commented 9 years ago

You're right, changing the calling code is better. I didn't change any of the exception messages, so whatever it is, is what was there before. Perhaps it can be improved.