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

Added filters to get block templates functions. #31806

Merged
merged 13 commits into from
Jun 23, 2021
Merged

Conversation

janw-me
Copy link
Contributor

@janw-me janw-me commented May 13, 2021

Allow short circuit finding of block templates before they start
And allow to filter the block templates found before returning.

Description

I'm planning to work on a plugin that handles the templates differently.
And with these changes plugins would be able to change the source of the templates. Or change the output of the block template(s).

How has this been tested?

Only apply_filters are added.
I've done simple test local and they work. If more test are required how should these be handled?

Types of changes

To change the source of the block templates I've added 3 "before" filtes. Based like WP_Query does it.

But even if the source isn't overriden and the normal gutenberg process is run, the found block output should be filterable.
Those 3 filters are also modeled after WP_Query.

I've only touched three functions those appear to be part of the public API. All other functions are prefixed with a _.
That might not prove enough in the future. But from what I expect this will do for now.

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

  • I've included developer documentation if appropriate.

My Questions:

  1. Are the filter names logic?
  2. Where should these filters be documented? The handbook filters appear mostly JS focused. I did add the appropriate php filter docblock.

@mkaz can you review?

Allow short circuit finding of block templates before they start
And allow to filter the block templates found before returning.
@janw-me
Copy link
Contributor Author

janw-me commented May 13, 2021

I'm not sure how to read the test error but it appears to be block output.
I highly doubt this is caused by my change. Can Anybody help me sort this out?

@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Templates API Related to API powering block template functionality in the Site Editor labels May 14, 2021
@draganescu draganescu added [Block] Template Part Affects the Template Parts Block and removed [Block] Template Part Affects the Template Parts Block labels May 26, 2021
@janw-me
Copy link
Contributor Author

janw-me commented May 27, 2021

As suggested in yesterday's Core Editor Meeting I'm providing the usecase for this PR.

TLDR

I want to create a plugin where the templates are stored and loaded from somewhere else other then in the post table or in the theme files (or a combination).
This PR allows to short-cicuit the loading of templates.
For the CRUD there are already the save_post hooks and the like. I need the loading.
Stricly speaking the 3 pre_get_** filters should be enough.
But there might be use cases for the return apply_filters in the future.

The full sales pitch

One of the hardest parts when developing in WordPress with several environments is the database. When creating a new or updating a FSE template the changes are stored in the database. There is a feature in gutenberg that will export all templates. But that requires extra steps for every adjustment.

My idea is to save/update/delete every template(part) on every save/update/delete form the wp-admin a seperate directery. This directory can then be put in git/svn/ftp/whatever.
This would also mean the that directory is the source of the templates, not the mix of the theme and the posts table.

Currently the DB takes priority over the theme folder, which is logical by default.
So if Developer A and B both work on template C. They each need to export the whole theme everytime and clear the DB of templates. Otherwise they will not see the updated template, just there own DB version of template C.
This will be a very fiddly process.

With the plugin I'm trying to create if Developer A and B both work on template C. When they are done they will just commit the changes like normal. Without extra steps of exporting or touching the DB.

I know this will open a can of worm about dynamic parts like hardcoded ID's in nav menu's but that's a different problem for which I have a couple of idea's.
There are a few other challenges but one step at the time.

I hope this clears up any doubts about the usefullness of this PR.
Questions? Suggestions? Please let me know!

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @janw-me!

I think these filters are a powerful way to add some flexibility here. For the most part this seems like the standard way they would be applied to functions like this and its looking pretty good. I added a couple comments below, but other than those I think this makes sense.

I wonder if others have any concerns regarding these filters? I remember there have been some concerns with adding/supporting more filters in the past, but also a totally different scenario. cc @youknowriad @TimothyBJacobs

lib/full-site-editing/block-templates.php Outdated Show resolved Hide resolved
* }
* @param array $template_type wp_template or wp_template_part.
*/
return apply_filters( 'queried_block_templates', $query_result, $query, $template_type );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name here should correspond to the same name as the pre_* filter above. So we would have pre_get_block_templates and get_block_templates (similar to as you did in the functions below).

Also, im not completely certain, but I wonder if we should be adding the gutenberg prefix to these filters the same as they are in the functions they are contained in? Like pre_gutenberg_get_block_templates / gutenberg_get_block_templates etc? It would be a little more specific to their use and hopefully less potential of conflict 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said, I'm also a bit unsure about the names of these filter names, and this one the most.

Searching the project for filters with a gutenberg_ prefix I only found 1. Blocks templates themself are pretty new territory, so I doubt there would be conflicts.

I'll change it to get_block_templates for at least now, if anybody has a better suggestion (for any of the filters) I'll change it again.

@TimothyBJacobs
Copy link
Member

I wonder if others have any concerns regarding these filters? I remember there have been some concerns with adding/supporting more filters in the past, but also a totally different scenario. cc @youknowriad @TimothyBJacobs

I have no concern with this personally, but I can't speak to whether GB will need flexibility in the future that may be more challenging due to making this function filterable.

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Jun 16, 2021

but I can't speak to whether GB will need flexibility in the future that may be more challenging due to making this function filterable.

Im not sure here either. The code makes sense to me, but I'l defer to @youknowriad and others from the core team regarding the above concern.

@mcsf
Copy link
Contributor

mcsf commented Jun 16, 2021

I have no concern with this personally, but I can't speak to whether GB will need flexibility in the future that may be more challenging due to making this function filterable.

Yes, I think this is the main thing.

@janw-me: this change is easier to make if we don't yet make a commitment to support these filters in the long term. Are you open to adding them only in the plugin (we would have to update the lines saying @since 5.8 to match the plugin version instead) while we get a feel for these filters? That way, we'd reserve the right to deprecate them before getting them in Core. Gutenberg's template infrastructure is young and it's likely that we will want to make architectural changes as we progress.

@janw-me
Copy link
Contributor Author

janw-me commented Jun 16, 2021

Are you open to adding them only in the plugin

In my head the best case scenario these filters will be in Gutenberg 10.8 & WP 5.9
I already assumed WP 5.8 has sailed. And that's fine.
So yes I'm okay to keep it in the plugin for now.

I'll update the since versions to Gutenberg 10.8

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looks good and consistent, let's see how it fares.

@mcsf mcsf merged commit c9950a3 into WordPress:trunk Jun 23, 2021
@github-actions github-actions bot added this to the Gutenberg 11.0 milestone Jun 23, 2021
@janw-me
Copy link
Contributor Author

janw-me commented Jun 24, 2021

Thank's really appreaciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Templates API Related to API powering block template functionality in the Site Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants