Page MenuHomePhabricator
Create Task
Maniphest T287611

TypeError: Argument 1 passed to Wikimedia\Parsoid\Utils\DOMCompat::getOuterHTML() must be an instance of Wikimedia\Parsoid\DOM\Compat\Element, instance of DOMElement given
Closed, ResolvedPublic
Actions

Description

On https://patchdemo.wmflabs.org/wikis/014e812f6d/wiki/Talk:New_section (demo for T282204), we noticed errors like this when trying to post a new topic, or viewing the page with topic subscriptions enabled:

[e3c6d34443edfd248060b0ab] /wikis/014e812f6d/wiki/Talk:New_section TypeError: Argument 1 passed to Wikimedia\Parsoid\Utils\DOMCompat::getOuterHTML() must be an instance of Wikimedia\Parsoid\DOM\Compat\Element, instance of DOMElement given, called in /srv/patchdemo-wikis/014e812f6d/w/extensions/DiscussionTools/includes/CommentFormatter.php on line 275

Backtrace:

from /srv/patchdemo-wikis/014e812f6d/w/parsoid/src/Utils/DOMCompat.php(386)
#0 /srv/patchdemo-wikis/014e812f6d/w/extensions/DiscussionTools/includes/CommentFormatter.php(275): Wikimedia\Parsoid\Utils\DOMCompat::getOuterHTML(DOMElement)
#1 [internal function]: MediaWiki\Extension\DiscussionTools\CommentFormatter::MediaWiki\Extension\DiscussionTools\{closure}(array)
#2 /srv/patchdemo-wikis/014e812f6d/w/extensions/DiscussionTools/includes/CommentFormatter.php(231): preg_replace_callback(string, Closure, string)
#3 /srv/patchdemo-wikis/014e812f6d/w/extensions/DiscussionTools/includes/Hooks/PageHooks.php(157): MediaWiki\Extension\DiscussionTools\CommentFormatter::postprocessTopicSubscription(string, LanguageEn, MediaWiki\Extension\DiscussionTools\SubscriptionStore, User)
#4 /srv/patchdemo-wikis/014e812f6d/w/includes/HookContainer/HookContainer.php(160): MediaWiki\Extension\DiscussionTools\Hooks\PageHooks->onOutputPageBeforeHTML(OutputPage, string)
#5 /srv/patchdemo-wikis/014e812f6d/w/includes/HookContainer/HookRunner.php(2686): MediaWiki\HookContainer\HookContainer->run(string, array)
#6 /srv/patchdemo-wikis/014e812f6d/w/includes/OutputPage.php(2017): MediaWiki\HookContainer\HookRunner->onOutputPageBeforeHTML(OutputPage, string)
#7 /srv/patchdemo-wikis/014e812f6d/w/includes/OutputPage.php(2029): OutputPage->addParserOutputText(ParserOutput, array)
#8 /srv/patchdemo-wikis/014e812f6d/w/includes/page/Article.php(843): OutputPage->addParserOutput(ParserOutput, array)
#9 /srv/patchdemo-wikis/014e812f6d/w/includes/page/Article.php(751): Article->doOutputFromRenderStatus(MediaWiki\Revision\RevisionStoreRecord, Status, OutputPage, array)
#10 /srv/patchdemo-wikis/014e812f6d/w/includes/page/Article.php(559): Article->generateContentOutput(User, ParserOptions, integer, OutputPage, array)
#11 /srv/patchdemo-wikis/014e812f6d/w/includes/actions/ViewAction.php(74): Article->view()
#12 /srv/patchdemo-wikis/014e812f6d/w/includes/MediaWiki.php(538): ViewAction->show()
#13 /srv/patchdemo-wikis/014e812f6d/w/includes/MediaWiki.php(320): MediaWiki->performAction(Article, Title)
#14 /srv/patchdemo-wikis/014e812f6d/w/includes/MediaWiki.php(925): MediaWiki->performRequest()
#15 /srv/patchdemo-wikis/014e812f6d/w/includes/MediaWiki.php(559): MediaWiki->main()
#16 /srv/patchdemo-wikis/014e812f6d/w/index.php(53): MediaWiki->run()
#17 /srv/patchdemo-wikis/014e812f6d/w/index.php(46): wfIndexMain()
#18 {main}

