I find it puzzling that "we know they cannot". We know for a fact that they are using a special pipeline for handling wiki content, and we have worked with them to build it. We can see their User-Agent in our logs ("MediaWikiCrawler-Google/2.0"). I've talked to Google engineers personally. They come to Wikimania. I can understand that Google is a big company, and sometimes it can be hard to find the right person at Google who actually knows how things work. But talking to the wrong person at Google isn't proof of anything.

We should update our developer docs as well to reflect best practices to installing Parsoid & running PHP tests. Should Parsoid be installed inside core/extensions/Parsoid and/or is setting $MW_INSTALL_PATH sufficient? Ideally composer extension-test or something would Do The Right Thing assuming proper set up (whether that's installing Parsoid inside the extension folder or setting MW_INSTALL_PATH or whatever else the Right Thing needs to be).

Looks like the DSR offsets are getting scrambled and/or the base text is changing, and selser is substituting regions from a different wikitext and/or from an offset region. Seems to require that a parameter of the ref is changed, so that selser isn't skipping over the entire ref but is instead trying to do a finer-grainer selective serialization of each parameter.

Right, I'm proposing to make it do that (if it doesn't already). As far as I know, extension tag parsing in wikitext (as opposed to HTML) should allow self-closed tags anywhere and treat it completely equivalently to an open/close tag pair with a zero-length content, so there shouldn't be any problem with always emitting self-closed tags when the content is empty.

The general practice seems to be that if the content is zero-length, then a self-closed tag would be generated. Would that be sufficient? I don't think we need to add an extra parameter. (What if you wanted to define an attribute named 'selfclose'?)

With gerrit 520293 the time taken drops from:

83.21user 267.41system 5:52.38elapsed

to

6.60user 0.08system 0:06.72elapsed

Adding gerrit 520294 on top further reduces runtime to

6.02user 0.06system 0:06.11elapsed

on this zh.wikisource page.

Wikitext source, for reference:

