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 3058052c756ac7b69ead21b4b237a1cd6714de8a 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 --normalize=parsoid

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.

This is consistent between Parsoid and PHP:

$ echo '[[[Main Page]]]' | bin/parse.js --normalize
<p>[[[Main Page]]]</p>

-vs-

$ echo '[[[Main Page]]]' | php maintenance/parse.php 
<p>[[[Main Page]]]
</p>

I was wrong, Preprocessor_DOM is a lie. At least the "DOM" part of it is -- it just runs DOMDocument::loadXML to wrap a thin veneer of DOMness around the result at the end. Furthermore, it has issues (like T216664) and the DOM extension has other further issues (T215000). Let's just kill it and keep Preprocessor_Hash instead.

Alternatively we should use Preprocessor_Hash on both HHVM and PHP 7 and deprecate/remove Preprocessor_DOM (see T204945: Deprecate one of the Preprocessor implementations for 1.33).

Node counting was added to Preprocessor_DOM in 2caa7829fcc6a0ab45f91c4346c0d5a9100ef4dc by @tstarling. It's not strictly needed in Preprocessor_Hash (as I understand it) because libdom allocates memory in a different way than used by Preprocessor_Hash, but perhaps it should be implemented for consistency -- so that we don't get pages which are only editable if you use Preprocessor_Hash not Preprocessor_DOM.

We definitely want strategy 1, at least long-term. The mb_* functions all take O(length of string) time, since you need to scan the string from the beginning in order to count codepoints.

Seems like we should probably make a plan to proactively manage Unicode transitions on our three platforms (browsers, PHP, server-side JS).

Interesting. Probably not actually related to PHP 7 but to Preprocessor_DOM -- I believe HHVM still runs Preprocessor_Hash. I've got an outstanding request to reduce the code duplication: T204945: Deprecate one of the Preprocessor implementations for 1.33.

Yep, most likely related. @Arlolra will probably write you a patch once he wakes up/gets online this am, but if you're impatient just removing the newlines from the indicated places in parserTests.txt will get you going again.

It would be a good idea to add some parser tests which exercise both modes of StringUtils::explode. If this hadn't thrown an exception for being not Countable, we probably wouldn't have noticed that the array keys shift on large articles until it made production.

Because the type hint just below it will say ?string not string|null? We should be using the one markup language which is used by PHP, not making up our own?

ExplodeIterator::key() returns $this->curPos, *not* the line number. So with the 497863 patch it fixes the exceptions (and so should be fine to backport to wmf.22) but for wikitexts over a thousand lines it will effectively "never always be on the last line" and so you'll get an extra trailing newline.

See also T218816: MediaWiki.Commenting.FunctionComment.DefaultNullTypeParam wants null even if type is nullable.

Seems to be fixed. Watching builds of https://gerrit.wikimedia.org/r/464096 https://gerrit.wikimedia.org/r/497471 and https://gerrit.wikimedia.org/r/497320 to confirm.

Yes, the intention is certainly that title (in normalized form, ie spaces converted) can always be derived by removing the relative path. Furthermore, in modern Parsoid (not historically, but we can ignore that) that relative path is *always* ./. So just strip the first two characters and you've got your title.

See https://gerrit.wikimedia.org/r/#/q/topic:trail+(status:open+OR+status:merged) for the set of patches merged; scribunto probably needs a patch somewhat like https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ParserFunctions/+/494939/

Yeah, we should just fix the scributo tests, not revert patches. This was a set of 9 or so dependent patches to merge, unrolling would be quite a chore, and the scribunto fix should just be to remove some trailing newlines from the parser tests....

There are a few patches which have actually (apparently) passed this test and gotten merged, eg https://gerrit.wikimedia.org/r/496080. But only about two in the past hour AFAICT. So that's UBN territory for me...

Yeah, there's a more-or-less standard-but-ugly workaround that involves using mb_encode to replace everything above U+007F with an HTML entity: https://github.com/wikimedia/html-formatter/blob/5e33e3bbb327b3e0d685cc595837ccb024b72f57/src/HtmlFormatter.php#L71

I'd say there's one other use case, and it's what tidy does (AIUI): mutate a string representation of a HTML document in a "safe" way, without every building the complete DOM tree in memory. That is, "safe" string-to-string transformations. There are probably lots of weird things you could do here, but I would love to see a basic "insert X into Y" (like innerHTML) or "append X to Y" utility, done in a safe way that respected tag boundaries etc. The API of https://github.com/wikimedia/html-formatter/blob/master/src/HtmlFormatter.php could be a guide, just imagine doing it string-to-string without creating an intermediate DOM.

We don't use this (I don't think) but be aware that if you create an attribute named 'xmlns' in PHP's DOM everything breaks: https://marc.info/?l=php-internals&m=155249142123136&w=2

A performance update is at T204595#5024206

In general I would be happy to see an interface that allowed components to pass around DOM subtrees instead of strings, stringifying the tree only where needed for a legacy API. Then "composition" is (eventually) just subtree assembly, and we don't have to worry about poorly-constructed components leaking open tags into the rest of the content...

Testing with the following script (from the zest library home dir) with xdebug off:

$ psysh 
Psy Shell v0.9.9 (PHP 7.3.2-3 — cli) by Justin Hileman
>>> require 'vendor/autoload.php';
=> Composer\Autoload\ClassLoader {#2}
>>> require('./tests/ZestTest.php');
=> 1
>>> $html100 = file_get_contents('./obama.html'); strlen($html100);                       => 2592386
>>> timeit -n10 \Wikimedia\Zest\Tests\ZestTest::parseHTML($html100, [ 'suppressHtmlNamespace' => true, 'ignoreErrors' => true ]) && true;
=> true
Command took 0.389295 seconds on average (0.376101 median; 3.892954 total) to complete.

In T204595#4615610, @cscott wrote:

Could you post your test scripts somewhere? To be fair we should probably factor out process startup and file read times out of the measurements (the 350ms overhead you're measuring). It seems like we should dig into the slow Remex performance on large documents more, though, to figure out if there are some O(N^2) tree-mutation algorithms we need to kill, and if so figure out how hard they will be to fix (ie, are the bugs in Remex, in the PHP DOM extension).