Page MenuHomePhabricator

PHP warning on Gather collections page
Closed, ResolvedPublic

Description

Visit http://localhost:8888/w/index.php/Special:Gather/by/Jdlrobson

Notice: Undefined property: Gather\models\CollectionsList::$mode in /Users/jrobson/git/core/extensions/Gather/includes/models/CollectionsList.php on line 240

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to To Do on the Reading-Web-Sprint-50-The-X-Files board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 22 2015, 6:04 PM

Does the warning message show up on the page? Should I set some debug key to true to see this?

It will if you have

error_reporting( -1 );
ini_set( 'display_errors', 1 );

Change 219958 had a related patch set uploaded (by Jdlrobson):
Make sure mode is always set to avoid php warnings

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

Change 219958 merged by jenkins-bot:
Make sure mode is always set to avoid php warnings

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

phuedx closed this task as Resolved.Jun 25 2015, 11:19 AM
phuedx added a subscriber: phuedx.

tl;dr: The fix is OK.

This could have done with a little more investigation into the underlying problem: it seems like a CollectionsList model is being initialised but never having its mode set and then being rendered.

The /by/<user> route is handled by SpecialGather#renderUserCollectionsList, which calls CollectionsList::newFromApi (with $mode defaulting to null). Regardless of what happens in the body of ::newFromApi, the result never has its mode set, despite an empty string being calculated as its default.

… the fix is OK as '' is a sensible default for mode and now clients don't explicitly have to set it.

Jdlrobson added a comment.EditedJun 25 2015, 3:24 PM

Note the fix is needed as getMode is being called before setMode
Certain collections do not care about mode. This is why the issue was happening. No investigation needed. The line you point to is an API parameter and not the same as the internal protected property.

Certain collections do not care about mode.

This kind of information would have been useful in reproducing the bug/demonstrating that your patch fixed the bug/for posterity. Could you provide more detail next time around?

The line you point to is an API parameter and not the same as the internal protected property.

I missed that the meaning $mode was overloaded there. Mibad. Though, why is $mode reassigned if it's falsy? I guess it doesn't matter.