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

Gallery Block: Use wp_enqueue_block_support_styles() if possible #43779

Merged

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 1, 2022

What?

In the Gallery Block, use wp_enqueue_block_support_styles() if it's available, and if it supports the $priority arg.

Why?

To enable us to use the two-argument version of wp_enqueue_block_support_styles() (currently being backported to Core here), and eventually remove the gutenberg_enqueue_block_support_styles() shim.

How?

By checking if the function exists, and if it supports two arguments.

Testing Instructions

Verify that the Gallery block still works.

@ockham ockham added the [Block] Gallery Affects the Gallery Block - used to display groups of images label Sep 1, 2022
@ockham ockham self-assigned this Sep 1, 2022
@ockham ockham requested a review from geriux as a code owner September 1, 2022 13:50
Comment on lines +110 to +113
if (
function_exists( 'wp_enqueue_block_support_styles' ) &&
2 === count( ( new ReflectionFunction( 'wp_enqueue_block_support_styles' ) )->getParameters() )
) {
Copy link
Contributor Author

@ockham ockham Sep 1, 2022

Choose a reason for hiding this comment

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

An alternative would be

Suggested change
if (
function_exists( 'wp_enqueue_block_support_styles' ) &&
2 === count( ( new ReflectionFunction( 'wp_enqueue_block_support_styles' ) )->getParameters() )
) {
if ( version_compare( get_bloginfo( 'version' ), '6.1', '>=' ) ) {

but I find that less semantic. It also is less accurate, e.g. during the 6.1 alpha phase (now): It would evaluate to false even once WordPress/wordpress-develop#3158 is merged (and used in a local install).

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

I've tested this classic and block themes. Editor and frontend match and the CSS is loading as expected. Thank you! 👍

It seems like a clever approach.

I was just curious to know why we couldn't leave gutenberg_enqueue_block_support_styles working as it is and remove it later once migration has occurred, or the minimum version is 6.1?

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

I was just curious to know why we couldn't leave gutenberg_enqueue_block_support_styles working as it is and remove it later once migration has occurred, or the minimum version is 6.1?

I don't have an answer but would also like to understand this. FYI, It was mentioned by @gziolo in #43440 (comment) that we should:

refactor ocurrences of gutenberg_enqueue_block_support_styles in core blocks in the Gutenberg plugin

@ockham
Copy link
Contributor Author

ockham commented Sep 5, 2022

Thanks, folks!

I was just curious to know why we couldn't leave gutenberg_enqueue_block_support_styles working as it is and remove it later once migration has occurred, or the minimum version is 6.1?

Aside from what Michal mentioned, I think the reason is that we don't want to have any gutenberg_ prefixed functions in Core -- which includes dynamic blocks' index.php files that are backported to Core. (And while we cannot entirely avoid the gutenberg_ prefixed function call being present in Core, we can at least ensure that it will never be called from there.)

@ockham ockham merged commit 0a51142 into trunk Sep 5, 2022
@ockham ockham deleted the update/make-enqueue-block-supports-style-shim-conditional branch September 5, 2022 12:09
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 5, 2022
@gziolo
Copy link
Member

gziolo commented Sep 5, 2022

It isn't the most elegant solution to run this type of check that exists only because of the limitation of the Gutenberg plugin. You can also use Webpack to rewrite PHP files during the build to use the function that patches older versions of WordPress, similar to what we have here:

/**
* We need to automatically rename some functions when they are called inside block files,
* but have been declared elsewhere. This way we can call Gutenberg override functions, but
* the block will still call the core function when updates are back ported.
*/
const prefixFunctions = [ 'build_query_vars_from_query_block' ];

@ockham
Copy link
Contributor Author

ockham commented Sep 5, 2022

Thanks @gziolo -- and TIL! That sounds like a good fit. I'll revert and will file an alternative PR.

We might need to tweak the Webpack logic a bit though to replace the wp_ prefix in wp_enqueue_block_support_styles with gutenberg_, since we might not want both, i.e. gutenberg_wp_enqueue_block_support_styles 🤔

@gziolo
Copy link
Member

gziolo commented Sep 5, 2022

We might need to tweak the Webpack logic a bit though to replace the wp_ prefix in wp_enqueue_block_support_styles with gutenberg_, since we might not want both, i.e. gutenberg_wp_enqueue_block_support_styles 🤔

It's nice to have, but I wouldn't make it too easy so it becomes too tempting to use everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants