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

Add generic has_blocks function #8631

Merged
merged 5 commits into from
Aug 16, 2018
Merged

Add generic has_blocks function #8631

merged 5 commits into from
Aug 16, 2018

Conversation

obenland
Copy link
Member

@obenland obenland commented Aug 6, 2018

See #4418.

Description

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?

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

  • Introduces has_blocks().
  • Deprecates gutenberg_post_has_blocks() and gutenberg_content_has_blocks().
  • Replaces existing uses of gutenberg_post_has_blocks() and gutenberg_content_has_blocks().
  • Adds PHP unit tests for has_blocks().

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

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.
@obenland obenland requested a review from pento August 6, 2018 22:40
@obenland obenland added [Type] Enhancement A suggestion for improvement. [Feature] Templates API Related to API powering block template functionality in the Site Editor labels Aug 6, 2018
@mtias mtias mentioned this pull request Aug 6, 2018
16 tasks
@danielbachhuber danielbachhuber mentioned this pull request Aug 7, 2018
4 tasks
@@ -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 ) ) {
Copy link
Member

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?

Copy link
Contributor

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 do
  • wp_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.

Copy link
Member

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 introducing wp_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 while has_excerpt accepts a post object. And then this function here accepts either / or 😬
  • 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 😆

Copy link
Member

@mtias mtias Aug 8, 2018

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

Copy link
Member

Choose a reason for hiding this comment

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

Aforementioned issue: #8352

Copy link
Member

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.

@aduth
Copy link
Member

aduth commented Aug 15, 2018

Is this the same as #8340 ?

@obenland
Copy link
Member Author

Is this the same as #8340 ?

For posterity since it was already answered in #8340 (comment), this one is about a generic has_blocks() while #8340 checks for a specific block.

@pento pento added this to the 3.6 milestone Aug 16, 2018
Copy link
Member

@pento pento left a 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. 🙂

@pento pento merged commit 8d9b4b3 into master Aug 16, 2018
@pento pento deleted the origin/add/has-blocks branch August 16, 2018 04:15
@mtias
Copy link
Member

mtias commented Aug 16, 2018

🎉

peiche added a commit to peiche/aesop-core that referenced this pull request Aug 17, 2018
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants