Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#46187 closed enhancement (fixed)

Add a dynamic hook to filter the content of a single block.

Reported by: manzoorwanijk's profile manzoorwani.jk Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests commit has-dev-note
Focuses: Cc:

Description

render_block filter in render_block() is very helpful. It would be good to have a dynamic hook (render_block_{$block_name}) to filter only the content of a specific block, to avoid the need of using the conditionals inside the filter callback.

register_block_type() does provide an option to pass render_callback but still that's not useful when you are not the creator of the block.

Attachments (4)

46187.diff (868 bytes) - added by manzoorwani.jk 6 years ago.
46187-1.diff (1.5 KB) - added by manzoorwani.jk 6 years ago.
46187-2.diff (1.0 KB) - added by manzoorwani.jk 4 years ago.
46187.2.diff (2.3 KB) - added by noisysocks 4 years ago.

Download all attachments as: .zip

Change History (19)

@manzoorwani.jk
6 years ago

#1 @birgire
6 years ago

  • Keywords has-patch added

Thanks for the ticket @manzoorwanijk

At a first glance this sounds useful.

Using a dynamic filter, we should make sure that the block names can't contain characters that are not supported as filter names.

Few questions come to mind: What limitations are on the block names and filter names? For example what is the maximum length of filter names and block names? Is it the same?

Regarding the if-condition in 46187.diff, should e.g. "0" be allowed, that is if "0" is a possible block name?

If this goes through, then it would need a @since x.y.z for the the filter documentation.

There seems to be some code standard adjustments needed in 46187.diff :

  • I think the variable assigning within the if-condition is not recommended.
  • Missing end of line periods in inline documentation.
  • Fix indentation.
  • First word in inline documentation not capitalized.

Should the specific dynamic filter come before or after the existing and more general render_block filter? I didn't find quickly some good existing core examples for the case of filters, but we have e.g. the dynamic action "save_post_{$post->post_type}" running before "save_post". In that case the more specific comes before the general one. This is the same order as in 46187.diff.

#2 follow-up: @manzoorwani.jk
6 years ago

Thank you for the valuable inputs @birgire

I have made the changes pointed out by you and also moved the dynamic filter after the general filter which seems to be a better place for a dynamic filter unlike hooks, couldn't find an example from the core.

Also, about the number of characters in a filter tag, I found your own answer 🙂

As far as the allowed characters in a filter are concerned, WordPress is stricter when it comes to a block name, so it should be pretty fine to use a block name in a dynamic filter, provided / between namespace and block is not a problem.

#3 in reply to: ↑ 2 @birgire
6 years ago

Replying to manzoorwani.jk:

lol, thanks for the link 🙂

#4 @noisysocks
4 years ago

  • Keywords close added

Hey, thanks for the patch. What's the benefit of a dynamic filter name as opposed to using render_block and checking $block['blockName']?

In my opinion it is not worth doing this given that there is a sensible alternative and given that it is confusing that the dynamic part of the filter name would nearly never match the block's name, e.g. core/image would have to become core_image.

#5 @manzoorwani.jk
4 years ago

@noisysocks benefit of that dynamic filter is written in the description - i.e. to avoid unnecessary if conditions inside the filter callback and also to avoid calling the callback on every single block.

Also, why core/image cannot work in a filter name? Doesn't it simply become an array index?
FYI, / is pretty fine for array indices.

#6 @noisysocks
4 years ago

Yes, / is a valid character in an array index but it doesn't fit with the WordPress naming conventions for filters.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

Another problem if we do this is that it will not be intuitive which filter (render_block vs render_block_$block) is called first. It will also not be possible for developers to use the $priority param in add_filter to e.g. make their render_block_$block filter fire before or after another plugin's render_block filter.

I don't personally think that the convenience is worth the added ambiguity.

#7 @manzoorwani.jk
4 years ago

Well, none of those arguments is valid.

You need to follow the coding standards in the code you write and you are not responsible for the dynamic part of the filter. An example of that is save_post_{$post->post_type}, where post_type can have some characters like - which apparently are not as per the coding standards for filter names, although there is nothing mentioned about filter names in WPCS. Another example is admin_post_{$action}, where I can write anything as $action, including /.

Regarding the order of dynamic and normal filter, aren't there tens of such dynamic filters in WordPress?? Is there any standard for order of such filters? Example is again save_post vs save_post_{$post_type}.

Last edited 4 years ago by manzoorwani.jk (previous) (diff)

#8 @noisysocks
4 years ago

  • Keywords 2nd-opinion added

#9 @johnbillion
4 years ago

  • Keywords close 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.7

This is a fairly common pattern in core, see for example: gettext/gettext_{$domain}, plugin_action_links/plugin_action_links_{$plugin_file}, manage_posts_columns/manage_{$post_type}_posts_columns, etc. The dynamically named filter should come after the generic one.

#10 @noisysocks
4 years ago

  • Keywords needs-refresh added

Hey @manzoorwanijk, could you please refresh the patch? It no longer applies cleanly.

#11 @manzoorwani.jk
4 years ago

@noisysocks Since that filter has been moved to wp-includes/class-wp-block.php, I have refreshed the patch. Thanks

@noisysocks
4 years ago

#12 @noisysocks
4 years ago

  • Keywords has-unit-tests commit added; needs-refresh removed

Thanks @manzoorwanijk! 46187-2.diff looks good to me. I tested it by writing a unit test which is in 46187.2.diff.

#13 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 50123:

Editor: Introduce a dynamic filter for the content of a single block:

render_block_{$this->name}

This complements the existing render_block hook and allows for filtering the content of a specific block without having to use conditionals inside the filter callback.

Props manzoorwani.jk, noisysocks, birgire, johnbillion.
Fixes #46187.

#14 @SergeyBiryukov
4 years ago

  • Keywords needs-dev-note added

Marking for a dev note for the new filter.

Note: See TracTickets for help on using tickets.