Maniphest T221989

Should Parsoid\Wt2Html\PP\Processors\Linter::getNextMatchingNode() be testing nextNonSepSibling() rather than nextSibling?
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Anomie
	Apr 26 2019, 7:24 PM

Description

See code review at https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/505778/6/src/Wt2Html/PP/Processors/Linter.php#136

		if ( $node->nextSibling ) { // TODO: Should this be testing DOMUtils::nextNonSepSibling() instead?
			return $this->leftMostDescendent( DOMUtils::nextNonSepSibling( $node ), $match );
		}
		return $this->getNextMatchingNode( $node->parentNode, $match );

Quoting @ssastry,

Good question. As it stands, if nextSibling is a separator elt, getNextMatchingNode would return null instead of going up the parent and looking there. Probably an edge case. But, worth fixing later. Maybe file a phab task for this?

Details

	Subject	Repo	Branch	Lines +/-
	Bump Parsoid to 0.12.0-a9	mediawiki/vendor	master	+240 -187
	Linter: Fix edge case in misnested-tag lint detection	mediawiki/services/parsoid	master	+10 -1

Customize query in gerrit

Event Timeline

Anomie created this task.Apr 26 2019, 7:24 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 26 2019, 7:24 PM

ssastry triaged this task as Medium priority.May 14 2019, 4:24 AM

ssastry edited projects, added Parsoid-Linter; removed Parsoid-PHP.

Aklapper edited projects, added Parsoid; removed Parsoid-Linter.Feb 29 2020, 5:13 PM

ssastry moved this task from Needs Triage to Needs Investigation on the Parsoid board.Feb 29 2020, 6:03 PM

ssastry claimed this task.Mar 24 2020, 4:34 AM

So, it was obvious that for reasons of code clarity and consistency, we should use DOMUtils::nextNonSepSibling($node) there, but I was curious if it actually led to poorer linting results which is why I punted on this originally when this first showed up during code review. It took a little effort cleaning up code, document, unbreaking some tools and debugging utilities and then some tinkering before I arrived at a test case (edge case of course) whose linter output is improved by the change. Patch coming shortly.

<div>
* <span>foo

</div>
</span>y

Without the change, Linter reports this as 2 lint errors (missing-end-tag + stripped-tag) which is usually an indication that it is a misnested tag. With the change, Linter now reports it as a misnested-tag lint error which is better.

ssastry lowered the priority of this task from Medium to Low.Apr 3 2020, 9:52 PM

ssastry moved this task from Needs Investigation to Bugs & Crashers on the Parsoid board.

Change 585828 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] Linter: Fix edge case in misnested-tag lint detection

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

gerritbot added a project: Patch-For-Review.Apr 3 2020, 9:59 PM

ssastry closed this task as Resolved.Apr 3 2020, 10:31 PM

Change 585828 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Linter: Fix edge case in misnested-tag lint detection

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

Maintenance_bot removed a project: Patch-For-Review.Apr 3 2020, 11:10 PM

Change 585888 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a9

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

gerritbot added a project: Patch-For-Review.Apr 4 2020, 2:59 PM

Change 585888 merged by jenkins-bot:
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a9

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

Maintenance_bot removed a project: Patch-For-Review.Apr 4 2020, 4:10 PM

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:43 PM

Should Parsoid\Wt2Html\PP\Processors\Linter::getNextMatchingNode() be testing nextNonSepSibling() rather than nextSibling?Closed, ResolvedPublicActions

Description

Details

Event Timeline

Should Parsoid\Wt2Html\PP\Processors\Linter::getNextMatchingNode() be testing nextNonSepSibling() rather than nextSibling?
Closed, ResolvedPublic
Actions