Another difference we found: PHP's Node::normalize() doesn't actually remove zero-length text nodes, like the spec says it should (DOM level 2: https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-normalize ; latest DOM spec: https://dom.spec.whatwg.org/#dom-node-normalize ).

I'm closing this bug since https://gerrit.wikimedia.org/r/502567 was merged. Technically, we ought to change the shipping defaults to make our production configuration, but we can handle that over in T204945.

I updated the patch ( https://gerrit.wikimedia.org/r/502578 ) to resolve merge conflicts since it was originally written for 1.33. Would be a good idea to deploy this (or something like it) since WMF is running Preprocessor_Hash on PHP7 now but our official release still defaults to Preprocessor_DOM on that platform.

Yes, now that https://gerrit.wikimedia.org/r/502567 has been merged, this task is no longer stalled.

This *might* be related to Parsoid's slightly-incorrect handling of initial newlines in wikitext (T175421: Parsoid doesn't insert same spacers when article text starts with two or more newlines.) -- I think the "insert leading newline if the contents begin with *" part is actually done by the preprocessor, and so the result Parsoid gets back from the legacy preprocessor should have this hack (T14974, T2529) already applied.

The bug here is related to the semantics of image options where "anything that doesn't look like a valid option is treated as a caption". Ie:

[[File:Foo.jpg|right|Caption|200px]]

Has the caption Caption because 200px looks like an image option. But:

[[File:Foo.jpg|right|Caption|x200px]]

...has the caption x200px.

Hm! Turns out that we've been worrying about the complex caching semantics required of RESTbase during edits, while all along merrily ignoring caching altogether....

js2php's regexps are correct, it is just a bit conservative about escaping $ in double-quoted strings. It doesn't technically need the backslash escape so long as the next character isn't a valid PHP identifier start or {. I can tweak js2php, but there's no bug that needs to be fixed per se.

This is now *mostly* fixed. Getting spurious failures on other jobs, eg ENOMEM on https://integration.wikimedia.org/ci/job/parsoidsvc-npm-run-roundtrip-node-6-docker/5677/console

We could probably also tweak the .phan/config.php to exclude more stuff in vendor/, including phan itself.

See https://www.mediawiki.org/wiki/Manual:How_to_debug#Creating_custom_log_groups -- mediawiki core already has a lot of logging support; hopefully we could reuse this rather than reinvent it.

The PHP parser does it in the preprocessor, and there are edge cases that mean if we want compatibility it's best to continue to do so. I moved it to the tokenizer to fix these edge cases, if we move it back we're going to regress on those.

You're right, never mind. Tag freely!

I have one more patch outstanding I was going to merge ( https://gerrit.wikimedia.org/r/508037 ) but I can do that in a subsequent point release. Note that wikipeg is a dual JS/PHP package now, you should probably release to both npm and composer. That involves bumping the version in package.json before tagging a commit. I can do this Tuesday if that's easier.

I don't think that anyone currently on @Parsing-Team knows anything about the mustache template code. I think all those folk were moved back into Platform.

It would be nice to support proposed changes like this with benchmarks. My strong suspicious is that character class optimizations are already done by the javascript regex runtime, so these changes are unlikely to produce any actual performance improvement.

If you look at Antlr ( https://www.antlr.org/ ) for inspiration, they have instead a single operator which means "return this!".

expansionDepth is a static variable in PPFrame_Hash::expand() -- as such it could be incremented by 1 by some PHP7-specific parse earlier in the process lifetime. For example, in the header which says "this is an experimental service running PHP7" or some other bit of wikitext which appears only on the PHP7 machines. I think that's a red herring.

Chasing it down...
MWTidy::tidy() -> RemexDriver::tidy() creates

		$tokenizer = new Tokenizer( $dispatcher, $text, [
			'ignoreErrors' => true,
			'ignoreCharRefs' => true,
			'ignoreNulls' => true,
			'skipPreprocess' => true,
		] );

and then calls $tokenizer->execute() -- and it appears that the remex patch in question isn't honoring ignoreCharRefs and is decoding the entities regardless (at least in certain situations).

In T222992#5173751, @ssastry wrote:

In T222992#5173749, @cscott wrote:

...though it seems impossible, I've confirmed that 3058052c756ac7b69ead21b4b237a1cd6714de8a in Remex causes these failures. (If only I knew why!)

I cannot find that commit in gerrit or git log. Is that the right hash?

...though it seems impossible, I've confirmed that 7b208b709161456b5a5a255e0bf249e57e2d5c97 in Remex causes these failures. (If only I knew why!)

These failures all have to do with how entities are escaped in attributes, which in theory shouldn't be affected by any of the changes between remex 2.0.1 and 2.0.2, which are about *parsing* (specifically tokenization) not *serialization*.

Yeah, it's interesting that the various ppvisitednodes etc counts confirm that HHVM and PHP seem to be doing exactly the same work; ie, there's no weird PHP-7 behavior which is causing it to generate a subtly different graph. Assuming that the generated objects are exactly the same (which again, the counts seem to confirm), slight representation differences for object *probably* wouldn't account for such a large difference. So here are my two theories:

This computation is done at grammar compile time; it has no effect on runtime (unless I'm missing something).

In T216664#5164591, @Krinkle wrote:

Does this mean HHVM is using less memory for the same task than PHP 7? Or maybe it's measuring/enforcing it differently?

I suspect what is happening is that we are incrementing section ID when we attempt to parse, and then failing to decrement when we backtrack out of the parse.

Reproduced:

$ (echo '==Foo==' ; echo '{{subst: a surname' ; echo '==Bar==') | bin/parse.js --wrapSections --normalize=parsoid
<section data-mw-section-id="0"></section><section data-mw-section-id="1">
<h2 id="Foo">Foo</h2>
<p>{{subst: a surname</p>
</section><section data-mw-section-id="3">
<h2 id="Bar">Bar</h2>
</section>

This is a weird corner case of legacy parser behavior; it's not entirely clear Parsoid should follow it. But it's not clear that what Parsoid is doing is totally consistent either.

It's not either-or -- wikitext like [http://example.com/{{1x|path/to/resource}} blah] is possible too, which your rewrite doesn't capture. We could strip the redundant checks and fall back on the final check which is done after template expansion, but in general we prefer doing the early checks, if they are simple and fast (as the protocol check is), in order to avoid wasting too much time on a potential parse which will ultimately be rejected. The sooner we can reject it the better, hence the three-fold check in the current code.

You've confused the precedence of the / operator a little bit. In fact, the addr can be the empty string "" which is why we need a secondary check. In fact, I believe there's a third-string check after templates are expanded, as well.

Current parsoid output for the test case wikitext above:

$ echo -n | bin/parse.js --pageName User:cscott/T222266
<!DOCTYPE html>
<html prefix="dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/" about="http://en.wikipedia.org/wiki/Special:Redirect/revision/895030248"><head prefix="mwr: http://en.wikipedia.org/wiki/Special:Redirect/"><meta charset="utf-8"/><meta property="mw:pageNamespace" content="2"/><meta property="mw:pageId" content="60633826"/><link rel="dc:replaces" resource="mwr:revision/0"/><meta property="dc:modified" content="2019-05-01T15:20:40.000Z"/><meta property="mw:revisionSHA1" content="68d05a01c3a5c6743d96a854d9754428a3b4fcc1"/><meta property="mw:html:version" content="2.1.0"/><link rel="dc:isVersionOf" href="//en.wikipedia.org/wiki/User%3Acscott/T222266"/><title>User:Cscott/T222266</title><base href="//en.wikipedia.org/wiki/"/><link rel="stylesheet" href="//en.wikipedia.org/w/load.php?modules=mediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.skinning.content.parsoid%7Cmediawiki.skinning.interface%7Cskins.vector.styles%7Csite.styles%7Cext.cite.style%7Cext.cite.styles%7Cmediawiki.page.gallery.styles&amp;only=styles&amp;skin=vector"/><!--[if lt IE 9]><script src="//en.wikipedia.org/w/load.php?modules=html5shiv&amp;only=scripts&amp;skin=vector&amp;sync=1"></script><script>html5.addElements('figure-inline');</script><![endif]--><meta http-equiv="content-language" content="en"/><meta http-equiv="vary" content="Accept"/></head><body data-parsoid='{"dsr":[0,240,0,0]}' lang="en" class="mw-content-ltr sitedir-ltr ltr mw-body-content parsoid-body mediawiki mw-parser-output" dir="ltr"><table data-parsoid='{"dsr":[0,107,2,2]}'>
<tbody data-parsoid='{"dsr":[3,105,0,0]}'><tr data-parsoid='{"autoInsertedStart":true,"dsr":[3,104,0,0]}'><td about="#mwt18" typeof="mw:ExpandedAttrs" data-parsoid='{"a":{"{{Grade II colour}}":null},"sa":{"{{Grade II colour}}":""},"dsr":[3,25,21,0]}' data-mw='{"attribs":[[{"txt":"style=\"background-color: \"","html":"&lt;span about=\"#mwt1\" typeof=\"mw:Transclusion\" data-parsoid=&apos;{\"pi\":[[]],\"dsr\":[4,23,null,null]}&apos; data-mw=&apos;{\"parts\":[{\"template\":{\"target\":{\"wt\":\"Grade II colour\",\"href\":\"./Template:Grade_II_colour\"},\"params\":{},\"i\":0}}]}&apos;>style=\"background-color: &lt;/span>&lt;span typeof=\"mw:Nowiki\" about=\"#mwt1\" data-parsoid=\"{}\">\n#ACE1AF&lt;/span>&lt;span about=\"#mwt1\" data-parsoid=\"{}\">\"&lt;/span>"},{"html":""}]]}'>X</td>
<td style="color:red" about="#mwt4" typeof="mw:ExpandedAttrs" data-parsoid='{"a":{"style":"color:red"},"sa":{"style":"color:&lt;nowiki>red&lt;/nowiki>"},"dsr":[26,63,36,0]}' data-mw='{"attribs":[[{"txt":"style"},{"html":"color:&lt;span typeof=\"mw:Nowiki\" data-parsoid=&apos;{\"dsr\":[40,60,8,9]}&apos;>red&lt;/span>"}]]}'>Y</td>
<td style="color:
green" about="#mwt7" typeof="mw:ExpandedAttrs" data-parsoid='{"a":{"style":"color:\ngreen"},"sa":{"style":"color:&lt;nowiki>\ngreen&lt;/nowiki>"},"dsr":[64,104,39,0]}' data-mw='{"attribs":[[{"txt":"style"},{"html":"color:&lt;span typeof=\"mw:Nowiki\" data-parsoid=&apos;{\"dsr\":[78,101,8,9]}&apos;>\ngreen&lt;/span>"}]]}'>Z</td></tr>
</tbody></table>

The <nowiki> in the style attribute was perhaps an attempt to workaround T5158: Parser inserts invalid &nbsp; in the middle of style attribute (French spaces)/T197902: Be more selective in applying French Space armoring, which was fixed in July 2018?

Yes, indeed. I just finished digging through git log to find that patch, and came here and you'd beaten me to it. ;)

I recall that we did have the issue with Parsoid being exposed to sensitive content, specifically usernames and deleted revisions. The details are foggy, but I vaguely recall that I had to remove some information from the <head> of the Parsoid document because it could expose information about deleted revisions.

@Pols12 that seems like possibly an interaction with french space armoring (T197902).

On the other hand, stating "we follow WHATWG HTML5" implies an intention to follow WHATWG as HTML5 grows/changes. We haven't stated that as an explicit goal in the past, but our community seems to assume it, insofar historically as soon as new elements were added to HTML (<section>, etc) there were folks who wanted to start using them in wikitext. Not every HTML feature intersects with wikitext, but tag and entity names are the most direct points of contact and historically they've tended to follow HTML's evolution closely.

...except for the Japanese ruby issue, where the W3C has philosophical differences which apparently conflict with the desires of jawiki. (I used to actually understand the differences, but I paged all that out long ago.) According to MDN, only firefox implements <rtc>, but all browsers except IE implement <rb>. Of course wikitext doesn't *have* to exactly follow anyone, but it's slightly less confusing to just say "wikitext follows the WHATWG HTML5 spec" than to say "wikitext follows the W3C HTML5 spec, except in the case of <ruby> elements".

This seems pretty low priority; W3C is following the WHATWG spec so there's no real difference. WHATWG tends to lead, and then their decisions are ratified by W3C sometime later.

As a counter-argument: $wgAllowImageTag is not turned on for any WMF production wikis. You could argue that wgAllowImageTag should be deprecated and removed, rather than supported and expanded.

Dup of T127884: Respect $wgAllowImageTag wiki configuration flag in the Sanitizer.

In T118520#3680881, @Arlolra wrote:

The spec has this explanation, https://www.mediawiki.org/wiki/Specs/HTML/1.5.0#Images

The outer <figure> element needs to become a <span> element when the figure is rendered inline, since otherwise the HTML5 parser will interrupt a surrounding block context. The inner <figcaption> element is rendered as a data-mw attribute in this case (since block content in an invisible caption would otherwise break parsing).

So, the difference is in how a browser will render it. Hope that helps.

Adding a few details to @ssastry's update: Parsoid was changed to use <figure> and <figure-inline> in c9f404761cd288e7b58b89623ac459bbb2901a7d (T118520). The remaining work to be done is to transition core to use this same markup. The original plan was to do this in two steps: first convert block markup to use <figure>, and then as a follow-up convert inline markup to use <figure-inline>. Arlo has core patches written (linked above), but actually deploying them will take a careful process of communicating w/ local communities, linting, etc, which we do not plan to tackle until after the Parsoid port to PHP is complete.

In T212124#4858668, @Gilles wrote:

I thought about Parsoid, but first let's see if it's beneficial at all for merely reading articles rendered by MediaWiki on a web browser?

In T221041#5124724, @Tgr wrote:

In my ideal world, both the legacy parser and Parsoid could be extensions implementing a Parser interface -- along with (eventually) perhaps a "wikitext 2.0" parser and even maybe a ReStructuredText or markdown parser or whatever.

Having multiple wikitext parsers and having different parsers for different markups are two entirely different layers of abstraction. The latter is already mostly possible via the content handler mechanism, and the extent to which it is not possible has less to do with the parser and more with baked-in assumptions about certain things (like system messages) always use wikitext. I don't see much overlap between wider support for non-wikitext markup and the Parsoid-PHP project.

Note that we have a similar issue with {{ }}, as shown in our test case for T70421:

$ echo '{{1x|{{ }}}}' | bin/parse.js --wt2wt
{{1x|{{ }<nowiki>}</nowiki>}}

But this doesn't round trip:

$ echo '{{1x|{{ }<nowiki>}</nowiki>}}' | php maintenance/parse.php 
<p>{{1x|{{ }}}}
</p>

Oh, there is one buglet on JS, I wonder if that's how I managed to get this to reproduce:

$ node
> [null, undefined].join(':')
':'
> [null, undefined,32].join(':')
'::32'

That is, null and undefined are treated as interchangeable in the cache key. That's an issue in the PHP port as well:

$ psysh
Psy Shell v0.9.9 (PHP 7.3.3-1 — cli) by Justin Hileman
>>> implode(':', [null, 32, '', false])
=> ":32::"

It would be safer to use json_encode() for PHP, probably, but JSON.stringify doesn't distinguish null and undefined on JS.

No, I'm an idiot. The above is close, but the original value of the variable is in fact stored in the cache key.

As I said over in T110413, .//Foo is pretty dubious as an href. But then again, https://en.wikipedia.org/wiki//e/_(operating_system) exists on enwiki, so I guess that's legit.

.//Foo is pretty dubious as an href. I'd expect ./%2FFoo (if subpages aren't enabled) or ./CurrentPageTitle/Foo (if they are). The base href is always the root of the wiki.

OK, pretty sure I found the bug. In the cache code, we save and restore reference values like so:

			start: [
				`$key = ${key};`,
				'$bucket = $this->currPos;',
				`$cached = $this->cache[$bucket][$key] ?? null;`,
				'if ($cached) {',
				'  $this->currPos = $cached[\'nextPos\'];',
				opts.loadRefs,
				'  return $cached[\'result\'];',
				'}',
				opts.saveRefs,
			].join('\n'),
			store: [
				`$cached = ['nextPos' => $this->currPos, 'result' => ${opts.result}];`,
				opts.storeRefs,
				`$this->cache[$bucket][$key] = $cached;`

Where opts.storeRefs typically looks something like this:

if ($saved_preproc !== $param_preproc) $cached['refs']["preproc"] = $param_preproc;
if ($saved_th !== $param_th) $cached['refs']["th"] = $param_th;

I know @ssastry and I disagree on this, but I'd like for us to move toward a place where Parsoid is an extension. In my ideal world, both the legacy parser and Parsoid could be extensions implementing a Parser interface -- along with (eventually) perhaps a "wikitext 2.0" parser and even maybe a ReStructuredText or markdown parser or whatever. I'd like to reduce the dependencies between mediawiki core and any one particular parser implementation.

I'd recommend merge of 502567 ^. @Krinkle, if you agree C+1 and we can get this scheduled for SWAT?

To be conservative, let's configure it for PHP7 first -- since that's an experimental deployment anyway. Then we can talk about changing the defaults once we are certain that we don't come across anything unexpected in production use. (T216664 was unexpected, for instance, at least to me. I'd think the likelihood of being surprised is even lower with a shift to Preprocessor_Hash since we're already using it in production on HHVM... but that's the thing about unknowns: you never know them in advance...)

I was sort of waiting to see if @tstarling had anything to say about this, as (a) he wrote both Preprocessor_DOM and Preprocessor_Hash (AFAIK) and (b) he was the one objection to the previous plan (deprecating _Hash in favor of _DOM).

In T217867#5089734, @ssastry wrote:

https://www.mail-archive.com/internals@lists.php.net/msg97075.html is also pertinent.