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

Update global styles public API #36610

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Conversation

oandregal
Copy link
Member

This simplifies the public API of the newly introduced methods in WordPress 5.9. It's based on feedback received as per #36556 and future improvements (such as having multiple theme.json files).

How to test

  • Verify that tests still pass.
  • Load the front and editors (post, site) and verify that it works as expected (presets and styles are loaded, etc).

@oandregal oandregal self-assigned this Nov 18, 2021
@oandregal oandregal added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 18, 2021
@skorasaurus
Copy link
Member

Maybe worth mentioning in a separate issue; but do we also want to keep the gutenberg_ prefix at the beginning of the functions?

@mtias
Copy link
Member

mtias commented Nov 18, 2021

All of those should be removed with the build for core.

@mcsf
Copy link
Contributor

mcsf commented Nov 19, 2021

Edit: disregard my comment, as this only applies to block-library.

[…] but do we also want to keep the gutenberg_ prefix at the beginning of the functions?

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

function gutenberg_get_global_settings( $path = array(), $block_name = '', $origin = 'all' ) {
if ( '' !== $block_name ) {
$path = array_merge( array( 'blocks', $block_name ), $path );
function gutenberg_get_global_settings( $path = array(), $context = array() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want function_exists checks for core?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need them as the current naming stands, but I discussed this with Miguel and I'm going to check how do we want this public API to operate (see). I'll post as a comment any follow-up PR.

@mcsf
Copy link
Contributor

mcsf commented Nov 22, 2021

Per our private conversation, this PR looks good in terms of API changes. As for the naming of the function so as to operate across plugin and core, that's another question. The two main opposing methods are:

  1. Keeping Core functions as the canonical ones, loaded first, with filters in them that allow the plugin to short-circuit them.
  2. Keeping plugin functions as the canonical ones, loaded first, then wrapping the Core definitions with if ( ! function_exists guards.

@oandregal oandregal merged commit 1ed3af0 into trunk Nov 22, 2021
@oandregal oandregal deleted the update/global-styles-public-api branch November 22, 2021 11:24
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 22, 2021
@oandregal
Copy link
Member Author

I've leveled up my understanding of how lib/compat/* code should work, and I'm proposing a follow-up to this at #36907

@oandregal
Copy link
Member Author

The second follow-up I thought we need is introducing short-circuit filters to be able to add new behavior from the plugin #36909

@oandregal
Copy link
Member Author

We decided against the filters. Rationale at #36909 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants