Bring the PHP implementation of the block parser inline with WordPress coding standards

[johnbillion]

The file that contains the PHP implementation of the block parser does not conform to WordPress core's coding standards (for example, it includes multiple class declarations in one file), therefore its corresponding file in core has to be excluded from the coding standards checks because of its low quality.

This file should be brought up to core coding standards which will allow the coding standards exclusion to be removed from core.

[chrisvanpatten]

I think it's very important to note that the exceptions made for the block parser are largely intentional — using camel-cased attribute names, for instance, is to keep the parser aligned with the JavaScript version. I believe the decision to keep multiple classes in one file was done, again, to align closer to the JavaScript parser. Some of the thinking here is also based on a previous iteration of the PHP parser, which was generated automatically from the PEG spec (#1152).

There is some previous discussion on this in #13016 (and I believe elsewhere, although I'm having trouble finding some of those discussions).

That's not to say it can't be aligned closer to the coding standards, but as it stands I believe most of the decisions made were done so intentionally. Perhaps to help minimize future confusion, those decisions could be logged in the code so it's clearer what those compromises are, and why they were chosen.

[chrisvanpatten]

(Also, I think @dmsnell might have additional context here as I think a lot of this work came from him.)

[chrisvanpatten]

Ah, I found some of the historical discussion — and sure enough it was me having the discussion there as well 😅

#10379

I think based on that conversation it does seem like there is probably some coding standards alignment that can be done, and perhaps limit the standards ignoring to only certain rules (I believe phpcs lets you ignore specific rules for an entire file?) like the variable naming sniffs.

[dmsnell]

because of its low quality.

well that feels harsh 😉

seriously though do we have any other examples of library code in WordPress? do we not have any other dependencies that conform to a different style?

@johnbillion do you think it would be better to split these internal classes into separate files? I feel like the parser still sits as something qualitatively different. splitting it into multiple files would only break apart logic that has to stay together, and those classes are doing nothing but providing some structure to otherwise dynamic data structures - making it convenient for an IDE to catch a bug when writing the code.

if we want to make the code less clear in order to make it lintable then I would argue that the more appropriate decision would be to remove WP_Block_Parser_Block, WP_Block_Parser_Frame, and all the documentation for their properties and replace them with associate arrays. it'd be wise to run this through the parser comparator to see if the removal of the classes would have any impact on performance, but from what I can tell, in PHP it shouldn't, unlike in JavaScript where the hidden classes do matter.

private function create_block( $name, $attrs, $innerBlocks, $innerHTML, $innerContent ) {
	return array(
		'blockName'    => $name,
		'attrs'        => $attrs,
		'innerBlocks'  => $innerBlocks,
		'innerHTML'    => $innerHTML,
		'innerContent' => $innerContent,
	);
}

private function create_frame( $block, $token_start, $token_length, $prev_offset = null, $leading_html_start = null ) {
	return array(
		'block'              => $block,
		'token_start'        => $token_start,
		'token_length'       => $token_length,
		'prev_offset'        => isset( $prev_offset ) ? $prev_offset : $token_start + $token_length,
		'leading_html_start' => $leading_html_start,
	);
}

-					new WP_Block_Parser_Frame(
-						new WP_Block_Parser_Block( $block_name, $attrs, array(), '', array() ),
+					$this->create_frame(
+						$this->create_block( $block_name, $attrs, array(), '', array() ),
						$start_offset,
						$token_length,
						$start_offset + $token_length,
						$leading_html_start
					)

this is mostly a lateral change to the code but I do think we lose value if we take away the statically-available type information.

concerning the variable names I think the argument still holds that conformance to the parser spec should probably take higher precedence than the linting rules. do you see other issues than this and the multiple class declarations that you think should be changed?

I don't think it'd be the worst thing in the world to split those helper classes into separate files but I do think it'd be a step backwards to expose them even more publicly. there's no reason for any code to ever create those outside of the WP_Block_Parser class as they are purely implementation internals to the algorithm.

anyway, I'm open to seeing what we can do to make the linting work easier to maintain and interested in your thoughts on these points.

[johnbillion]

well that feels harsh 😉

Apologies, I meant "low quality" from the robotic viewpoint of the WordPress coding standards ruleset, nothing else.

do we have any other examples of library code in WordPress? do we not have any other dependencies that conform to a different style?

Yeah, loads! SimplePie, ID3, Requests, etc. The difference is that these are third party dependencies whereas Gutenberg is a WordPress project. I would expect that it should adhere to the WordPress coding standards.

This brings up a deeper discussion point - if WordPress itself doesn't adhere to its own coding standards, is that ok? Or should the standards change? Does this apply to the files in WordPress core that originate from Gutenberg? There might not be anything inherently "wrong" with the code in parser.php, but it doesn't match how the rest of the WordPress code is formatted.

[dmsnell]

if WordPress itself doesn't adhere to its own coding standards, is that ok?

I'm definitely not trying to expand this discussion to break from the coding standards everywhere. This code specifically feels like an exemption to the norm. For example, a few weeks ago I wrote a Java implementation and I've been meaning to toss it in here well - with that third implementation (important for the WordPress app) all three default parsers are highly symmetric even though they are written in three different languages. Should someone have to learn three different systems in order to make a small change to the parsing code?

I don't think this is the case that the parser needs an exemption so much as it feels like the coding standard fails to address this scenario.

Are there other places where there needs to be a high symmetry between the PHP and the JavaScript? I looked up autop but it started as PHP first and in JavaScript adopts the JS conventions.

---

If this were a third-party library like ID3 I don't think there'd be any reason to make the changes, mostly because it's a highly-specific library symmetric with implementations in other languages. 🤷

[chrisvanpatten]

We’ve gotten more questions about this issue in the past few weeks from new contributors. Given that there is still debate on the best direction, I’m removing the “good first issue” label so new contributors aren’t guided here until it’s in a place where work can be done.

[dmsnell]

At the end of the discussion if having something different in the parser is causing people to stumble then it's just an inconvenience for a few folk who work in the parser to separate it all out. Sometimes we make concessions for the linter we otherwise wouldn't want to do and that's just how it works; because it screams 😉