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

Blocks: Pass parent block, block index, chunk index to serialize_block(s) callback argument #5242

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 18, 2023

See https://core.trac.wordpress.org/ticket/59313.

TODO:

  • Needs a ton more test coverage
  • Could maybe even pass the innerBlock (and innerChunk?) indices to the callback -- a bit like JS/lodash does

Trac ticket: TBD


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ockham ockham self-assigned this Sep 18, 2023
@ockham
Copy link
Contributor Author

ockham commented Sep 18, 2023

@gziolo I think this (plus #5240) is how we can implement sibling insertion 🤞

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@ockham
Copy link
Contributor Author

ockham commented Sep 19, 2023

@gziolo Since we're adding quite a bit of complexity to serialize_block(s), I'm starting to consider renaming it to e.g. traverse_and_serialize_block(s), and keep serialize_block(s) as it was before #5187. (This will allow us to also optimize traverse_and_serialize_block(s), as it would enable it to call serialize_block(s) for its children if no callback is given -- thus making the is_callable check only at the top level, but avoiding it for all descendants, as basically discussed here.)

WDYT?

@ockham ockham changed the title Blocks: Pass parent block to serialize_block(s) callback argument Blocks: Pass parent block, block index, chunk index to serialize_block(s) callback argument Sep 19, 2023
@gziolo
Copy link
Member

gziolo commented Sep 19, 2023

traverse_and_serialize_block - that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.

@ockham
Copy link
Contributor Author

ockham commented Sep 19, 2023

traverse_and_serialize_block - that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.

I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s), which is what we'd try to avoid.)

@gziolo
Copy link
Member

gziolo commented Sep 19, 2023

I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s), which is what we'd try to avoid.)

I know realized that we are going to add more changes later in the process. That's fine 👍🏻

@gziolo
Copy link
Member

gziolo commented Sep 19, 2023

Opened #5246 with the changes discussed in the comments.

@ockham
Copy link
Contributor Author

ockham commented Sep 19, 2023

Closing in favor of #5246.

@ockham ockham closed this Sep 19, 2023
gziolo added a commit to gziolo/wordpress-develop that referenced this pull request Sep 19, 2023
…_block(s) callback argument

See WordPress#5242.
Co-authored-by: ockham <ockham@git.wordpress.org>
@ockham ockham deleted the update/serialize-block-pass-parent-to-callback branch September 19, 2023 11:31
$block_content .= $chunk;
} else {
$inner_block = $block['innerBlocks'][ $block_index ];
if ( is_callable( $callback ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I was a fan of your suggestion to split the loop, but without that, we could at least examine the impact of trapping $is_callable = is_callable( $callback ) outside of the loop. it appears like is_callable() is a function call and not a PHP operand, so it could be notable in its cost, particularly on long posts.

in any case, here it seems a little odd to embed that check since $callback shouldn't change between iterations and there's no need to recompute it every time.

@@ -887,11 +893,16 @@ function serialize_block( $block, $callback = null ) {
*
* @param array[] $blocks An array of representative arrays of parsed block objects. See serialize_block().
* @param callable|null $callback Optional. Callback to run on each block in the tree before serialization. Default null.
* It is called with the following arguments: $block, $parent_block, $block_index, $chunk_index.
Copy link
Member

Choose a reason for hiding this comment

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

this comment appears to directly contradict the code below it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants