Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warning: Undefined index: instance_id #8371

Closed
wants to merge 2 commits into from
Closed

Fix warning: Undefined index: instance_id #8371

wants to merge 2 commits into from

Conversation

ThaDafinser
Copy link
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)

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)
@ThaDafinser ThaDafinser changed the title Fix warning Fix warning: Undefined index: instance_id Jul 17, 2015
@mnapoli
Copy link
Contributor

mnapoli commented Jul 19, 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…

if (!empty($instanceId)) {
return $instanceId;
$configGeneral = Config::getInstance()->General;
if(isset($configGeneral['instance_id']) && !empty($configGeneral['instance_id'])){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to check isset when using !empty.

@ThaDafinser
Copy link
Contributor Author

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

@mnapoli
Copy link
Contributor

mnapoli commented Jul 20, 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
Copy link
Contributor Author

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

sgiehl commented Jul 20, 2015

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

@ThaDafinser
Copy link
Contributor Author

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
Copy link
Contributor

mnapoli commented Jul 20, 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
Copy link
Member

mattab commented Jul 20, 2015

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

👍 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)

@mattab mattab closed this Jul 20, 2015
@mattab mattab added the wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it. label Jul 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants