Fix gutenberg prefixed function references in core

[talldan]

Description

Closes #36962

The problem described there is that when the navigation block's PHP file is copied to WordPress core's codebase, it still contains calls to three functions with gutenberg_ prefixes, which do not exist in core.

This PR moves the three functions (gutenberg_get_menu_items_at_location, gutenberg_sort_menu_items_by_parent_id, and gutenberg_parse_blocks_from_menu_items) to the navigation block's PHP file and renames them to have block prefixes. Also introduces a IS_GUTENBERG_PLUGIN definition and uses that to avoid defining and calling these functions in WordPress core

As part of that, I deleted gutenberg_migrate_menu_to_navigation_post which also calls those functions. That function isn't needed any more as the navigation area block is deprecated. This won't break any user content, as this migration is a side-effect rather than a main feature of the block.

I also removed gutenberg_get_navigation_areas_paths_to_preload for the same reason.

How has this been tested?

Testing that __unstableLocation works in the plugin

1. Use a classic theme (these steps will use TT1, the non-blocks version)
2. Create a classic menu with at least one item and give that item a memorable label that's different to any of your post or page titles.
3. Assign that menu to the primary location
4. Create a new post, switch to the code editor and paste the following:

<!-- wp:navigation {"__unstableLocation":"primary"} /-->

5. Preview the post, it should display the menu with the memorably labeled item from step 2.

Testing that __unstableLocation is disabled in core

1. Following on from the previous steps, comment out the following line in lib/load.php:

define( 'IS_GUTENBERG_PLUGIN', true );

2. Preview the post again, the nav block should fall back to showing the Page List by default.

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[noisysocks]

I like the approach. It's simple, analogous to an existing concept we have (GUTENBERG_PHASE), and I think it will prove useful. We've had at least two cases now of functionality in a block's index.php which we want to exist in the plugin but not in Core.

I'd like more opinions from other @WordPress/gutenberg-core members and @hellofromtonya.

Instead of a PR review, and because I like the sound of my fingers typing, you get a collection of my disparate thoughts on this approach:

• I think we should think about making GUTENBERG_PHASE and IS_GUTENBERG_PLUGIN have the same name so that there is parity between the two concepts. In practice, GUTENBERG_PHASE is always used to determine if the plugin is installed, so I think it would be best to rename GUTENBERG_PHASE to IS_GUTENBERG_PLUGIN.

• The PHP code wrapped in if ( defined( 'IS_GUTENBERG_PLUGIN' ) ) { ... } will still be shipped to Core. This is different to how it works with JavaScript where if ( process.env.GUTENBERG_PHASE === 2 ) { ... } is removed with tree shaking. I don't think this is a problem for two reasons:

  • There is no significant downside to distributing unnecessary PHP code, unlike JavaScript where extraneous code increases the time it takes to download and parse the bundle.

  • Brave WordPress users could, in theory, define this constant in their wp-config.php and get access to experimental new features. (In practice there would probably be errors.) I think it's nice to let people tinker if they want to.

[talldan]

I think we should think about making GUTENBERG_PHASE and IS_GUTENBERG_PLUGIN have the same name so that there is parity between the two concepts. In practice, GUTENBERG_PHASE is always used to determine if the plugin is installed, so I think it would be best to rename GUTENBERG_PHASE to IS_GUTENBERG_PLUGIN.

Yeah agreed, this is something I already planned to do at some point.

[ockham]

If I'm not mistaken, this is a similar problem like we've had before when backporting GB (and specifically, block specific) PHP code to Core. E.g. WordPress/wordpress-develop#1796 backported the FSE infrastructure, which includes the core/template-part block.

We solved the rename issue in GB by

• moving the relevant functions to files in lib/compat/wordpress-5.9/,
• dropping the gutenberg_ prefix both from function definitions and callsites so the functions would have the same names as in Core,
• and guarding the function definitions with if ( ! function_exists( 'my_function' ) ) { clauses.

For an example, see #36180 and #36201.

Is this an approach that could be applied here, too? It'd be nice to consistently use the lib/compat/-layer based approach across different parts of the codebase 🙂

[noisysocks]

Is this an approach that could be applied here, too? It'd be nice to consistently use the lib/compat/-layer based approach across different parts of the codebase 🙂

The difference here is that the block PHP (/packages/block-library/src/navigation/index.php) which is used both in Core and in Gutenberg has calls to functions that exist in Gutenberg but not Core. I suppose we could wrap the calls in if ( function_exists( ... ) { ... }? And then move the definitions to lib/compat/experimental since we don't know what WordPress version this functionality will land in?

[adamziel]

Test failures seems unrelated, I just rebased – let's see what happens.

[adamziel]

The PHP Unit Tests failures in trunk are exactly the same as in this PR. These functions still use the legacy attribute name so I'll rename them – otherwise it looks good. The functions here are only intended to be called in the Gutenberg plugin context, and so it introduces a IS_GUTENBERG_PLUGIN constant that enables distinguishing between the two situations. I can't see any better way to do that. 👍

[hellofromtonya]

I like the approach. It's simple, analogous to an existing concept we have (GUTENBERG_PHASE), and I think it will prove useful. We've had at least two cases now of functionality in a block's index.php which we want to exist in the plugin but not in Core.

I'd like more opinions from other @WordPress/gutenberg-core members and @hellofromtonya.

I'm sorry for my lateness in responding.

👍 I like the approach of guarding these Gutenberg-only functions by wrapping them within a check that specifically checks for the plugin being activated. A constant is one approach that works well, i.e. defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN. This implementation means WordPress Core without the Gutenberg plugin activated, this code will be skipped.

⚠️ Caution: For WordPress 5.9+, make sure these functions are not loaded into memory by the Gutenberg plugin. Why? Fatal error will happen if the plugin also attempts to load them as functions cannot be redeclared.

👍 I also like the approach of removing the gutenberg_ prefix as it avoids confusion for contributors maintaining WordPress Core. Avoiding confusion can mitigate effort.

[ockham]

Is this an approach that could be applied here, too? It'd be nice to consistently use the lib/compat/-layer based approach across different parts of the codebase 🙂

The difference here is that the block PHP (/packages/block-library/src/navigation/index.php) which is used both in Core and in Gutenberg has calls to functions that exist in Gutenberg but not Core. I suppose we could wrap the calls in if ( function_exists( ... ) { ... }? And then move the definitions to lib/compat/experimental since we don't know what WordPress version this functionality will land in?

Thanks for elaborating @noisysocks! Right, I think the part that was lost on me is that this is for an optional/__unstable feature 🤔

I agree that it doesn't make sense to backport these to Core if it's not sure that we eventually want them there (so I'd rather recommend against the callsite-guarding and lib/compat/experimental approach). But if we're certain enough that however unstable the feature itself is, we'll still need those functions, then it might be worth considering backporting them to Core already... 🤔

[talldan]

Caution: For WordPress 5.9+, make sure these functions are not loaded into memory by the Gutenberg plugin. Why? Fatal error will happen if the plugin also attempts to load them as functions cannot be redeclared.

I might be wrong, but I think there's something about the block library that means this isn't required. There are already quite a lot of block functions that exist in core and don't require guarding in the Gutenberg plugin.

I'm not entirely sure how or why that works.

[noisysocks]

Caution: For WordPress 5.9+, make sure these functions are not loaded into memory by the Gutenberg plugin. Why? Fatal error will happen if the plugin also attempts to load them as functions cannot be redeclared.

I might be wrong, but I think there's something about the block library that means this isn't required. There are already quite a lot of block functions that exist in core and don't require guarding in the Gutenberg plugin.

I'm not entirely sure how or why that works.

That's correct. When block PHP is processed for use in the Gutenberg plugin, all PHP function names in block-library/src/*/index.php are prefixed with gutenberg_ to avoid conflicts with the same functions that exist in Core.

gutenberg/tools/webpack/blocks.js

Lines 123 to 131 in e96d47b

	// Prepend the Gutenberg prefix, substituting any
	// other core prefix (e.g. "wp_").
	return result.replace(
	new RegExp( functionName, 'g' ),
	( match ) =>
	'gutenberg_' +
	match.replace( /^wp_/, '' )
	);
	}, content )

[talldan]

Thanks for the feedback so far everyone. I'll keep this open until Monday or Tuesday next week to see if there's any more feedback, but if there isn't any will move ahead with merging.

[noisysocks]

I rebased this to (hopefully) fix failing tests.

[noisysocks]

@hellofromtonya: This was already backported. You can tell by looking for a commit in wp/trunk that references the PR. It shows up in GitHub above.