Remove references to Gutenberg specific functions

[desrosj]

There are a few references to Gutenberg specific functions or named with the gutenberg_ prefix that should be updated in Core.

• The render_block_core_legacy_widget() function has calls to gutenberg_get_widget_key() and gutenberg_get_widget_object() (source). It seems that this entire conditional block can be removed in favor of the first condition's code since WordPress >= 5.8 can be confident the methods exist.
• In wp-includes/blocks/post-template.php, there is a function named gutenberg_register_legacy_query_loop_block(). The docblock says that it "can be removed when WordPress 5.9 is released" and appears to unregister the core/query-loop block before re-registering it. Is there any reason why Core can't authoritatively register the block type as it does others? (source).

[walbo]

The conditional in render_block_core_legacy_widget() can be removed. As you say 5.8 can be confident the methods exist (and gutenberg_get_widget_key()/gutenberg_get_widget_object() doesn't exist in core)

[hellofromTonya]

The render_block_core_legacy_widget() function has calls to gutenberg_get_widget_key() and gutenberg_get_widget_object() (source). It seems that this entire conditional block can be removed in favor of the first condition's code since WordPress >= 5.8 can be confident the methods exist.

Code in question:

if ( method_exists( $wp_widget_factory, 'get_widget_key' ) && method_exists( $wp_widget_factory, 'get_widget_object' ) ) {
        $widget_key    = $wp_widget_factory->get_widget_key( $id_base );
        $widget_object = $wp_widget_factory->get_widget_object( $id_base );
} else {
        $widget_key    = gutenberg_get_widget_key( $id_base );
        $widget_object = gutenberg_get_widget_object( $id_base );
}

This method_exists and else conditional exist for the Gutenberg plugin and not for Core. They allow the plugin to run with older versions of WordPress. Core doesn't need them.

They can be removed from Core, simplifying the code to:

$widget_key    = $wp_widget_factory->get_widget_key( $id_base );
$widget_object = $wp_widget_factory->get_widget_object( $id_base );

However in doing so, the packages in Gutenberg diverge from Core. Other parts of merged code have already diverged. But I don't believe the packages themselves have.

Does modifying this function in Core run the risk of being overwritten when the Gutenberg packages are updated and merged in later?

[walbo]

Replying to hellofromTonya:

Does modifying this function in Core run the risk of being overwritten when the Gutenberg packages are updated and merged in later?

Running npm run build:dev will revert any changes done in Core.

[hellofromTonya]

Replying to walbo:

Running npm run build:dev will revert any changes done in Core.

Thanks @walbo! Alrighty, this means the change comes from the packages in the Gutenberg repo and are then merged back into Core. However, the code is needed in the plugin to allow it to run on other WordPress versions.

One way to fix it:
Set and check for >= WordPress 5.8 in Gutenberg for the widgets functionality. Then render_block_core_legacy_widget in the package file can be modified to remove the GB-specific code.

This is likely an upstream discussion.

[azaozz]

Replying to walbo:

Running npm run build:dev will revert any changes done in Core.

Right, unfortunately. BTW why are there PHP files in the JS packages in npm? Sounds... somewhat not right (I agree that's another discussion, will open a ticket/issue).

The most straightforward way to fix this seems to be to "decouple" wp-includes/blocks/legacy-widget.php and stop updating it from npm. Should also be removed from the package, but that's a discussion for the Gutenberg repo.

If the intent is to keep Gutenberg working as a plugin "decoupling" seems to be the best way forward for all PHP files, not just this one. Also, it should follow the basic rule for WP plugins: always prefix all names! :)

[noisysocks]

Yes, like others have pointed out, this bit of PHP is copied from the Gutenberg plugin when grunt build --dev is run. We can't remove the references to gutenberg_ functions from the PHP code in the Gutenberg plugin because the Gutenberg plugin needs to support previous versions of WordPress.

Right, unfortunately. BTW why are there PHP files in the JS packages in npm? Sounds... somewhat not right (I agree that's another discussion, will open a ticket/issue).
The most straightforward way to fix this seems to be to "decouple" wp-includes/blocks/legacy-widget.php and stop updating it from npm. Should also be removed from the package, but that's a discussion for the Gutenberg repo.

A block's JavaScript and CSS is highly dependent on any PHP that the block has. I'd argue that coupling them is the correct thing to do. But, yes, like you say, that's a discussion for another time :)

I don't think we should make an exception for only legacy-widget.php. Right now, all blocks have their PHP copied from Gutenberg. Let's be consistent.

If the intent is to keep Gutenberg working as a plugin "decoupling" seems to be the best way forward for all PHP files, not just this one. Also, it should follow the basic rule for WP plugins: always prefix all names! :)

Gutenberg will prefix any function name in the block's PHP when running as a plugin. The prefix is removed when packaged to npm for use in Core.

---

Personally I think that there is no issue here. There is no user-facing bug and the core PHP code which references gutenberg_ functions will never run.

It's potentially confusing to developers who are reading wp-includes/blocks/legacy-widget.php to see a gutenberg_ prefix in the source code, but this will be removed once the Gutenberg plugin requires WordPress 5.8 as its minimum version. We should potentially also look into making it clearer that these files are auto-generated by including a comment at the top of the file.

So, I'd close this ticket as wontfix. But happy to defer to the group :)

[azaozz]

Replying to noisysocks:

A block's JavaScript and CSS is highly dependent on any PHP that the block has.

Right, but what happens when that PHP code in no good for core? Should the Gutenberg plugin include PHP in packages that is intended only for the current WP trunk? Thinking yes :)

