Page MenuHomePhabricator
Create Task
Maniphest T212771

[Bug] Special:Contributions broken when the Flow extension is loaded: "Cannot access private property ContribsPager::$tagFilter"
Closed, ResolvedPublic
Actions

Description

Steps to reproduce

  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Special:Contributions/phuedx-test-1

Expected results

I expect to see the contributions for that user.

Actual results

Screenshot 2019-01-02 at 10.47.52.png (1×2 px, 216 KB)

Environments observed

  • Browser version: Google Chrome (71.0.3578.98)
  • OS version: macOS Mojave (10.14.2)
  • Device model: Mac mini (Late 2014)
  • Device language: English

Check any additional observations

Stack trace

PHP fatal error: 
Cannot access private property ContribsPager::$tagFilter

Notes

Prior to rMWa67a4b10871a: Add missing, dynamically declared properties in ContribsPager, ContribsPager::$tagFilter was dynamically created in ContribsPager::__construct and was therefore public. While there are no accesses of the property in the mediawiki codebase, ContribsPager::reallyDoQuery runs the hook of the same name. Flow's FlowHooks::onContributionsQuery handles this hook and access the property there: https://gerrit.wikimedia.org/g/mediawiki/extensions/Flow/+/d840802bc596434e65cb5757e18f3bddd3dbc101/Hooks.php#921

I can't be absolutely certain of the above as I haven't been able to find a strack trace yet.

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+5 -15Make ContribsPager properties private again
mediawiki/extensions/Flowmaster+9 -8Use getters to access ContribsPager properties
mediawiki/coremaster+8 -1Replace $this with options object in ContribsPager::reallyDoQuery hook
mediawiki/extensions/Flowmaster+27 -11Make ContributionsQuery accept value objects instead of DeletedContribsPager
mediawiki/extensions/Flowmaster+14 -8Make ContributionsQuery work with a value object instead of a ContribsPager
mediawiki/coremaster+35 -0Add getters for properties accessed by Flow
mediawiki/coremaster+33 -0DeletedContribsPager: Add getters for properties accessed by Flow
mediawiki/coremaster+15 -5Temporarily make ContribsPager properties public to unblock Flow
Customize query in gerrit

Related Objects
Search...

Event Timeline

phuedx created this task.Jan 2 2019, 11:02 AM
Restricted Application added a project: Growth-Team. · View Herald TranscriptJan 2 2019, 11:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
phuedx added a comment.Jan 2 2019, 11:03 AM

@thiemowmde @Umherirrender: I've subscribed you both as you authored and merged the change I mentioned, respectively.

phuedx mentioned this in T200927: Enable smooth/animated scrolling between links on pages.Jan 2 2019, 11:03 AM
phuedx renamed this task from [Bug] Special:Contributions broken for users that exist to [Bug] Special:Contributions broken when the Flow extension is loaded.Jan 2 2019, 11:06 AM
Aklapper renamed this task from [Bug] Special:Contributions broken when the Flow extension is loaded to [Bug] Special:Contributions broken when the Flow extension is loaded: "Cannot access private property ContribsPager::$tagFilter".Jan 2 2019, 12:18 PM
Umherirrender added a parent task: T206666: 1.33.0-wmf.12 deployment blockers.Jan 2 2019, 12:20 PM

At the moment I have no quick idea to avoid this property access (Doing the check in another way).

It needs to made public or a getter to access the value. A getter function would increase the version requirements to 1.33.0

phuedx added a subscriber: • pmiazga.Jan 2 2019, 12:49 PM
Jdlrobson triaged this task as Unbreak Now! priority.Jan 2 2019, 4:00 PM
Jdlrobson added a subscriber: Jdlrobson.

Marking as unbreak now before it hits production as 500s there on such an accessible page would not be a good thing!

Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptJan 2 2019, 4:00 PM
kostajh added a subscriber: kostajh.Jan 2 2019, 4:03 PM

It needs to made public or a getter to access the value. A getter function would increase the version requirements to 1.33.0

Alright, shall we just make it a public property for now?

phuedx added a comment.Jan 2 2019, 4:17 PM
In T212771#4849357, @kostajh wrote:

Alright, shall we just make it a public property for now?

👍

gerritbot added a subscriber: gerritbot.Jan 2 2019, 4:26 PM

Change 481877 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Make some properties public so Flow and other extensions can access

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

gerritbot added a project: Patch-For-Review.Jan 2 2019, 4:26 PM
phuedx added a comment.Jan 2 2019, 4:30 PM

Meta:

I think the original change was a good one – constraining a very loose API is a Good Thing™. However, while the ContribsPager API was very loose (a number of member variables were created in the constructor that happily defaulted to public!), I have to ask: should the public member variables have been formally deprecated?

gerritbot added a comment.Jan 2 2019, 5:21 PM

Change 481891 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] Replace $this with options object in ContribsPager::reallyDoQuery hook

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

gerritbot added a comment.Jan 2 2019, 5:46 PM

Change 481900 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Flow@master] Make ContributionsQuery work with a value object instead of a ContribsPager

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

thiemowmde added a comment.Jan 2 2019, 5:59 PM

I uploaded https://gerrit.wikimedia.org/r/481891 and https://gerrit.wikimedia.org/r/481900 as an alternative to https://gerrit.wikimedia.org/r/481877.

I wonder why no browser test broke? Doesn't have Flow tests?

Temporarily making the properties public might still be a good quick hack to unbreak production, if necessary. But I strongly suggest to not make this bad design a permanent thing. The point of a "pager" is to execute a query. It is not a value object that describes what the query will do. If such a value object is needed, it should be created. And this is exactly what my suggested patches do.

