As we start porting more and more components and want to run large scale tests against production pages, we will run into this requirement quickly since we don't yet have a way to test it with core. So, good to get this done sooner than later.

This functionality has now been merged into the Parsoid codebase and we are starting to use it. Thanks @Tgr and @cscott. Exposing this to user outside Parsoid would be a separate task.

We are targeting 7.2

Thanks! :-)

In T216584#5043984, @mobrovac wrote:

In T216584#5042583, @ssastry wrote:

Google had asked us about what dsr information means because they were using it for some reason and I added https://www.mediawiki.org/wiki/Parsoid/Internals/data-parsoid to document it with lots of caveats that we will and can change internals without warning. But worth at least giving them a heads up by tagging the relevant phab accounts on this ticket.

Mind tagging them @ssastry ? Thnx!

In T213493#5043958, @Dzahn wrote:

@ssastry app servers like mwdebug have 7.0 and 7.2 but looking at a prod parsoid server like wtp1025 there is also 7.0 there. It should match wtp servers, right?

In T213493#5043882, @ssastry wrote:

Oh 7.0? Isn't production on PHP 7.1 / 7.2?

Oh 7.0? Isn't production on PHP 7.1 / 7.2?

Google had asked us about what dsr information means because they were using it for some reason and I added https://www.mediawiki.org/wiki/Parsoid/Internals/data-parsoid to document it with lots of caveats that we will and can change internals without warning. But worth at least giving them a heads up by tagging the relevant phab accounts on this ticket.

In T114445#5041285, @Krinkle wrote:

Moving to backlog as current status is unclear.

If the RFC has a clear desired outcome or problem statement, and resourcing commitment from a team that is interested in wider feedback, input or approval, then move it to the Inbox to let TechCom know :)

Thanks! So, I am going to mark this stalled. But, if priorities change and you need this sooner, please update this ticket accordingly.

In T218358#5034306, @Niedzielski wrote:

@ssastry, this task is about improving the current implementation in Popups and now Minerva which have JavaScript client-side implementations. As far as I know, both use the PHP parser presently and there's no rush to replace the client-side hacks. I just wanted to track the work since 1) @Krinkle identified this improvement in code review and 2) dropping the client side implementations would hopefully drop a bunch of "guesswork" code.

In T218358#5034021, @Niedzielski wrote:

My understanding is that if the link is marked with mw:wikilink you can just infer the title from the relative path. Is there a case where this wouldn't work?
VE does this here: https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/8846e65e4475fb1942e2d30478d072b5261cd4a7/modules/ve-mw/ve.MWutils.js#L163

I think this would cover the use case in Popups and Minerva. @ssastry, can you confirm that using mw:wikilink anchors with @Esanders' regular expression is safe?

We are right now heads down in porting Parsoid to PHP and would prefer not to undertake any development on the JS codebase unrelated to the porting itself. So, clarity around priority would be helpful for us to figure out how and when to engage with this request.

This is flagged for editors via the https://www.mediawiki.org/wiki/Help:Extension:Linter/unclosed-quotes-in-heading category.

Looks like our numbers are now more in sync after you disabled xdebug. But, here is the script I used.

See https://www.mediawiki.org/wiki/Topic:Uueiraicppplzvm6 and T194872: Linter : have correct counters for categories populated with only a few errors (or none)

@Pchelolo is T171788 applicable to otrswki?

That is correct. This can be closed and discussion can continue on that wiki page.

@Adithyak1997 I already replied on that wiki page where you asked the question.

The discussion on this topic: https://www.mediawiki.org/wiki/Topic:Uueiraicppplzvm6 might help clarify.

In T217850#5013240, @ssastry wrote:

In T217850#5013208, @ssastry wrote:

More specifically, it seems to be tied to 'skipPreProcess' => true. I haven't looked at why \r handling varies on the presence of this option (i.e. if this is a bug or a feature).

Feature.

Docs and the normalization performed in code.

In T217850#5013208, @ssastry wrote:

More specifically, it seems to be tied to 'skipPreProcess' => true. I haven't looked at why \r handling varies on the presence of this option (i.e. if this is a bug or a feature).

I was trying to figure out the reason for the difference reported in failing test #1 in https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/494253/10//COMMIT_MSG.
I suspected one of (a) file reading (b) Remex parsing (c) XML Serializer.

All this speaks for us all to consolidate behind a common solution for HTML parsing and DOM manipulation on the PHP side. That could be Remex + the DOMCompat code that is currently in review ( https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/491892 ). One of the original suggestions @cscott made was to release this dom compat code as a composer lib but we just went with it being a Parsoid-internal compat layer for simplicity. But, if there is broader immediate interest, perhaps we could make it a composer lib which everyone can build upon.

Bonus points: sync file generation and options between transformTests and domTests .... i.e. both should probably take a common output directory flag and compute the file name from other options (prefix, page, transformer name).

In T213493#4871551, @Dzahn wrote:

Hi @ssastry We already have ticket T201366 to setup scandium as the replacement for ruthenium. That includes a jessie to stretch upgrade which means PHP7.0, 7.1 or 7.2 could be installed.

In T217766#5006890, @Catrope wrote:

This is the consequence of a batshit insane bug in PHP's DOMDocument. It'll drop all entity encoding if your attribute value contains something that looks like an HTML comment:

A wrapper 'env' class around the different configs (site / wiki, parsoid, page) might still be useful so we can pass one object everywhere where needed.

Alright, I'll make this decision since it seems we have been going round and round a bit.

In T215000#4997770, @ssastry wrote:

@Tgr, @Smalyshev, @ori but you cannot guarantee that all references to the deleted node will be removed before you use findElementById. So, this still is not a solution.

@Tgr, @Smalyshev, @ori but you cannot guarantee that all references to the deleted node will be removed before you use findElementById. So, this still is not a solution.

<?php

In T215000#4994273, @Tgr wrote:

I guess the options are:

1. wrap getElementById to walk up the parents and see if it is part of the document
   
   6 seems the least painful, as long as getElementById is not broken in the other direction (where it doesn't return elements it should). Seems like ID handling is broken in more ways than one, though: https://3v4l.org/pi599

In T215000#4993197, @Tgr wrote:

...actually not, since it also applies to descendants of the removed node (and they'd have to be fixed individually). Filed as https://bugs.php.net/bug.php?id=77686

In T215000#4993172, @Tgr wrote:

New sadness: https://3v4l.org/UQYTG
Apparently the id => element map is not always updated when the DOM changes.

In T215002#4987086, @matmarex wrote:

But you do not need <br> in wikitext to emit empty paragraphs in HTML? You just need newlines. I don’t understand why the <br> tags end up in wikitext.

In T215002#4986367, @matmarex wrote:

I know about that task (I even linked it here). There should be <br /> generated in the HTML output for multiple newlines in wikitext. There must not be any in the wikitext output for multiple empty paragraphs in HTML, though.

In T215002#4986294, @matmarex wrote:

It's reproducible. I just added an empty paragraph between the lead and section heading in this edit: https://en.wikipedia.org/w/index.php?title=User:Matma_Rex/sandbox&diff=885235533&oldid=885235196&diffmode=source

There should be no <br /> in the wikitext output. Only a bunch of newline characters.

In T215002#4948631, @Esanders wrote:

CC @ssastry does this look like a Parsoid issue?

@Esanders thoughts?

In T217093#4982822, @Arlolra wrote:

In any case, @cscott, this is a testing gap currently.

Lots of tests mocha were introduced in,
https://github.com/wikimedia/parsoid/commit/4f298233cc95b823c34d605a08f109cf5b1a4157

but it looks like this only would have been caught if the DOMTraverser was given something with encapsulation,
https://github.com/wikimedia/parsoid/blob/master/lib/utils/DOMTraverser.js#L122

So, our pre-deploy testing has a hole for lang variants since rt testing doesn't cover this feature. Presumably, this would have been caught on beta cluster but that is currently readonly. In any case, @cscott, this is a testing gap currently.

In T213228#4982228, @Anomie wrote:

In T213228#4979188, @ssastry wrote:

• No need to use promises since all requests will be synchronously satisfied. Is there a reason you see for using promises for potential future async modes in PHP?

The only reason I asked about promises was because not using promises will result in structural changes to the calling code, hopefully just directly calling the result-processing code instead of passing it as a callback to .then(). I wasn't sure of the extent to which we wanted to do that during the port.

A few comments:

• No need to use promises since all requests will be synchronously satisfied. Is there a reason you see for using promises for potential future async modes in PHP?
• Currently, usePHPPreProcessor is false for parser test runs in Parsoid. It uses the Parsoid-native template expansion and parser functions code. That has always existed since the beginning, but had too many edge case bugs and performance wasn't considered good enough to actually develop it further. But, using mediawiki api for parser tests runs would have been excessive given how frequently they run and it would have slowed down parser test runs greatly. But, the qn of what to do now with integration with Parsoid remains. I don't see a reason to not use the same core preprocessing code for everything (including parser tests).
• We need memoization support for sure for perf reasons. But, we probably do not need batching of template and extension requests any more.
• But, we do need batch query support to avoid hitting the db for every single image/link. The "redlinks" and "mediainfo" dom passes make these bulk metadata requests. So, we need support for these bulk/batch requests.
• As for linting, yes, you could use any existing interfaces provided by SiteConfig.

In T215000#4973529, @Anomie wrote:

In T215000#4966839, @Tgr wrote:

• attribute name with or without value (with quotes, possibly with case insensitive flag), = / ~=)

If Parsoid needs case-insensitive attribute selectors, and we use css-sanitizer for parsing as you did in your patch, then we'll need to update css-sanitizer to support Selectors Level 4 (it currently only supports Level 3).

We should probably do that anyway at some point, since https://caniuse.com/#feat=css-case-insensitive shows decent browser support. This would just raise the priority of doing that.

In T214099#4970637, @Pchelolo wrote:

Parsoid was in a bit of trouble again today. At 02:44 XioNoX: depool eqsin, so all the mobile traffic for Chinese wiki started hitting RESTBase. Since Chinese has variants, RB started requesting Parsoid to transform HTML into correct variant. As the transformation request rate reached roughly 40 r/s, Parsoid started experiencing troubles, alerting and timing out.

I would like to prioritize this issue, even though node Parsoid is being replaced, depooling an edge CDN DC should not be able to bring down our infrastructure. Also, 40 r/s for lang variant transforms seem quite low to affect Parsoid cluster so drastically. I believe there's a bug somewhere.

In T215000#4969279, @Tgr wrote:

It would. Object creation is pretty cheap in PHP though.

In T215000#4967484, @Tgr wrote:

The syntax I'm playing with is along the lines of DomCompat::wrap( $node )->remove(). (Properties would have to be replaced by getters/setters since property getters/setters are a mess in PHP: DomCompat::wrap( $node )->getBody() etc.) That makes the syntax easier to remember/read than static methods.

In T215000#4965052, @cscott wrote:

@subbu it's hard to grep for add! These are generally manipulations of Element#classList. See git grep classList which probably pulls up most of them.

In T215000#4958564, @cscott wrote:

Here's an inventory of all domino top-level methods (that is, not those called internally w/in the domino implementation) invoked by Parsoid when running npm run parserTests. The first column is the number of dynamic invocations, which isn't so interesting. What would be more interesting is the number of different callsites within Parsoid; I'll try to generate that next.

I've added a third column which lists the equivalent PHP function, if any.

25500	DOMTokenList#add	-
14375	DOMTokenList#contains	-
8028	DOMTokenList#length[get]	-
8	DOMTokenList#remove	-

Yes.