Page MenuHomePhabricator
Create Task
Maniphest T267035

Call to undefined method MediaWiki\Extension\DiscussionTools\HeadingItem::addWarning()
Closed, ResolvedPublicBUG REPORT
Actions

Description

CommentParser::computeId has a parameter, ThreadItem $threadItem, and it checks that the object is an instance of either HeadingItem or CommentItem. Later, it sometimes tries to call $threadItem->addWarning( 'Duplicate comment ID' );
However, the addWarning method is not part of the ThreadItem interface, and while CommentItem::addWarning is defined, HeadingItem::addWarning() is not

See beta logstash: https://logstash-beta.wmflabs.org/goto/b2d5d6dba1a2543e737a94ccd8df9c50
Impact so far: 2 errors

Details

SubjectRepoBranchLines +/-
Move warnings stuff from CommentItem to ThreadItemmediawiki/extensions/DiscussionToolsmaster+1 K -30
Handle any errors, not just exceptionsmediawiki/extensions/DiscussionToolsmaster+3 -3
Handle exceptions in the OutputPageBeforeHTML hookmediawiki/extensions/DiscussionToolsmaster+23 -4
Customize query in gerrit

Related Objects
Search...

Event Timeline

DannyS712 created this task.Nov 2 2020, 3:38 PM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptNov 2 2020, 3:38 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Reports on the User-DannyS712 board.Nov 2 2020, 3:39 PM
DannyS712 changed the subtype of this task from "Task" to "Bug Report".
DannyS712 added a project: Release-Engineering-Team (Logspam).
DannyS712 mentioned this in T263182: 1.36.0-wmf.16 deployment blockers.
Krinkle added a parent task: T263182: 1.36.0-wmf.16 deployment blockers.Nov 2 2020, 6:28 PM
JTannerWMF added subscribers: Esanders, matmarex, • ppelberg, JTannerWMF.Nov 2 2020, 6:28 PM
• ppelberg added a project: Editing-team (Kanban Board).Nov 2 2020, 6:33 PM
gerritbot added a comment.Nov 2 2020, 6:36 PM

Change 638154 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Move warnings stuff from CommentItem to ThreadItem

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

gerritbot added a project: Patch-For-Review.Nov 2 2020, 6:36 PM
matmarex added a comment.Nov 2 2020, 6:45 PM

See beta logstash: https://logstash-beta.wmflabs.org/goto/b2d5d6dba1a2543e737a94ccd8df9c50

What do I need to do to view this?

thcipriani triaged this task as Unbreak Now! priority.Nov 2 2020, 6:48 PM
matmarex added a comment.Nov 2 2020, 6:51 PM

The buggy code was in 1.36.0-wmf.15 last week, but that was not deployed. I think we don't need to backport anything here as long as we merge before the branch cut in a few hours?

taavi subscribed.Nov 2 2020, 6:51 PM
In T267035#6597685, @matmarex wrote:

See beta logstash: https://logstash-beta.wmflabs.org/goto/b2d5d6dba1a2543e737a94ccd8df9c50

What do I need to do to view this?

See https://wikitech.wikimedia.org/wiki/Nova_Resource:Deployment-prep/Log_Files

DannyS712 added a comment.Nov 2 2020, 6:54 PM
In T267035#6597685, @matmarex wrote:

See beta logstash: https://logstash-beta.wmflabs.org/goto/b2d5d6dba1a2543e737a94ccd8df9c50

What do I need to do to view this?

Credentials are available at /root/secrets.txt on deployment-deploy01.deployment-prep.eqiad.wmflabs

gerritbot added a comment.Nov 2 2020, 7:12 PM

Change 638160 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Handle exceptions in the OutputPageBeforeHTML hook

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

matmarex added a comment.Nov 2 2020, 7:20 PM

The affected page is https://de.wikipedia.beta.wmflabs.org/wiki/Hilfe:VisualEditor/Bedienung. The entire page doesn't display because of this error, so I also submitted https://gerrit.wikimedia.org/r/638160 to fix that. And https://gerrit.wikimedia.org/r/c/638154 should fix the underlying issue.

gerritbot added a comment.Nov 2 2020, 8:00 PM

Change 638160 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Handle exceptions in the OutputPageBeforeHTML hook

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

gerritbot added a comment.Nov 2 2020, 8:31 PM

Change 638174 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Handle any errors, not just exceptions

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

gerritbot added a comment.Nov 2 2020, 8:57 PM

Change 638174 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Handle any errors, not just exceptions

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.16; 2020-11-03).Nov 2 2020, 9:01 PM
matmarex added a comment.Nov 2 2020, 9:29 PM

For future reference:

Screenshot of the exception page before these patches:

image.png (2×3 px, 710 KB)

After the above patches, the HTML comment indicating the exception:
image.png (2×3 px, 679 KB)

The ID is "X6B3kawQBHcAAFcgW8UAAAAI" and I can see it is logged in Logstash exactly the same as before (thanks @Majavah @DannyS712), except with "caught_by: other" (example) instead of "caught_by: entrypoint" (example). Hopefully if we ever have a similar issue in the future, doing it like this should still trigger your alerts or whatever, but without making affected pages completely inaccessible.

DannyS712 added a comment.Nov 2 2020, 9:37 PM
In T267035#6598313, @matmarex wrote:

For future reference:

Screenshot of the exception page before these patches:

image.png (2×3 px, 710 KB)

After the above patches, the HTML comment indicating the exception:
image.png (2×3 px, 679 KB)

The ID is "X6B3kawQBHcAAFcgW8UAAAAI" and I can see it is logged in Logstash exactly the same as before (thanks @Majavah @DannyS712), except with "caught_by: other" (example) instead of "caught_by: entrypoint" (example). Hopefully if we ever have a similar issue in the future, doing it like this should still trigger your alerts or whatever, but without making affected pages completely inaccessible.

I didn't have any alerts - I was just looking through logstash

gerritbot added a comment.Nov 2 2020, 10:03 PM

Change 638154 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Move warnings stuff from CommentItem to ThreadItem

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

matmarex lowered the priority of this task from Unbreak Now! to High.Nov 2 2020, 10:38 PM
matmarex moved this task from Incoming to Ready for Sign Off on the Editing-team (Kanban Board) board.

All fixed

• ppelberg closed this task as Resolved.Nov 3 2020, 2:43 AM
• ppelberg claimed this task.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and 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