It looks like commit rGPARbcaca588336b: Add class alias file to allow swapping in Dodo for DOMDocument changed type hints in an incompatible way. The errors will probably soon also occur on beta cluster and production? What should we do to fix this?

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Don't strictly enforce type hints in DOMCompat methodsmediawiki/services/parsoidnododo-a10+35 -34
Don't refer directly to PHP `dom` extension classes; avoid nonstandard behaviormediawiki/extensions/DiscussionToolsmaster+263 -251
Use DOMCompat::newDocument() to create a new Documentmediawiki/extensions/DiscussionToolsmaster+29 -5
WIP: Don't refer directly to PHP `dom` extension classesmediawiki/extensions/DiscussionToolsmaster+211 -211
Bump wikimedia/parsoid to 0.14.0-a12mediawiki/vendormaster+163 K -2 K
Bump wikimedia/parsoid to 0.14.0-a11mediawiki/vendormaster+144 K -2 K
Don't strictly enforce type hints in DOMCompat methodsmediawiki/services/parsoidmaster+82 -31
Customize query in gerrit

Related Objects

Event Timeline

matmarex created this task.Jul 28 2021, 5:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 28 2021, 5:46 PM
matmarex mentioned this in T282204: Handle URLs for starting new sections.Jul 28 2021, 5:48 PM
ssastry subscribed.Jul 28 2021, 5:54 PM

This is not on production yet. So, all good there. But yes will impact beta cluster. Will make sure @cscott sees this.

ppelberg added a project: Editing-team (Tracking).Jul 28 2021, 6:00 PM
ppelberg subscribed.
ssastry added a comment.Jul 28 2021, 6:13 PM

But, looks like if we are going to down this class alias route to support Dodo, we will need to figure out how to address non-Parsoid-repo uses of DOMCompat class from Parsoid's repo.

ppelberg added a comment.Jul 28 2021, 6:30 PM

@ssastry: what do you think is a good way to explicitly communicate this task as blocking the release of the next version of Parsoid? [i]

...after quickly scanning https://phabricator.wikimedia.org/project/view/4519/, I didn't see a ticket to relate this one (T287611) to.

i. I ask this assuming: 1) we agree this issue is high priority and 2) this issue would only impact people were it not to be resolved before the next version of Parsoid is resleased.

ppelberg moved this task from Backlog to Dependent on others on the Editing-team (Tracking) board.Jul 28 2021, 6:31 PM
cscott added a comment.Jul 28 2021, 6:41 PM

Seems like maybe this is an isolated issue with discussion tools calling parsoid methods with documents that didn't come from parsoid. I'll look into this, code search will.probably help.

The issue about properly tagging release blocker bugs in parsoid is a separate one. Our usual rule used to be "don't release parsoid unless beta is clear" and this seems like it would fall under that.

cscott added a comment.Jul 28 2021, 6:43 PM

(also Dom element types shouldn't be used in PHP type hints, so that's a bug in parsoid.)

ppelberg added a comment.Jul 28 2021, 7:18 PM
In T287611#7244221, @cscott wrote:

The issue about properly tagging release blocker bugs in parsoid is a separate one. Our usual rule used to be "don't release parsoid unless beta is clear" and this seems like it would fall under that.

Understood - thank you for explaining, @cscott. We'll then assume this ticket will make it into y'alls workflow in a way that matches up with what you've stated above and leave it at that.

gerritbot added a comment.Jul 28 2021, 9:06 PM

Change 708591 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] Don't strictly enforce type hints in DOMCompat methods

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

gerritbot added a project: Patch-For-Review.Jul 28 2021, 9:06 PM
cscott added a comment.Jul 28 2021, 9:08 PM

There will be another follow up to have DiscussionTools use the appropriate DOMCompat methods to create and load a Document object, which will address the root cause both ways. (In the future if/when Parsoid switches to Dodo it will be more important to ensure that any clients use the same DOM implementation as Parsoid does.)

