Multiple classes should be in separate files (class-wp-block-parser.php)

[aristath]

The wp-includes/class-wp-block-parser.php file currently contains multiple classes.
We should separate those, and keep one class per file as per our coding standards.

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

[rajanpanchal2028]

Separating each class into its own file can improve the maintainability and readability of the code. This approach also helps with the organization of code and makes it easier to find specific classes.

[aristath]

Adding the has-patch keyword, there's a PR attached to the ticket (​https://github.com/WordPress/wordpress-develop/pull/4152)

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).

are there any tests associated with these classes ?

I didn't find any 👍

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).

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).

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 :slightly_smiling_face:

[manfcarlo]

It looks like this file should not be edited, see [43751]

[aristath]

@manfcarlo Thank you for the comment!
I submitted an accompanying PR in Gutenberg: ​https://github.com/WordPress/gutenberg/pull/48693 👍

[SergeyBiryukov]

Related: #50009

[spacedmonkey]

@SergeyBiryukov The existing PR looks good to merge to me. This is a simple change and you should be good to commit.

Is this blocked by gutenberg at all? Does the gutenberg ​PR need to need before this?

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

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

[mukesh27]

This ticket was discussed during the bug scrub.

The PR is ready for commit, but there are some failures in the end-to-end (e2e) tests that need to be fixed before proceeding with the commit.

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]

Committers: Please remember to use the svn cp command when committing this in order to preserve the history of the files.

See [35389] as an example.

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

​https://github.com/WordPress/wordpress-develop/blob/d125ed019c21970d3f4b6f057908def327a2be85/tools/webpack/packages.js#L101-L103

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

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

​https://github.com/WordPress/wordpress-develop/blob/d125ed019c21970d3f4b6f057908def327a2be85/tools/webpack/packages.js#L101-L103

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?

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]

The PR looks ready to commit, having discussed this with @isabel_brison I'll be committing this seperately to the other package updates as it's a little more complex than the usual package update.

In order to retain the history in the new files, I'll be running the following commands:

svn cp ./src/wp-includes/class-wp-block-parser.php ./src/wp-includes/class-wp-block-parser-block.php
rm ./src/wp-includes/class-wp-block-parser-block.php
svn cp ./src/wp-includes/class-wp-block-parser.php ./src/wp-includes/class-wp-block-parser-frame.php
rm ./src/wp-includes/class-wp-block-parser-frame.php
gh pr diff 4152 --repo=WordPress/wordpress-develop | patch -p1

The rm commands are to make sure patch doesn't get upset creating the files but doesn't affect the history.

I've moved this ticket to the Editor component as the bulk of the changes come from the upstream Gutenberg repo.

[peterwilsoncc]

In 56048:

Editor: Update block-serialization-default-parser package for WP 6.3 Beta 1.

Update the @wordpress/block-serialization-default-parser to 4.35.1 for WordPress 6.3 Beta 1. These changes split the following classes in to their own files in order to match the WordPress PHP coding standards:

• WP_Block_Parser_Block
• WP_Block_Parser_Frame
• WP_Block_Parser

These classes were previously all included in the src/wp-includes/class-wp-block-parser.php file. In order to maintain backward compatibly for developers requiring the file directly, the relocated classes are replaced with require_once calls in the original file.

In order to retain the commit history of the new files, they have been created using the svn copy command.

Props aristath, rajanpanchal2028, jrf, SergeyBiryukov, costdev, manfcarlo, spacedmonkey, mukesh27, isabel_brison, dd32.
Fixes #57832.
See #58623.

merged r56048 / ​https://github.com/WordPress/wordpress-develop/commit/4aeb284d44ff29b3b67dc568b6f8f0472a0e62c8

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