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

Add filters to shortcircuit the public API of global styles #36909

Closed
wants to merge 1 commit into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 26, 2021

A follow-up to #34843 #36610 #36907

This PR introduces a shortcircuit filter for each one of the public functions we're introducing for global styles in WordPress 5.9.

It works this way (pseudo-code):

function wp_get_global_* ( ... ) {
  $pre_global_* = apply_filters( 'pre_wp_get_global_*', null, ... );
  if ( null !== $pre_global_* ) {
    return $pre_global_*;
  }

  // Normal operation.
  // ...
}

By doing this, we allow the Gutenberg plugin (and any 3rd party) to modify the behavior of these functions once they are in WordPress core. This is useful especially for Gutenberg, where we may need the ability to introduce future changes to how these functions work. Examples of changes that we're considering: support for multiple theme.json files ("global styles variations"), new schema versions, etc.

It's inspired by the pre_render_block filter.

@oandregal oandregal changed the base branch from trunk to update/gs-api-name November 26, 2021 11:30
@oandregal oandregal self-assigned this Nov 26, 2021
@oandregal
Copy link
Member Author

We've been wary of introducing filters for the internals of the global styles lifecycle and its data structure. The rationale is that the API needs room for consolidation/maturation with production feedback. I still think we should not introduce that kind of filters.

However, now that we have an official public API to get data from the theme.json we need the ability to update how these functions work to add the future behavior we're already planning. Adding filters to short-circuit them seems a safe first step to balance extensibility and API evolution.

@oandregal oandregal added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json 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 Nov 26, 2021
Base automatically changed from update/gs-api-name to trunk November 26, 2021 11:45
@oandregal oandregal force-pushed the add/shorcircuit-filters-to-gs-api branch from d29d8e1 to c179f2b Compare November 26, 2021 11:47
@oandregal
Copy link
Member Author

Rebased as the branch this was based on #36907 has landed and, apparently, the automated change of base branch that GitHub didn't work as expected.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Provided the functions are being part of the core, I think this short-circuit behavior is a good way to allow us to change things on the plugin.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Why a "pre" and not a "post" filter. In other places in FSE we have post filters I believe.

@youknowriad
Copy link
Contributor

See

return apply_filters( 'get_block_template', $block_template, $id, $template_type );

@oandregal
Copy link
Member Author

oandregal commented Nov 26, 2021

Why a "pre" and not a "post" filter. In other places in FSE we have post filters I believe.

I thought in this case it'd be more convenient for the kind of things we may want to do: not modifying internal data that is pre-calculated by WordPress core but short-circuiting core implementation altogether. For example, useful if the $path parameter is something core doesn't recognize or if the $context object has info about new data sources, etc. Essentially, we want Gutenberg to be the canonical implementation and ignore core's implementation (doesn't run twice so it's more performant, etc).

@youknowriad
Copy link
Contributor

That reasoning is sound but in general I feel like the general guidelines (probably not everywhere) for core filters has been to filter a value that is computed by Core. Granted that it's less performant though.

@youknowriad
Copy link
Contributor

I actually think see that there's a lot of places in core where there's both but can't stop but think that a "post" should be the default.

@oandregal
Copy link
Member Author

What do you think of the filter names? In this case, given these are for short-circuiting core's implementation of a particular function, I'd think it's best they are pre_{name_of_function}.

I'm less opinionated about the actual names of the functions (whether or not to use the wp_* prefix). I've noticed that there're some functions and hooks in core that use the wp infix. Haven't found any rules of guides for this.

@youknowriad
Copy link
Contributor

The naming itself make sense for pre filters, I just wonder about the consistency and whether "pre" make sense without "post" (without the actual post_ in the filter name)

@oandregal
Copy link
Member Author

