Page MenuHomePhabricator

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

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

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 changed the subtype of this task from "Task" to "Bug Report".

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

thcipriani triaged this task as Unbreak Now! priority.Nov 2 2020, 6:48 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?

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

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

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.

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

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

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

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

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

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.

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

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

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

ppelberg claimed this task.