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

Fix gutenberg prefixed function references in core #37021

Merged
merged 8 commits into from
Dec 6, 2021

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Dec 1, 2021

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"} /-->
  1. 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 );
  1. 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).

@talldan talldan added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Dec 1, 2021
@talldan talldan self-assigned this Dec 1, 2021
@noisysocks
Copy link
Member

noisysocks commented Dec 1, 2021

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

talldan commented Dec 1, 2021

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

ockham commented Dec 1, 2021

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
Copy link
Member

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 adamziel force-pushed the fix/gutenberg-prefixed-functions-in-core branch from 4eef46c to b2b5bb9 Compare December 2, 2021 13:37
@adamziel
Copy link
Contributor

adamziel commented Dec 2, 2021

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

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

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

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

ockham commented Dec 2, 2021

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

talldan commented Dec 3, 2021

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
Copy link
Member

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.

// 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 talldan force-pushed the fix/gutenberg-prefixed-functions-in-core branch from 934099c to 6ae1420 Compare December 3, 2021 04:46
@talldan talldan requested a review from a team December 3, 2021 04:52
@talldan
Copy link
Contributor Author

talldan commented Dec 3, 2021

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.

@talldan talldan force-pushed the fix/gutenberg-prefixed-functions-in-core branch from 6ae1420 to f334127 Compare December 3, 2021 06:52
@noisysocks noisysocks force-pushed the fix/gutenberg-prefixed-functions-in-core branch from f334127 to 6610305 Compare December 6, 2021 01:17
@noisysocks
Copy link
Member

I rebased this to (hopefully) fix failing tests.

@talldan talldan merged commit b59c37b into trunk Dec 6, 2021
@talldan talldan deleted the fix/gutenberg-prefixed-functions-in-core branch December 6, 2021 02:14
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 6, 2021
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 6, 2021
noisysocks pushed a commit that referenced this pull request Dec 6, 2021
* Remove navigation area preloading

* Remove navigation area theme switching functionality

* Only run if statement in the gutenberg plugin

* Move functions to nav block php

* Gate function defintions using an if statement

* Fix incorrectly named function

* Rename navigationMenuId to ref

* Also check value of `IS_GUTENBERG_PLUGIN`

Co-authored-by: Tonya Mork <hello@hellofromtonya.com>

Check value of `IS_GUTENBERG_PLUGIN`

Co-authored-by: Tonya Mork <hello@hellofromtonya.com>

Co-authored-by: Adam Zieliński <adam@adamziel.com>
Co-authored-by: Tonya Mork <hello@hellofromtonya.com>
@hellofromtonya hellofromtonya added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 7, 2021
@noisysocks
Copy link
Member

@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.

@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 gutenberg_-functions called in wp-trunk (blocks/navigation.php)
6 participants