Inspecting a bit my gut feeling for a "pre" filter and against a "post" filter and this is what I came up with. This is the usage of these functions in WordPress core:

  • wp_get_global_styles, not used in core yet. It's used in Gutenberg for exposing data to mobile.

  • wp_get_global_settings, in the code to load the __experimentalFeatures into block editor settings and some supports to query theme.json data. The data this is supposed to return is "experimental", so I'd lean towards not allowing to filter it until it's not marked as mature. There's already the block editor settings filter, so people can already do a "post" filter.

  • wp_get_global_stylesheet, in the code to load the styles for the editor and to generate the global-styles-inline-css stylesheet for the front-end. This function returns a string, so a "post" filter maybe is not that useful (have to parse the string, etc) and it'd be perhaps most interesting to be able to hook into settings/styles this function uses to generate the actual styles. Too low-level at the moment, in my view.

By doing this, we allow both the Gutenberg plugin and any 3rd party
to change the behavior of these functions. This is useful specially
for Gutenberg, where we may need to introduce future changes so
we need a way to hook into the core functions.

It's inspired by the pre_render_block filter.
@oandregal oandregal force-pushed the add/shorcircuit-filters-to-gs-api branch from c179f2b to 9ae37db Compare November 26, 2021 13:53
@oandregal
Copy link
Member Author

(rebased for the CI jobs to restart, it seems they didn't like the changing of base branches)

@oandregal
Copy link
Member Author

Also: if there was a "post" filter for settings/styles in theme.json, consumers would have the reasonable expectation that it'd affect every place where we use them, which I don't think it'd be the case at the moment. There're still places that use the low-level/internal API (rest endpoints, wp_get_global_stylesheet, etc.). I'd argue for migrating them and consolidating all the pieces before creating that type of filtering.

@youknowriad
Copy link
Contributor

the reasonable expectation that it'd affect every place where we use them, which I don't think it'd be the case at the moment.

That makes sense to me but for me, I have the same expectation but I also have the same expectations for pre_ filters.

@youknowriad
Copy link
Contributor

the reasonable expectation that it'd affect every place where we use them, which I don't think it'd be the case at the moment.

I've been thinking more about this, it seems this expectation is almost impossible to meet. Let me explain:

  • The editors and the frontend in general is going to rely on endpoints to retrieve these settings and styles
  • If we do want these to take into account the filters proposed here, the endpoints need to be rewritten to call these functions for all keys... It seems highly unlikely to be achievable (as we don't know all what's defined all potential calls for these functions)

In other words, this lead me to think, that these filters here are not good filters or at least they might be good for the Gutenberg plugin for potential future behavior change for these functions but that's it. So maybe internal filters is better for now or no filters (and just build new functions and use new functions in Gutenberg when the time comes to introduce new behavior)

@oandregal
Copy link
Member Author

Discussed our options with Riad.

This approach:

  • Pro: this approach would allow us to modify the current behavior while retaining the same function name for all consumers that use it.
  • A con of this approach is that the current filters are thought to be used by Gutenberg to re-implement these functions, should we need it, and that they don't allow other consumers to modify global styles behavior in all places as the functions are not yet extensively used.

No filters:

  • Pro: we don't have a baggage to support down the road when/if the API changes, giving us more flexibility.
  • Con: by not doing this, adding new behavior in Gutenberg requires re-implementing the functions and rewriting all core behavior that uses them (not a lot so far, and we are already rewriting it).

All in all, we decided to go with no filters so far. It's the hardest option for us (Gutenberg) but it's more consistent with how WordPress core works: when we add filters they should let consumers filter every place the global styles settings/styles/stylesheet are used.

@oandregal oandregal closed this Nov 29, 2021
@oandregal oandregal deleted the add/shorcircuit-filters-to-gs-api branch November 29, 2021 11:57
@oandregal oandregal 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 Nov 29, 2021
@oandregal
Copy link
Member Author

@youknowriad @jorgefilipecosta I've run into a couple of places that add changes targeted to WordPress 6.0: webfonts API by @aristath and axial block gap support by @ramonjd @andrewserong

I'm thinking that we already need to bootstrap something for 6.0, so people can continue working on GS features slated for 6.0 while the WordPress 5.9 is still ongoing.

@oandregal
Copy link
Member Author

Related: allow users to specific custom filters #38055 (comment) and #38150

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants