Block Hooks: Mark and document publicly available global functions as being Core-only internal functions.

[hellofromTonya]

This ticket is a follow-up to the ​discussion in Make/Core slack core channel today.

​For my assessment which @bernhard-reiter confirmed

For now, seems the code is not intended for extensibility, but rather is a low-level internal Core only functionality.

The new functions, such as traverse_and_serialize_blocks() et all, are low-level Core-only functionality that should not be used outside of Core.

It's too late in the 6.4 cycle for an architectural redesign to "hide" (encapsulate) the internal functionality (though will be considered in a follow-up ticket for a future release).

For new functions that are not currently denoted as Core-only, such as traverse_and_serialize_blocks(), ​this ticket proposes:

• Marking them with @access private.
• Adding an explanation to the DocBlock to better document they are intended for Core only usage.
• Consider renaming with the private _ prefix to further denote these are for Core only.

Note: Core-only includes further iteration and refinement in Gutenberg, which will come to Core.

[Bernhard Reiter]

FWIW, I couldn't think of the proper functions to compare this to during our earlier Slack convo with @azaozz, but I'd liken traverse_and_serialize_blocks() to PHP's data structure traversal functions, such as array_filter(), or, perhaps most similarly, ​`array_map()`.

Much like the latter, traverse_and_serialize_blocks() takes a data structure (in our case, a block tree), traverses it, and applies a callback to each element it finds.

Granted, traverse_and_serialize_blocks() is somewhat more domain-specific as it is traversing a tree of blocks rather than a more generic data structure; but within the domain of blocks and block trees, its applications are arguably comparably wide-ranging as array_map() is for arrays.

For a generic traversal tool like that, filters didn't seem like the right fit, much like one wouldn't want to add a filter before invoking array_map instead of passing a callback to it (and remembering to remove the filter afterwards!) While it's really only used inside of Core for now, it's conceivable that traverse_and_serialize_blocks() could be of some use to extenders.

It seems like the major qualm about traverse_and_serialize_blocks() using a callback argument (rather than a filter) is that people might the function multiple times with different callbacks, which will negatively impact performance, as traversal and serialization is a potentially costly operation, especially for large block trees. I'll just note that the same applies to array_map(); a simple way to avoid running it multiple times for different callbacks is to compose one function from all callbacks that need to be applied, and pass that as the argument. This is something we could also add to the function's PHPDoc, if needed.

Anyway, if folks feel that it should be marked as @access private and documented to be Core use only, I'm happy to oblige. I'd be also happy if we could maybe at least avoid the _ prefix :)

Change the PHPDoc for traverse_and_serialize_block and traverse_and_serialize_blocks to:

• Mark them as with @access private.
• Add an explanation to the DocBlock to better document they are intended for Core only usage.
• Consider renaming with the private _ prefix to further denote these are for Core only.

Trac ticket: https://core.trac.wordpress.org/ticket/59783

[azaozz]

Replying to Bernhard Reiter:

Thanks for the explanation!

traverse_and_serialize_blocks() (is similar) to PHP's data structure traversal functions, such as array_filter(), or, perhaps most similarly, ​`array_map()`.

Yes, you're right, I see that.

For a generic traversal tool like that, filters didn't seem like the right fit, much like one wouldn't want to add a filter before invoking array_map instead of passing a callback to it

Right. Thinking this is more a question of "PHP built-in functions that use callbacks" vs. WordPress functions that use callbacks (filters). Broadly speaking a WP filter is a method to execute multiple (named) callbacks at approximately the same time. Nothing more. They are still callbacks but somewhat dynamic and with more functionality. The WP filters make it very convenient for plugins to "hook in" and change how the core code operates at many places. And this is not limited to just one plugin at a time. Another advantage is that these callbacks can also be removed.

Unfortunately array_map() and similar cannot run multiple callbacks. They simply do not have that functionality. Well, that can be "fixed" in WP by something like this if it's ever needed (untested code):

function wp_array_map( $filter_name, $array ) {
    $array = array_map(
        function( $element ) use ( $filter_name ) {
            return apply_filters( $filter_name, $element );
        },
        $array
    );

    return $array;
}