https://gerrit.wikimedia.org/r/481900 introduces two minor issues in the Flow code that could be solved by slightly modifying the patch. Instead of replacing the ContribsPager reference with a value object, we could add it as a parameter to the hook. The ContribsPager reference will then still be used to call $pager->getIndexField() as well as $pager->getDatabase(). More precisely: that would not be a ContribsPager reference any more, but an IndexPager reference, because all methods are declared there.

Jdlrobson added a comment.Jan 2 2019, 6:56 PM

I wonder why no browser test broke? Doesn't have Flow tests?

Browser tests did break... but in MobileFrontend [1] which tests the mobile version of the page.

I think Flow's phpunit tests run against core so would be great as part of this to add some protection at that level.

[1] https://integration.wikimedia.org/ci/view/Reading-Web/job/selenium-MobileFrontend/

Catrope assigned this task to thiemowmde.Jan 3 2019, 1:20 AM
Catrope edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
Catrope moved this task from Incoming to Code Review on the Growth-Team (Current Sprint) board.
gerritbot added a comment.Jan 3 2019, 8:44 AM

Change 481990 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Flow@master] Make ContributionsQuery accept value objects instead of DeletedContribsPager

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

gerritbot added a comment.Jan 3 2019, 7:09 PM

Change 482119 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Add getters for properties accessed by Flow

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

gerritbot added a comment.Jan 3 2019, 7:09 PM

Change 482120 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/Flow@master] Use getters to access ContribsPager properties

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

gerritbot added a comment.Jan 3 2019, 7:09 PM

Change 481877 abandoned by Kosta Harlan:
Make some properties public so Flow and other extensions can access

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

gerritbot added a comment.Jan 4 2019, 2:44 AM

Change 482259 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] DeletedContribsPager: Add getters for properties accessed by Flow

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

thiemowmde mentioned this in T200259: ContribsPager should use a unique criterion for paging.Jan 4 2019, 9:37 AM
gerritbot added a comment.Jan 4 2019, 9:43 AM

Change 481877 restored by Thiemo Kreuz (WMDE):
Make some properties public so Flow and other extensions can access

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

gerritbot added a comment.Jan 4 2019, 10:12 AM

Change 481877 merged by jenkins-bot:
[mediawiki/core@master] Temporarily make ContribsPager properties public to unblock Flow

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.12; 2019-01-08).Jan 4 2019, 11:00 AM
Jdlrobson lowered the priority of this task from Unbreak Now! to High.Jan 4 2019, 5:28 PM
Jdlrobson removed a project: MW-1.33-notes (1.33.0-wmf.12; 2019-01-08).

Thanks for the temporary fix our browser tests are passing again and I'm not seeing any other issues. Does this temporary fix need to be SWATed into next week's deploy while we work out the rest (I'm a bit confused about the timeline here and post-holiday deploy schedule) ?

kostajh added a comment.Jan 4 2019, 5:53 PM

@Jdlrobson No need to SWAT unless further changes are made after the branch is cut on Tuesday and we decide they need to be on the train.

gerritbot added a comment.Jan 4 2019, 9:16 PM

Change 482119 merged by jenkins-bot:
[mediawiki/core@master] Add getters for properties accessed by Flow

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

gerritbot added a comment.Jan 4 2019, 9:16 PM

Change 482259 merged by jenkins-bot:
[mediawiki/core@master] DeletedContribsPager: Add getters for properties accessed by Flow

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

gerritbot added a comment.Jan 4 2019, 9:36 PM

Change 482358 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Make ContribsPager properties private again

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.12; 2019-01-08).Jan 4 2019, 10:00 PM
gerritbot added a comment.Jan 4 2019, 10:34 PM

Change 481900 abandoned by Catrope:
Make ContributionsQuery work with a value object instead of a ContribsPager

Reason:
Superseded by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/ /482120

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

gerritbot added a comment.Jan 4 2019, 10:34 PM

Change 481990 abandoned by Catrope:
Make ContributionsQuery accept value objects instead of DeletedContribsPager

Reason:
Superseded by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/ /482120

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

gerritbot added a comment.Jan 4 2019, 10:35 PM

Change 481891 abandoned by Catrope:
Replace $this with options object in ContribsPager::reallyDoQuery hook

Reason:
Superseded by https://gerrit.wikimedia.org/r/c/mediawiki/core/ /482119 and https://gerrit.wikimedia.org/r/c/mediawiki/core/ /482259

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

gerritbot added a comment.Jan 4 2019, 10:50 PM

Change 482120 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Use getters to access ContribsPager properties

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

gerritbot added a comment.Jan 4 2019, 10:50 PM

Change 482358 merged by jenkins-bot:
[mediawiki/core@master] Make ContribsPager properties private again

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

kostajh moved this task from Code Review to QA on the Growth-Team (Current Sprint) board.Jan 4 2019, 10:56 PM
Catrope closed this task as Resolved.Jan 4 2019, 11:02 PM
Catrope reopened this task as Open.
Catrope added a subscriber: Catrope.

Whoops, forgot that this needs QA still

Etonkovidova closed this task as Resolved.Jan 4 2019, 11:39 PM
Etonkovidova added a subscriber: Etonkovidova.

Checked in betalabs - added to my list of stuff to check after deployment (wmf.12).

Liuxinyu970226 removed a subscriber: Liuxinyu970226.Jan 16 2019, 1:33 AM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL