Page MenuHomePhabricator

Undefined index: lower-roman in cite_body.php
Closed, ResolvedPublic

Description

Notice: Undefined index: lower-roman in /srv/mediawiki/php-1.27.0-wmf.4/extensions/Cite/Cite_body.php on line 654

Revisions and Commits

Event Timeline

mmodell created this task.Oct 29 2015, 7:34 PM
mmodell raised the priority of this task from to Needs Triage.
mmodell updated the task description. (Show Details)
mmodell added a subscriber: mmodell.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 29 2015, 7:34 PM
mmodell triaged this task as Unbreak Now! priority.Oct 29 2015, 7:41 PM

1.27.0-wmf.4 had to be rolled back because this was spamming logs a lot.

I guess it happens because "!$this->mRefs[$group]" is used instead of "!isset( $this->mRefs[$group] )"?

The gerrit code review comments are amusing: https://gerrit.wikimedia.org/r/#/c/223250/

Krinkle added a comment.EditedOct 29 2015, 9:57 PM

Fatal logs during wmf.4 being on wikipedias: https://logstash.wikimedia.org/#dashboard/temp/

This task is about a PHP notice though, those shouldn't end up on fatalmonitor?

The gerrit code review comments are amusing: https://gerrit.wikimedia.org/r/#/c/223250/

They are amusing and related to array counting, not array index existence. In fact, this regression supports the view that empty() has a negative impact on code quality and maintainability due to the inability to understand what code intends to do because empty() does it all.

Fortunately the rest of the Cite extension (before the above commit) already used isset() for this particular purpose. Only one place in the code used empty() instead of isset(). It was an outlier that had to be replaced with one of two things: boolean cast or isset (depending on why empty() is there). We picked the wrong one.

Fixed in https://gerrit.wikimedia.org/r/#/c/249889/.

Change 249889 had a related patch set uploaded (by Krinkle):
Add isset() check before accessing $this->mRefs[$group]

https://gerrit.wikimedia.org/r/249889

Change 249889 merged by jenkins-bot:
Add isset() check before accessing $this->mRefs[$group]

https://gerrit.wikimedia.org/r/249889

Umherirrender closed this task as Resolved.Nov 1 2015, 5:59 PM
Umherirrender assigned this task to Krinkle.
Umherirrender set Security to None.