HTML API: Backport updates from Core

[dmsnell]

What?

In this patch updates to the HTML API are being brought back into Gutenberg from WordPress Core.

Notably:

• Small updates to the docblocks.
• A fix for when parsing incomplete tags.
• Introduction of a minimal HTML Processor for semantic HTML querying.

This is a "blessed" PR and the changes have already received code review in Core. What needs review here is the mechanism of back-porting, checking for the right guards on class definitions, etc…

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.4/html-api/class-wp-html-active-formatting-elements.php
❔ lib/compat/wordpress-6.4/html-api/class-wp-html-open-elements.php
❔ lib/compat/wordpress-6.4/html-api/class-wp-html-processor-state.php
❔ lib/compat/wordpress-6.4/html-api/class-wp-html-processor.php
❔ lib/compat/wordpress-6.4/html-api/class-wp-html-token.php
❔ lib/compat/wordpress-6.4/html-api/class-wp-html-unsupported-exception.php
❔ lib/load.php

[dmsnell]

@ockham I'm wondering if you can help me appropriately understand this.

• There's no reason to add these classes into wordpress-6.2 because that will no longer be supported once we start targeting 6.4
• wordpress-6.3 should receive the full class files because they will be absent in 6.3 environments
• wordpress-6.4 should get a Gutenberg_HTML_Processor version of the class for updates that come along in the 6.4 cycle?

[dmsnell]

@hellofromtonya @anton-vlasenko another blessed PR, more coding style errors rejecting the accepted code. is someone going to fix this problem? I've had it block every single backport PR I've made from Core and it's not getting any better.

[ockham]

@ockham I'm wondering if you can help me appropriately understand this.

• There's no reason to add these classes into wordpress-6.2 because that will no longer be supported once we start targeting 6.4

That's correct 👍

• wordpress-6.3 should receive the full class files because they will be absent in 6.3 environments

No, wordpress-6.3 shouldn't receive these, because they're also absent from 6.3 environments 😅
The way this works is that each compat directory is supposed to recreate the API available from its namesake WP version -- so that API will be available to Gutenberg if run on an earlier WP.

In our case: If Gutenberg is run on WP 6.3, we need to make sure that any functionality we require from the HTML API but that's only available from WP 6.4 is "polyfilled". To that end, we put it into wordpress-6.4.

• wordpress-6.4 should get a Gutenberg_HTML_Processor version of the class for updates that come along in the 6.4 cycle?

Almost; except we don't actually have to rename it. Since we're only introducing the WP_HTML_Processor in WP 6.4, we will be able to keep the two codebases in sync. To reflect that, we should keep the WP_ prefixed names for anything that's newly added in 6.4. (See precedent for the HTML Tag Processor in the 6.2 compat layer.)

[dmsnell]

@hellofromtonya I believe that WPCS is confused here: it thinks I'm missing a @throws tag for a function that I believe cannot throw that exception.

I don't want to lie in the code just to get around the linter. what do you recommend?

also, in case you missed my last message, I was wondering if anyone is working on the mismatch between the linting preferences in this repo vs in Core, as it keeps appearing on these backports, whereas the CI step is rejecting code that has already been accepted and adopted into Core.

should we exclude the lib/compat folder from the linting rules?

[anton-vlasenko]

Suggested change

	$this->last_error = self::ERROR_UNSUPPORTED;
	throw new WP_HTML_Unsupported_Exception( 'Cannot parse outside of the IN BODY insertion mode.' );
	}
	} catch ( WP_HTML_Unsupported_Exception $e ) {
	/*
	* Exceptions are used in this class to escape deep call stacks that
	* otherwise might involve messier calling and return conventions.
	*/
	return false;
	}
	}
	$this->last_error = self::ERROR_UNSUPPORTED;
	$this->throw_unsupported_operation_exception( 'Cannot parse outside of the IN BODY insertion mode.' );
	}
	} catch ( WP_HTML_Unsupported_Exception $e ) {
	/*
	* Exceptions are used in this class to escape deep call stacks that
	* otherwise might involve messier calling and return conventions.
	*/
	return false;
	}
	}
	/**
	* Throws a WP_HTML_Unsupported_Exception message.
	*
	* @param string $error_message An error message.
	*
	* @throws WP_HTML_Unsupported_Exception An exception that is being throwed.
	*/
	private function throw_unsupported_operation_exception( $error_message ) {
	throw new WP_HTML_Unsupported_Exception( $error_message );
	}

