Make WordPress Core

Opened 12 months ago

Closed 8 months ago

Last modified 8 months ago

#57832 closed enhancement (fixed)

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

Reported by: aristath's profile aristath Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: Editor Keywords: has-patch commit gutenberg-merge
Focuses: coding-standards Cc:

Description

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.

Change History (27)

This ticket was mentioned in PR #4152 on WordPress/wordpress-develop by @aristath.


12 months ago
#1

  • Keywords has-patch added

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

#2 @rajanpanchal2028
12 months ago

  • Focuses accessibility added
  • Keywords needs-patch added; has-patch removed
  • Version set to trunk

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.

#3 @aristath
12 months ago

  • Keywords has-patch added; needs-patch removed

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

@jrf commented on PR #4152:


12 months ago
#4

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

#5 @jrf
12 months ago

  • Focuses accessibility removed

@aristath commented on PR #4152:


12 months ago
#6

are there any tests associated with these classes ?

I didn't find any 👍

@jrf commented on PR #4152:


12 months ago
#7

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

@jrf commented on PR #4152:


12 months ago
#8

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

#9 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

@costdev commented on PR #4152:


12 months ago
#10

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:

#11 @manfcarlo
12 months ago

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

#12 @aristath
12 months ago

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

#14 @spacedmonkey
10 months ago

  • Keywords commit added

@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?

@spacedmonkey commented on PR #4152:


9 months ago
#15

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

@aristath commented on PR #4152:


9 months ago
#16

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

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


9 months ago

#18 @mukesh27
9 months ago

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.

@spacedmonkey commented on PR #4152:


8 months ago
#19

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

#20 @peterwilsoncc
8 months ago

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.

@peterwilsoncc commented on PR #4152:


8 months ago
#21

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.

@spacedmonkey commented on PR #4152:


8 months ago
#22

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?

#23 @spacedmonkey
8 months ago

  • Keywords gutenberg-merge added

@peterwilsoncc commented on PR #4152:


8 months ago
#24

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.

#25 @peterwilsoncc
8 months ago

  • Component changed from General to Editor

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.

#26 @peterwilsoncc
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

@peterwilsoncc commented on PR #4152:


8 months ago
#27

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.

Note: See TracTickets for help on using tickets.