If Gutenberg wants to be compatible with older WP versions, that compatibility code should "live" in /lib imho. (Yeah, that means in older WP the Gutenberg plugin will not include the PHP code from the package but a modified version of it).

Personally I think that there is no issue here...

It's potentially confusing to developers...

Yeah, after [51193] perhaps just some nice inline comments explaining that the gutenberg_ prefix is temporary and what's going on. Then try to really fix this in the Gutenberg plugin for WP 5.9.

[desrosj]

I've opened a PR in the Gutenberg repository to address the two widget functions. The PR:

• Adds two filters for changing the value of $widget_key and $widget_object.
• Adds functions hooked to those filters in the compatibility folder.
• Removes the Gutenberg specific function calls in the file copied to WordPress Core.

I don't love adding two individual filters to get around this, but I dislike shipping plugin specific code more.

Please give it a review and test if you can: ​https://github.com/WordPress/gutenberg/pull/33331

I'm looking at the post-template.php gutenberg_ function call now.

[desrosj]

Adjusted the PR to account for the second plugin specific code segment being included in Core.

[jorbin]

If the packages include PHP that is solely intended to be copied into WordPress Core, then I think it's important for that code to follow the WordPress standards. WordPress uses either no prefix, or it uses the wp_ prefix. The gutenberg prefix should be reserved for the plugin. The only time that WordPress core should be referencing functions with the gutenberg prefix are when upgrading WordPress should be forcing a deactivation of the plugin.

The committer who is updating code, whether it is auto-added such as here, written by themselves, or from someone's patch should be looking for things like this and not making updates that add function names that aren't in line with core's standards.

[azaozz]

Replying to desrosj:

I don't love adding two individual filters to get around this, but I dislike shipping plugin specific code more.

Yeah, from a technical point of view having two (mostly unusable?) filters there that will have to be supported in the future is not a good thing.

On the other hand having some code in core that references a plugin is no good either.

On yet another hand Gutenberg is part of WP core despite that it is being developed in a different repo and that many of the contributors to it don't contribute to the rest of WP.

Replying to jorbin:

The gutenberg prefix should be reserved for the plugin. The only time that WordPress core should be referencing functions with the gutenberg prefix are when upgrading WordPress should be forcing a deactivation of the plugin.

Yep, I agree. Would be ideal to not have any code in core that references a plugin, no matter which. On the other hand Gutenberg can be seen as a "component" of core that's also being made available for previous versions of WP. It uses the plugins API for lack of a better way. In that case, perhaps, having some core code referencing it is not completely out of the question.

Frankly I'm 50/50 on which is better: few gutenberg_ prefixes (that will likely be removed in the next WP version) or couple of highly specialized filters (that will likely stick around "forever"). From a purely technical point of view the prefixes are better/easier to deal with. Perhaps adding some inline comments there clearly explaining the use of the prefixes would be an acceptable compromise?

[SergeyBiryukov]

Replying to azaozz:

Frankly I'm 50/50 on which is better: few gutenberg_ prefixes (that will likely be removed in the next WP version) or couple of highly specialized filters (that will likely stick around "forever"). From a purely technical point of view the prefixes are better/easier to deal with. Perhaps adding some inline comments there clearly explaining the use of the prefixes would be an acceptable compromise?

That does seem like an acceptable compromise to me :)

[desrosj]

I think that would be an acceptable compromise as well.

I've been making a few adjustments on the PR, and the post-template.php related function can most likely just be renamed. See the ​discussion on the PR.

[desrosj]

I've ​updated the PR with a detailed inline comment preceding the gutenberg_ functions in the legacy-widget.php file.

Please review if you have a chance!

[desrosj]

This was addressed in [51421] and backported to 5.8 in [51422].