Avoid declaring a function inside another function #49049
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| return $block; | ||
| } | ||
| if ( ! empty( $block['innerBlocks'] ) ) { | ||
| $post_content = gutenberg_find_first_block( $block_name, $block['innerBlocks'] ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@andrewserong Good catch, updated!
|
Thanks @tellthemachines and @andrewserong!
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 |
There was a problem hiding this comment.
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! ✨
|
@felixarntz Do you think we can leave this one until 6.4? |
|
@ramon this has already been backported as part of WordPress/wordpress-develop#4614. |
|
Great thanks! I'll check it off 🙇 |
ramonjd
added
[Type] Code Quality
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.