Split class-wp-block-parser.php to multiple files

[aristath]

This is a simple change, splitting the classes from wp-includes/class-wp-block-parser-block.php to 3 separate files (one file per class).

Trac ticket: https://core.trac.wordpress.org/ticket/57832

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[costdev]

Thanks for the PR @aristath! LGTM 👍

On a separate note regarding those properties/variables that use camelCase rather than snake_case:

I'm not sure why we still exclude those - or why they were committed as such in the first place. I know that the respective JS keys are in camelCase, but it seems strange to create this inconsistency in Core so that it matches a different language's standards. In this situation, we'd usually implement magic methods for back-compat and bring these in line with the PHP coding standards. That may be something to do in future, along with some minor docs updates.

[jrfnl]

On a separate note regarding those properties/variables that use camelCase rather than snake_case:

I'm not sure why we still exclude those - or why they were committed as such in the first place. I know that the respective JS keys are in camelCase, but it seems strange to create this inconsistency in Core so that it matches a different language's standards. In this situation, we'd usually implement magic methods for back-compat and bring these in line with the PHP coding standards. That may be something to do in future, along with some minor docs updates.

Please, please, please do NOT introduce magic methods. For a full explanation, see: https://www.youtube.com/watch?v=vDZWepDQQVE

The function call parameter names for the __construct() method can be changed to snake_case without impacting plugins/themes as those are method local names (providing people aren't using PHP 8.0+ function calls with named parameters, but as WP hasn't announced compatibility with that, that's not our concern).

The properties names can unfortunately not be changed easily (not unless we make this a final class and yes, introduce magic methods, but then correctly).

[jrfnl]

One question: are there any tests associated with these classes ? If so, should those be split off to their own test classes and test files to match the source classes/files as well ?

[aristath]

are there any tests associated with these classes ?

I didn't find any 👍

[jrfnl]

are there any tests associated with these classes ?

I didn't find any 👍

Ah, so not relevant for this PR, though enhancement opportunity for a future PR (not that there is much to test in these classes, so not high priority). Thanks for checking. 👍🏻

[costdev]

The properties names can unfortunately not be changed easily (not unless we make this a final class and yes, introduce magic methods, but then correctly).

Yep, that's what I meant 🙂

[spacedmonkey]

@aristath I am happy to commit. But I am seeing e2e and performance test failures. Can you take a look into these?

[aristath]

I have a few days off until mid next week... I'll be able to follow up then 👍

[spacedmonkey]

E2E tests are failing because of this error.

Fatal error: Cannot declare class WP_Block_Parser_Block, because the name is already in use in /var/www/src/wp-includes/class-wp-block-parser.php on line 15
Error: There has been a critical error on this website.Learn more about troubleshooting WordPress. There has been a critical error on this website.
255

[peterwilsoncc]

@spacedmonkey has asked me to take a look at the failing tests, I'll add a follow up comment if I figure it out.

I've put a note inline about ensuring back-compat.

[peterwilsoncc]

Tests are failing as the file is coming from the package https://github.com/WordPress/gutenberg/tree/trunk/packages/block-serialization-default-parser, so the npm build script is reverting the changes made in files within this PR.

The files have been split in the Gutenberg package so as part of the package update, these lines will need to be updated

wordpress-develop/tools/webpack/packages.js

Lines 101 to 103 in d125ed0

	const phpFiles = {
	'block-serialization-default-parser/parser.php': 'wp-includes/class-wp-block-parser.php',
	};

That should solve the problem. I'll log a follow up issue on the Gutenberg repo about ensuring backward compatibility if it's required.

[spacedmonkey]

Tests are failing as the file is coming from the package https://github.com/WordPress/gutenberg/tree/trunk/packages/block-serialization-default-parser, so the npm build script is reverting the changes made in files within this PR.

The files have been split in the Gutenberg package so as part of the package update, these lines will need to be updated

wordpress-develop/tools/webpack/packages.js

Lines 101 to 103 in d125ed0

	const phpFiles = {
	'block-serialization-default-parser/parser.php': 'wp-includes/class-wp-block-parser.php',
	};

That should solve the problem. I'll log a follow up issue on the Gutenberg repo about ensuring backward compatibility if it's required.

Thanks for looking at this @peterwilsoncc . This is blocked up the upstream back is released right?

[peterwilsoncc]

Thanks for looking at this @peterwilsoncc . This is blocked up the upstream back is released right?

Yeah, pretty much. The update to the block-editor packages for beta will include the new files.

I think this PR can be superseded by one that changes the build process in WP-Dev to account for the incoming changes from the packages. Otherwise, WP-Dev will end up throwing a fatal once they're updated.

[peterwilsoncc]

merged r56048 / 4aeb284

@felixarntz I missed your code review so will add you to the props list manually via make/core.