[anton-vlasenko]

@dmsnell I will try to reply to your comments above.

I believe that WPCS is confused here: it thinks I'm missing a @throws tag for a function that I believe cannot throw that exception.

Yes, it seems so.
This bug has been reported: squizlabs/PHP_CodeSniffer#3685

I don't want to lie in the code just to get around the linter. what do you recommend?

I'm not exactly sure what you mean by "lie in the code". I don't want to lie in the code either. :)

I suggest moving

throw new WP_HTML_Unsupported_Exception( 'Cannot parse outside of the IN BODY insertion mode.' );

into a separate method to avoid linting errors, given that there is no estimated time of resolution for squizlabs/PHP_CodeSniffer#3685.

Please refer to https://github.com/WordPress/gutenberg/pull/52908/files#r1277785374 for an example.

should we exclude the lib/compat folder from the linting rules?

Honestly, I don't think the lib/compat folder needs to be excluded just because of one bug.

another blessed PR, more coding style errors rejecting the accepted code. is someone going to fix this problem? I've had it block every single backport PR I've made from Core and it's not getting any better.

In February, I submitted a PR to ensure parity between Core and Gutenberg. In my opinion, it's not about just "working" on the problem, because the PR is already there. It's about getting all involved contributors to agree that these changes are needed.
Please take a look at this discussion.

[dmsnell]

I don't think the lib/compat folder needs to be excluded just because of squizlabs/PHP_CodeSniffer#3685.

Nor do I, nor was I suggesting that. I raise this because these backports aren't doing the same things that most PRs are. They aren't introducing code. Apart from the need to guard against redeclaring classes and functions, these backports are taking code that's already been reviewed, accepted, and merged, and brining it over so that functionality continues to work on environment with older versions of WordPress.

My question about linting is that we're asking at the wrong place, because the review has already been made. It's a bit of a disorienting feeling to end up having to make multiple iterations of code on one side because issues aren't raised when the code is actually introduced and accepted.

This has happened to me multiple times and the experience is deflating. At least the CI job is fast enough, but I still frequently experience cases where it rejects code it doesn't like, I adjust the code to what it prefers, but then it decides it doesn't like other aspects it didn't report the first time. Then I have to go back to Core and run through the changes over there and work through opposition of making styling-only changes, particularly for the sake of Gutenberg, and hope I can get it in, all the while Gutenberg plugin code is missing the support that's supposed to be there.

If Core has accepted code, why should we reject it here? If we're unhappy that Core accepted that code, why should we raise those objections after it's happened instead of taking that up in Core?

[dmsnell]

Please take a look at #48603 (review).

Yeah, thanks for the link. I believe that the hassle I go through and my desire to exclude lib/compat is well explained in that discussion.

[dmsnell]

sadly @ockham we somehow missed a * when WPCS wants a /** to be a /*

painfully I've updated this here in expectation that we'll land it in some other change in Core. painfully because I really don't want to have split code in Gutenberg/Core but I'm not going to delay accepted code by another week for the sake of a * on a private internal method 🤦‍♂️

so hopefully we'll get this merged, people can start testing in Gutenberg, and we'll get a move on BUTTON and other support additions

[ockham]

sadly @ockham we somehow missed a * when WPCS wants a /** to be a /*

Uh, bummer 😕

painfully I've updated this here in expectation that we'll land it in some other change in Core. painfully because I really don't want to have split code in Gutenberg/Core but I'm not going to delay accepted code by another week for the sake of a * on a private internal method 🤦‍♂️

Yeah, that's absolutely reasonable.

so hopefully we'll get this merged, people can start testing in Gutenberg, and we'll get a move on BUTTON and other support additions

💯

[dmsnell]

I should have noted here: this missing * was added into Core so there remains no discrepancy between the repos.