Skip to content

Better synchronisation between Gutenberg and Core code#37141

Merged
youknowriad merged 2 commits intotrunkfrom
update/sync-fse-code-1
Dec 7, 2021
Merged

Better synchronisation between Gutenberg and Core code#37141
youknowriad merged 2 commits intotrunkfrom
update/sync-fse-code-1

Conversation

@youknowriad
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
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.)

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