Avoid declaring a function inside another function

[felixarntz]

What?

Just a quick cleanup following #45299 (I didn't get to reviewing the PR until it was already merged).

Why?

We shouldn't declare functions inside of other functions.

How?

This PR introduces the function in a reusable way, making the block name a parameter.

[tellthemachines]

Thanks for addressing this! Code looks good and it's working as expected.

Another option would be to inline the logic inside the if ( ! empty( $current_template ) ) condition. I recently got feedback on a backport to the effect that we should avoid introducing functions that are only used in one place, but I'm not sure to what extent that is a rule for core code?

[andrewserong]

This looks good to me, too, and I could imagine other use cases for finding the first instance of a particular block in an array of blocks, so I like the idea of generalising it. Just left a tiny nit comment about the name of a variable inside the function, but other than that, LGTM!

[andrewserong]

Tiny nit: since this function has been abstracted away from being solely used for the post content block, would it be better to name this variable $found_block rather than $post_content?

[felixarntz]

@andrewserong Good catch, updated!

[felixarntz]

Thanks @tellthemachines and @andrewserong!

I recently got feedback on a backport to the effect that we should avoid introducing functions that are only used in one place, but I'm not sure to what extent that is a rule for core code?

I think that depends on the functions. If it's an extremely specific function for one use-case, it shouldn't be introduced as a global function. But the more flexible and reusable the purpose in other scenarios, the more introducing a global function is justifiable.

I wouldn't introduce the original inline function as a global function since it was only for the core/post-content block. But in its new shape where it works for any block, I think it is useful. For example, a common use case for certain plugins or even themes in the past has been to detect the first image from the content. This function would make that a breeze for block content by looking for the core/image block. That's just one use-case this new function would facilitate.

[andrewserong]

Thanks for updating @felixarntz! This is testing well for me, and I agree, the way this is written I can see a number of good use cases for it. Also, it'd be easy to revert this further down the track if there's any pushback in introducing the function to core for 6.3, so I can't see any blockers to landing this in lib/experimental/ 👍

LGTM! ✨

[ramonjd]

@felixarntz Do you think we can leave this one until 6.4?

[tellthemachines]

@ramon this has already been backported as part of WordPress/wordpress-develop#4614.

[ramonjd]

Great thanks! I'll check it off 🙇