-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add generic has_blocks function #8631
Conversation
Merges `gutenberg_post_has_blocks()` and `gutenberg_content_has_blocks()`, resulting in a template function that can be used context-independently. It accepts a content string, a post object, or post ID, and default to the current post if none of those are given. Make this part of the API behave close to existing WordPress template functions. It's my first Gutenberg PR, so I'd love to get feedback on this, especially around a better phpunit setup potentially? See #4418.
@@ -57,7 +57,7 @@ function gutenberg_parse_blocks( $content ) { | |||
* If there are no blocks in the content, return a single block, rather | |||
* than wasting time trying to parse the string. | |||
*/ | |||
if ( ! gutenberg_content_has_blocks( $content ) ) { | |||
if ( ! has_blocks( $content ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been brought up the potential for conflicts with function names. We might need to prefix with wp_
here. But then the name can be seen as misleading "WordPress has blocks" (well, of course it does!).
Do we have similar functions in core that we should model after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In core, I can think of has_shortcode
has_excerpt
has_category
or has_custom_logo
.
Functions implemented with wp_*
seem to be related to actions like wp_{action}_{object}
like wp_get_nav_menu
or wp_get_attachment_{meta}
.
I would vote for wp_
prefix just to make a difference from PHP native functions like is_string
is_readable
file_exists
.
Anyway, I'm also thinking about this topic in #8340, and I would love to follow the decision here. In that case, I was thinking about a formula like wp_{who}_has_{what}
for example:
wp_post_has_block
where the $post parameter can be defaulted for$post
global as many WordPress functions dowp_content_has_block
Utility to quickly search for a block in a piece of content (maybe widgets content or excerpts.)
Aside: And I don't even want to think about the fact that there may be a semantic difference between block
and block_type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chiming in with my own thoughts:
- Consistency is important. If there's already
has_shortcode
,has_excerpt
, etc. the thought of introducingwp_has_blocks
is worrying merely in that it breaks convention and therefore expectations of the developer.- On point of consistency, I'm a bit disappointed to see that
has_shortcode
accepts a string whilehas_excerpt
accepts a post object. And then this function here accepts either / or 😬
- On point of consistency, I'm a bit disappointed to see that
- Conflicts could occur, but the plugin handbook already prescribes recommendations to avoid naming conflicts
- Recent additions have been introduced without prefix, e.g.
register_rest_route
- Whatever decision is made should probably be made and adopted universally, so that this discussion doesn't come up on every addition of a new core function 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should probably move this chat to the umbrella issue, as that was the point of it, to avoid repeating similar conversations in multiple PRs :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aforementioned issue: #8352
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given #8352 (comment) and linked discussion in Slack meeting, I'm inclined to think this should remain has_blocks
as proposed, for consistency with existing comparable WordPress function names.
Is this the same as #8340 ? |
For posterity since it was already answered in #8340 (comment), this one is about a generic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks @obenland!
Because I wasn't online for core dev chat, I'll note here that has_blocks()
is the correct name to use here. 🙂
🎉 |
The function `gutenberg_content_has_blocks` is deprecated as of version 3.6.0. The discussion leading up to the change can be found [here](WordPress/gutenberg#4418) and the PR which was merged 2 days ago can be found [here](WordPress/gutenberg#8631).
See #4418.
Description
Merges
gutenberg_post_has_blocks()
andgutenberg_content_has_blocks()
, resulting in a template function that can be used context-independently.It accepts a content string, a post object, or post ID, and default to the current post if none of those are given. Make this part of the API behave close to existing WordPress template functions.
It's my first Gutenberg PR, so I'd love to get feedback on this, especially around a better phpunit setup potentially?
How has this been tested?
I added unit tests and ran those, as well as navigating around post and page edit screens.
Types of changes
has_blocks()
.gutenberg_post_has_blocks()
andgutenberg_content_has_blocks()
.gutenberg_post_has_blocks()
andgutenberg_content_has_blocks()
.has_blocks()
.Checklist: