Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better synchronisation between Gutenberg and Core code #37141

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

youknowriad
Copy link
Contributor

In the ideal scenario, Gutenberg's php code should be composed of three things:

  • A lib/compat folder with all the changes that are in Core or supposed to land in the next version in Core. (Most of the php code)
  • PHP code that get updated and back ported on each release automatically. For now, we have just two things here: Dynamic blocks php code and block supports.
  • Glue code that is specific to the plugin: "demo page for instance..."

Right now, we still have a lot of php code that is "unclear": lives outside of lib/compat but is already on core or its status need to be clarified. This kind of code makes it very hard to maintain the plugin and do the backports on each WordPress release. Ideally, we should seek to remove that code or move it to the right place. This PR starts that work by focusing on two FSE files.

@youknowriad youknowriad added [Type] Code Quality Issues or PRs that relate to code quality Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Dec 6, 2021
@youknowriad youknowriad self-assigned this Dec 6, 2021
@@ -68,4 +68,7 @@ function gutenberg_filter_wp_template_unique_post_slug( $override_slug, $slug, $

return $override_slug;
}

// Remove 5.8 filter if existant.
remove_filter( 'pre_wp_unique_post_slug', 'wp_filter_wp_template_unique_post_slug' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That filter exists on 5.8 but is slightly different.

@@ -152,7 +152,7 @@ static function( $classes ) {
'siteUrl' => site_url(),
'postsPerPage' => get_option( 'posts_per_page' ),
'styles' => gutenberg_get_editor_styles(),
'defaultTemplateTypes' => gutenberg_get_indexed_default_template_types(),
'defaultTemplateTypes' => get_default_block_template_types(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gutenberg plugin is using a special function here that is different from Core, I was not entirely sure which one was the right one, but I decided to use the Core approach. Let me know if this is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a failing end2end test, if I had to guess, I think it's related to the change here, meaning Core is probably also broken and missing a backport :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so this should be solved with dd377ed and that commit need to be backported in Core properly.


foreach ( gutenberg_get_template_type_slugs() as $template_type ) {
$template_type_slugs = array_keys( get_default_block_template_types() );
foreach ( $template_type_slugs as $template_type ) {
if ( 'embed' === $template_type ) { // Skip 'embed' for now because it is not a regular template type.
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template loader is something that landed on 5.8 I believe but was tweaked on 5.9, so ideally the code here should be moved somehow to lib/compat/5.9 folder maybe. cc @ockham

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I see you're addressing that in #37149 👍

@talldan
Copy link
Contributor

talldan commented Dec 6, 2021

A lib/compat folder with all the changes that are in Core or supposed to land in the next version in Core. (Most of the php code)

To be honest, I didn't know this until recently. I wonder if other contributors might be the same. It might be good to put a README in the folder to explain how it works. I don't mind having a look at doing that (hopefully I have the right understanding now).

@youknowriad
Copy link
Contributor Author

To be honest, I didn't know this until recently. I wonder if other contributors might be the same.

Yeah, it's not very old, @gziolo proposed this a couple months ago, It makes sense and simplify backports and such. And yes, we definitely need to document this in the folder structure page or more. 👍

@gziolo
Copy link
Member

gziolo commented Dec 6, 2021

To be honest, I didn't know this until recently. I wonder if other contributors might be the same.

Yeah, it's not very old, @gziolo proposed this a couple months ago, It makes sense and simplify backports and such. And yes, we definitely need to document this in the folder structure page or more. 👍

Related issue: #33810. We should formalize it once we have the structure in place. I know that @noisysocks started exploring those ideas a few months ago in the https://github.com/WordPress/gutenberg/tree/update/refactor-php branch, but I guess it was too much work for a single contributor.

@youknowriad
Copy link
Contributor Author

Yes, we can't change everything in one step, doing it file by file seems a good compromise while also checking the impact compared to Core. For instance this just helped uncover an unbackported change dd377ed

@youknowriad youknowriad force-pushed the update/sync-fse-code-1 branch from dd377ed to 28797dc Compare December 6, 2021 11:38
@youknowriad
Copy link
Contributor Author

I could use a ✅ here to continue iteration on these kind of changes.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Riad! ✅

@noisysocks
Copy link
Member

Agree with all of this 👍 thanks for tackling it @youknowriad.

A README.md in lib would be good to add. It could contain some best practices too. For example:

  • It's easier to backport things when PHP is grouped together by feature (full-site-editing.php, widgets.php) instead of by functionality (blocks.php, functions.php).
  • It's easier to backport things when the call to add_filter, add_action is right next to the function definition.
  • It's easier to backport things when PHP doc comments are used to describe where in Core the functionality is supposed to land. (Or, even better, there's a link to a Trac ticket.)

@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants