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

lib/compat is confusing. Let's document how the contributors can use it effectively. #41023

Open
adamziel opened this issue May 12, 2022 · 9 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability Developer Experience Ideas about improving block and theme developer experience [Type] Code Quality Issues or PRs that relate to code quality

Comments

@adamziel
Copy link
Contributor

adamziel commented May 12, 2022

What problem does this issue aim to solve?

There's something off with the mental model of lib/compat. There's friction every time it comes up.

For example, we have:

  • lib/compat/wordpress-5.9
  • lib/compat/wordpress-6.0
  • lib/compat/wordpress-6.1

Can you answer these questions?

  • Where do I put code that should only run on WordPress 6.0+?
  • Where do I put code that should only run up to WordPress 6.0?
  • How do I fix a bug in lib/compat/wordpress-5.9 after WordPress 5.9 is released?
  • Why are all the files in these directories always loaded regardless of the actual WordPress version?

If you had to stop for a minute and think really hard, that's what I mean by friction.

Importantly, these question keep coming up on GitHub and slack – perhaps that's why the compat folders are not used as much day to day. @gziolo had to move a bunch of code changes from other locations to these compat folders for the 6.0 release.

What solution does this issue propose?

Ideally, I'd be able to glance over these directories and know exactly what to do.

I'm not sure how to get there, though.

Anything that moves us away from having to think about compat matrices like the one below is a win:

CleanShot 2022-05-12 at 12 53 01

Related

This bit of documentation lives in the README.md:

lib/compat/wordpress-X.Y - Stable features that are intended to be merged to Core in the future X.Y release, or that were previously merged to Core in the X.Y release and remain in the plugin for backwards compatibility when running the plugin on older versions of WordPress.

cc @gziolo @youknowriad @scruffian @getdave @draganescu @noisysocks @talldan @oandregal @ntsekouras @jsnajdr @dmsnell @peterwilsoncc @hellofromtonya @Mamaduka @paaljoachim

@adamziel adamziel added Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Code Quality Issues or PRs that relate to code quality Developer Experience Ideas about improving block and theme developer experience labels May 12, 2022
@adamziel adamziel changed the title Make lib/compat less confusing lib/compat is confusing May 12, 2022
@gziolo
Copy link
Member

gziolo commented May 12, 2022

There should be a different mindset for this. We are trying to be always ahead of WordPress core, so we are adding the code that is not there or needs to be extended.

When we decide that code goes in the given major release, then we add all the code that represents it's number, for example, lib/compat/wordpress-6.0 for the current cycle. We also have to ensure that this code will work for all previous WP releases supported by the Gutenberg plugin. Finally, we need to prevent function/class redeclaration when the same API lands in WP core.

Finally, we can safely remove the given compat folder when the WP version in its name is equal (or lower) to the minimum WP version supported by the plugin. It’s the moment when all logic in that compat folder is present in all supported WP versions.

@ntsekouras
Copy link
Contributor

ntsekouras commented May 12, 2022

The below comments are based on my understanding and might be wrong about something, so please correct me if needed 😄

Where do I put code that should only run up to WordPress 6.0?

What would be a case for that?

How do I fix a bug in lib/compat/wordpress-5.9 after WordPress 5.9 is released?

A bug in 5.9 would be handled in core in a minor version, so it would need to be back ported to GB afterwards.

If you need to update some functionality that exists in core and is intended to land in the next version, you move the code to the next version compat folder and we always need to be careful with this:
Finally, we need to prevent function/class redeclaration when the same API lands in WP core as @gziolo mentioned above.

Why are all the files in these directories always loaded regardless of the actual WordPress version?

Because all these files make GB work for the supported WP versions(2). The contents of the files in previous versions are conditionally declared/called with if ( function_exists ) checks and similar.

@adamziel
Copy link
Contributor Author

adamziel commented May 12, 2022

Where do I put code that should only run up to WordPress 6.0?
What would be a case for that?

For example a function that gets merged to core in WordPress 6.0 and should no longer run the code path from the plugin.

How do I fix a bug in lib/compat/wordpress-5.9 after WordPress 5.9 is released?
A bug in 5.9 would be handled in core in a minor version, so it would need to be back ported to GB afterwards.

That sounds nice, cc @scruffian for the purposes of #40811

If you need to update some functionality that exists in core and is intended to land in the next version, you move the code to the next version compat folder and we always need to be careful with this

That's what I ended up suggesting in https://github.com/WordPress/gutenberg/pull/40811/files#r871221188 but it took some thinking. It's not a straightforward thought process but keep repeating it. An extended README section would be already a big relief.

Because all these files make GB work for the supported WP versions(2). The contents of the files in previous versions are conditionally declared/called with if ( function_exists ) checks and similar.