gerritbot added a comment.Jul 28 2021, 10:58 PM

Change 708591 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Don't strictly enforce type hints in DOMCompat methods

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

Maintenance_bot removed a project: Patch-For-Review.Jul 28 2021, 11:10 PM
gerritbot added a comment.Jul 29 2021, 2:19 AM

Change 708618 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/DiscussionTools@master] Use DOMCompat::newDocument() to create a new Document

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

gerritbot added a project: Patch-For-Review.Jul 29 2021, 2:19 AM

Change 708619 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/DiscussionTools@master] WIP: Don't refer directly to PHP `dom` extension classes

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

cscott added a comment.Jul 29 2021, 2:15 PM

https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/708591 should have fixed the problem, but I think we need to deploy that to mediawiki-vendor before it will actually fix beta and the patchdemo. Once it is deployed to mediawiki-vendor the related patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/708618 should pass gerrit, and when merged that will also help in a belt-and-suspenders sort of way.

ppelberg moved this task from Backlog to Triaged on the DiscussionTools board.Jul 29 2021, 3:52 PM
gerritbot added a comment.Jul 29 2021, 8:11 PM

Change 708791 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.14.0-a11

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

gerritbot added a comment.Jul 29 2021, 8:40 PM

Change 708791 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.14.0-a11

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

gerritbot added a comment.Jul 30 2021, 2:21 PM

Change 709062 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.14.0-a12

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

gerritbot added a comment.Jul 30 2021, 2:46 PM

Change 709062 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.14.0-a12

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.17; 2021-08-02).Jul 30 2021, 3:00 PM
Arlolra assigned this task to cscott.Jul 30 2021, 3:01 PM
Arlolra triaged this task as High priority.
Arlolra moved this task from Needs Triage to Bugs & Crashers on the Parsoid board.
gerritbot added a comment.Jul 30 2021, 9:22 PM

Change 708619 abandoned by C. Scott Ananian:

[mediawiki/extensions/DiscussionTools@master] WIP: Don't refer directly to PHP `dom` extension classes

Reason:

Abandoned in favor of I3c4f41c3819770f85d68157c9f690d650b7266a3

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

gerritbot added a comment.Jul 30 2021, 9:28 PM

Change 709061 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/DiscussionTools@master] WIP: Don't refer directly to PHP `dom` extension classes

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

Krinkle merged a task: T287791: Internal error when trying to a load page on beta-cluster.Jul 30 2021, 10:07 PM
Krinkle added a project: Wikimedia-production-error.
Krinkle moved this task from Untriaged to July 2021 on the Wikimedia-production-error board.
Krinkle added subscribers: Bas_dehaan, Zabe, Metaverse.
cscott added a comment.Jul 30 2021, 11:03 PM

Should be fixed by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/708618 (which hasn't been merged yet).
(in particular the tweak to postprocessTopicSubscription in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/708618/9/includes/CommentFormatter.php which is implicated in the backtrace in T287791.)

gerritbot added a comment.Aug 1 2021, 9:31 AM

Change 708618 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Use DOMCompat::newDocument() to create a new Document

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

gerritbot added a comment.Aug 1 2021, 10:09 AM

Change 709061 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Don't refer directly to PHP `dom` extension classes; avoid nonstandard behavior

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

cscott added a comment.Aug 2 2021, 2:36 PM

Tested the request URL from T287791 and it seems to be fixed now. The patchdemo site needs to be updated before it will be fixed.

matmarex edited projects, added Editing-team (Kanban Board); removed Editing-team (Tracking).Aug 2 2021, 5:34 PM
matmarex moved this task from Inbox to Ready for Sign Off on the Editing-team (Kanban Board) board.
matmarex edited projects, added Skipped QA; removed Patch-For-Review.

@cscott and I have tested it.

gerritbot added a comment.Aug 2 2021, 7:18 PM

Change 709529 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@nododo-a10] Don't strictly enforce type hints in DOMCompat methods

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

gerritbot added a project: Patch-For-Review.Aug 2 2021, 7:18 PM
ppelberg closed this task as Resolved.Aug 3 2021, 7:49 PM
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 · Credits