Page MenuHomePhabricator

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

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?

Event Timeline

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

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.

@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.

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.

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

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.

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

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.)

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

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

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

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.

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

Change 708791 merged by jenkins-bot:

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

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

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

Change 709062 merged by jenkins-bot:

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

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

Arlolra triaged this task as High priority.
Arlolra moved this task from Needs Triage to Bugs & Crashers on the Parsoid board.

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

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

Change 708618 merged by jenkins-bot:

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

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

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

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.

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