@ntsekouras That's what I initially thought, too, but there are files like this one that don't appear to have that conditional logic:

add_action( 'init', 'register_site_editor_homepage_settings', 10 );
/**
* Tells the script loader to load the scripts and styles of custom block on site editor screen.
*
* @param bool $is_block_editor_screen Current decision about loading block assets.
* @return bool Filtered decision about loading block assets.
*/
function gutenberg_site_editor_load_block_editor_scripts_and_styles( $is_block_editor_screen ) {
return ( is_callable( 'get_current_screen' ) && get_current_screen() && 'appearance_page_gutenberg-edit-site' === get_current_screen()->base )
? true
: $is_block_editor_screen;
}
add_filter( 'should_load_block_editor_scripts_and_styles', 'gutenberg_site_editor_load_block_editor_scripts_and_styles' );

@ntsekouras
Copy link
Contributor

Where do I put code that should only run up to WordPress 6.0?

What would be a case for that?

For example a function that gets merged to core in WordPress 6.0 and should no longer run the code path from the plugin.

That's when we introduce the checks for existence. Usually a new GB function has the gutenberg prefix, but when it lands in the new WP version, in core we remove that prefix. Then we update GB to have the check and also rename the function.

For example a new function is gutenberg_foo. When we backport to core we name it wp_foo(in core) and then update in GB the gutenberg_foo to if (wp_foo ! exists) and change all its usages. An example PR of this is here .

Because all these files make GB work for the supported WP versions(2). The contents of the files in previous versions are conditionally declared/called with if ( function_exists ) checks and similar.

@ntsekouras That's what I initially thought, too, but there are files like this one that don't appear to have that conditional logic:

The above process is manual and things might have been missed to be renamed/safeguarded or even moved to the new compat folder. The project is huge and lot's of code is added frequently. Also there might be other nuances with filters etc.. like the example you shared. There might be some extensions by GB that do not introduce a new function but make use of a function that applies some filter like here. So the code inside that function is patched into the existing function in core and we don't need a new function. There are many nuances I might not be aware with all the complexity 😄

@adamziel
Copy link
Contributor Author

adamziel commented May 12, 2022

There are many nuances I might not be aware with all the complexity 😄

That's my point! 😆 I wonder how much this is about the specific structure of lib/compat that we use, and how much about the unavoidable complexity of maintaining WP/Gutenberg separately.

@gziolo
Copy link
Member

gziolo commented May 12, 2022

There are many nuances I might not be aware with all the complexity 😄

That's my point! 😆 I wonder how much this is about the specific structure of lib/compat that we use, and how much about the unavoidable complexity of maintaining WP/Gutenberg separately.

I had also a hard time wrapping my head around what WP versions mean in lib/compat/ and when it's going to be removed so this needs to be included in the docs. If we can find better words to explain those ideas in the folder structure then it would be great.

@ntsekouras
Copy link
Contributor

That's my point! 😆 I wonder how much this is about the specific structure of lib/compat that we use, and how much about the unavoidable complexity of maintaining WP/Gutenberg separately.

I think it's mostly about the unavoidable complexity. In the last two years that I was first involved with that process, I believe great progress has been made, but in general this is no easy task 😄

If we can find better words to explain those ideas in the folder structure then it would be great.

I agree with @gziolo 👍

@noisysocks
Copy link
Member

noisysocks commented May 16, 2022

lib/compat is confusing

Agree.

I'm not sure how to get there, though.

Agree.

Anything that moves us away from having to think about compat matrices like the one below is a win:

I don't think anything we come up with can eliminate the need for a compat matrix like that because at the end of the day we'll always have different versions of WordPress and Gutenberg which can be installed in various combinations onto a single website.

I think all we can do right now is better documentation. The README.md file I added was just something basic to get us started. It would be nice if we had FAQ-style documentation for what to do in all of the common scenarios that developers face. For example @glendaviesnz ran into a confusing scenario last week, it would be good to document the solution.

Or, you know, merge Core and Gutenberg into a monorepo 😀

@adamziel
Copy link
Contributor Author

adamziel commented May 16, 2022

I think all we can do right now is better documentation.

@noisysocks Hmm, I agree. There is also a chance that improving the documentation will help us notice patterns we don't see right now and ultimately lead to a simpler process.

Until then, let's keep an eye on pull requests targeting the lib/compat and nudge the authors to document their use case in README.md.

Or, you know, merge Core and Gutenberg into a monorepo 😀

I'd really like to see that happening!

@adamziel adamziel changed the title lib/compat is confusing lib/compat is confusing. Let's document how the contributors can use it effectively. May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability Developer Experience Ideas about improving block and theme developer experience [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

4 participants