It seems like the major qualm about traverse_and_serialize_blocks() using a callback argument (rather than a filter) is that people might the function multiple times with different callbacks, which will negatively impact performance, as traversal and serialization is a potentially costly operation

Yep, this is one of the concerns if traverse_and_serialize_blocks() and friends are intended for use by extenders. Another is that using (static) callbacks for APIs that are intended to be used by plugins is not a good practice in WP. Filters are much more convenient and promote interoperability between (multiple) plugins and core.

On the other hand if traverse_and_serialize_blocks() and friends are only intended for use by core code (as for now), it would be better to make them truly "private". In PHP there are several ways to do that, perhaps most common is to make them private methods of a final class. As @hellofromTonya mentioned on Slack, this seems like a better code design pattern for the requirements and I hope the existing functions can be deprecated in 6.5 and replaced with it.

[gziolo]

I just wanted to raise traverse_and_serialize_blocks is an improvement over what was available in WordPress core before. A good example is the deprecated in WP 6.4 _inject_theme_attribute_in_block_template_content method that was calling parse_blocks + _flatten_blocks and applying direct modifications to the internal in-memory block representation before serializing it back to HTML. In fact, it's also the same approach that Bernie used to implement Block Hooks in the Gutenberg plugin, example:

​https://github.com/WordPress/gutenberg/blob/6f46fe824ffe2053890a073213b49a3aea8ae772/lib/compat/wordpress-6.4/block-hooks.php#L252-L253

traverse_and_serialize_blocks is an attempt to simplify that processing in WordPress core and to consolidate modifications necessary to the blocks serialized as HTML more formally. We didn't discuss opening this to extenders so marking it as private is a good middle ground for now.

From my perspective, the biggest challenge is that these callbacks are passed down through recursive calls of traverse_and_serialize_block when processing also inner blocks. I'd be happy to discuss ideas on how we could refactor the existing code to use filters instead. The simplest way to approach it I can think of would be setting filters inside traverse_and_serialize_blocks so anyone could extend the functionality of callbacks:

<?php
function traverse_and_serialize_blocks( $blocks, $pre_callback = null, $post_callback = null ) {

    $pre_callback   = apply_filters( 'traverse_blocks_pre', $pre_callback );
    $post_callback = apply_filters( 'traverse_blocks_post', $post_callback );

    /// Function body continues...
}

However, this way, you don't have access to additional information that we pass to block visitors, like the list of all hooked blocks (probably irrelevant for extenders), but the information about the processed template or block pattern might be helpful.

I'm sure there are many alternative ways it could be refactored, so what I shared is more of a conversation starter. The most important question is whether this should be opened to extenders, knowing that we use the same processing every time we need to inject hooked blocks. Today, it's only for templates, template parts, and block patterns shipped with the theme, but we are about to apply the same mechanism to the templates and patterns modified by users and stored in the database.

[kirasong]

Replying to gziolo:

<snip>
We didn't discuss opening this to extenders so marking it as private is a good middle ground for now.

As we're getting closer to freeze, I wanted to leave a quick note that I think this makes sense for 6.4 and am +1 with commit and backport.

There seems to be consensus around this approach as a compromise, and it'll give more time for the conversation about implementation + access to happen.

[azaozz]

Replying to gziolo:

traverse_and_serialize_blocks is an attempt to simplify that processing in WordPress core and to consolidate modifications necessary to the blocks serialized as HTML more formally.

Yep, that sounds good. Seems it may be repeating some similar (or the same) functionality that already exists in render_block()?

Another thing that seems unclear is what is it intended for and how it is supposed to be used in combination with or instead of the existing functionality like serialize_block() (which is not supposed to be used on the front-end according to the docs, https://core.trac.wordpress.org/browser/trunk/src/wp-includes/blocks.php#L974), parse_blocks(), render_block(), etc. and all the available filters there.

From my perspective, the biggest challenge is that these callbacks are passed down through recursive calls of traverse_and_serialize_block when processing also inner blocks. I'd be happy to discuss ideas on how we could refactor the existing code to use filters instead.

Sure, sounds good! Lets have another look at all the block serializing, parsing and rendering functions and all the filters that run there and come up with something clearer and "unifying" :)

The simplest way to approach it I can think of would be setting filters inside traverse_and_serialize_blocks so anyone could extend the functionality of callbacks

Filters are generally a way to run callbacks. If using filters it won't be necessary/good to also pass callbacks as function arguments.

However, this way, you don't have access to additional information that we pass to block visitors, like the list of all hooked blocks...

Hmm, as far as I see all context can also be passed to the callbacks that run through filters. There's no difference.

I'm sure there are many alternative ways it could be refactored, so what I shared is more of a conversation starter. The most important question is whether this should be opened to extenders, knowing that we use the same processing every time we need to inject hooked blocks.

Yep, that's the first/most important question. As far as I understand for now this should not be used by extenders so making traverse_and_serialize_blocks() and friends private in WP 6.5 would be best. If this is eventually added to the (public) blocks API, it would probably have to be refactored a bit to make it more useful.

BTW the make_before_block_visitor() and make_after_block_visitor() don't seem to make sense in PHP. I know returning a lambda function from a function is a common pattern in JS, but scopes in PHP work differently (this is the reason why the use language construct is used) so these functions look... somewhat weird :)

All functions in PHP are always created in the global scope, no matter how and where they are written. Also if I remember right these may also cause a memory leak, perhaps only in older PHP versions (each time one of the make_*_visitor() function runs it will crate a new lambda function in the global scope which afaik is not automatically destroyed). Would definitely need refactoring whether they are private or not.

[azaozz]

Replying to mikeschroder:

As we're getting closer to freeze, I wanted to leave a quick note that I think this makes sense for 6.4 and am +1 with commit and backport.

Also +1.

[hellofromTonya]

Patch: ​https://github.com/WordPress/wordpress-develop/pull/5607

The patch fits the scope of this ticket, as it marks the remaining Blocks Hooks global functions as private.

Though not required, I also like the added note within the docblock that further explains the functions are for Core internal usage only. Thinking that same note could be applied to the other non _ functions added for Block Hooks, e.g. make_after_block_visitor() et al. I can add that same docblock explanation at commit prep.

Prepping commit now.

Commit 2db9f425015a80841b3bcc483b6499c45d4ede1a adds the docblock explanation to make_before_block_visitor() and make_after_block_visitor(), so that all 4 of these new functions have the same documentation.

[hellofromTonya]

In 57066:

Blocks: Document Block Hooks functions as private.

Documents the 4 new 6.4 Block Hooks global functions as private and for Core-only internal usage:

• make_before_block_visitor()
• make_after_block_visitor()
• traverse_and_serialize_block()
• traverse_and_serialize_blocks()

This is being done as the architectural design of these new functions may change in the next cycle. Further denoting them as private / Core only can help to avoid extender churn if any of these functions are deprecated.

Follow up to [56649], [56620].

Props azaozz, hellofromTonya, bernhard-reiter, gziolo, mikeschroder.
Fixes #59783.
See #59313.

[hellofromTonya]

Reopening for 2nd committer sign-off to backport [57066] to the 6.4 branch.

Committed to trunk via https://core.trac.wordpress.org/changeset/57066.

[karmatosed]

Giving this a 2nd committer sign-off. Thanks.

[hellofromTonya]

Thank you @karmatosed :) Backporting now.

[hellofromTonya]

In 57071:

Blocks: Document Block Hooks functions as private.

Documents the 4 new 6.4 Block Hooks global functions as private and for Core-only internal usage:

• make_before_block_visitor()
• make_after_block_visitor()
• traverse_and_serialize_block()
• traverse_and_serialize_blocks()

This is being done as the architectural design of these new functions may change in the next cycle. Further denoting them as private / Core only can help to avoid extender churn if any of these functions are deprecated.

Follow up to [56649], [56620].

Reviewed by karmatosed.
Merges [57066] to the 6.4 branch.

Props azaozz, hellofromTonya, bernhard-reiter, gziolo, mikeschroder.
Fixes #59783.
See #59313.