@ThaDafinser opened this issue on July 17th 2015

WARNING: ...\piwik\core\SettingsPiwik.php(408): Notice - Undefined index: instance_id - Piwik 2.14.1 - Please report this message in the Piwik forums: http://forum.piwik.org (please do a search first as it might have been reported already)

@mnapoli commented on July 19th 2015

Weird, the @ should prevent the warning from appearing (even with our custom error handler: https://github.com/piwik/piwik/blob/master/core/ErrorHandler.php#L69-L72). I don't understand why the warning appeared…

@ThaDafinser commented on July 20th 2015

@mnapoli not with xdebug.scream = On setting IMO we should try to get rid of the @

@mnapoli commented on July 20th 2015

Ah right, then I would say it's a wontfix then. We use the error suppressor everywhere in Piwik. I'm not saying it's good, but removing it here because of xdebug (which shouldn't be installed or enabled in production) wouldn't make sense if we leave it everywhere else.

@ThaDafinser commented on July 20th 2015

@mnapoli i was developing a plugin and wanted to test it in development, therefor i need the Piwik installation. (I dont want to change my php.ini settings because of one project)

But i know what thats only one place of many.

@sgiehl commented on July 20th 2015

Shouldn't we work towards removing all of the error suppression? So why not starting with this one?

@ThaDafinser commented on July 20th 2015

To remove all suppressions from the config it would be easiert to change this Config::getInstance()->General['instance_id'] to something like Config::getInstance()->get('general', 'instance_id') And then return null or something else if not set

@mnapoli commented on July 20th 2015

i was developing a plugin and wanted to test it in development, therefor i need the Piwik installation. (I dont want to change my php.ini settings because of one project)

Makes sense. However the "scream" option is meant to achieve exactly what you get here, and currently @ is accepted practice so it's kind of incompatible (or at least, inconvenient for you).

So we get back to this point:

Shouldn't we work towards removing all of the error suppression? So why not starting with this one?

I wouldn't be against discussing that with pros and cons (and I'm not against removing @), but I'm just trying to go in the correct order of things :) The error suppressor is not deemed "bad" in current Piwik practices and it's still used and introduced in new code. So unless we decide to change that, it doesn't make sense to start removing it (since it will be added again later in other places).

TL/DR: before removing @ we should discuss and decide that we shouldn't use it anymore?

@mattab commented on July 20th 2015

TL/DR: before removing @ we should discuss and decide that we shouldn't use anymore?

:+1: for now, we don't need to remove the extras @ in front of INI config setting reading. (as said, it's regularly done in the codebase, and even in some of the new code)

This issue was closed on July 20th 2015
Powered by GitHub Issue Mirror