@ThaDafinser opened this Pull Request on July 17th 2015 Contributor

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 Member

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 Contributor

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

@mnapoli commented on July 20th 2015 Member

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 Contributor

@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 Member

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

@ThaDafinser commented on July 20th 2015 Contributor

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 Member

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 Owner

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 Pull Request was closed on July 20th 2015
Powered by GitHub Issue Mirror