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

(latest-posts) Make latest-posts ssr categories handling more defensive #53659

Merged

Conversation

fullofcaffeine
Copy link
Member

@fullofcaffeine fullofcaffeine commented Aug 14, 2023

What?

This is an attempt to fix #53648.

Why?

In some multisite instances (i.g Wordpress.com) data corruption somewhere else (we couldn't troubleshoot the origin yet, unfortunately) can cause the categories attribute passed over to render_block_core_latest_posts to be blank (''), this ends up causing a PHP warning:

array_column() expects parameter 1 to be array, string given

And worst of all, ends up printing the warning in the HTML instead of the actual latest-posts block!

How?

By making the condition more defensive by making sure the assignment doesn't happen if $attributes['categories] is empty. For some reason, categories is added with an empty string, while under normal circumstances, it should not be part of the $attributes array at all if it's not set.

Testing Instructions

Unfortunately, it's not a scenario that's trivial to reproduce. The only way is to force it by passing an $attributes array arg with an empty categories (set to '') at the beginning of this method: https://github.com/WordPress/gutenberg/blame/trunk/packages/block-library/src/latest-posts/index.php#L36. I also didn't find unit tests for this block so this was the only way I could test it.

@github-actions
Copy link

Flaky tests detected in 197c98d.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5861602111
📝 Reported issues:

@fullofcaffeine
Copy link
Member Author

Does anyone know if there are any existing blocks that have unit tests for their SSR/PHP logic? I couldn't find any so far.

@fullofcaffeine fullofcaffeine added the [Type] Bug An existing feature does not function as intended label Aug 15, 2023
@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Aug 15, 2023

I further debugged it and it seems the bug happens at save-time, meaning that by the time we get the error from this stacktrace:

1. `/plugins/gutenberg-core/v16.3.0/build/block-library/blocks/latest-posts.php:60` - `array_column()`
2. `wp-includes/class-wp-block.php:263` - `gutenberg_render_block_core_latest_posts()`
3. `wp-includes/blocks.php:1133` - `render()`
4. `wp-includes/blocks.php:1171` - `render_block()`
5. `wp-includes/class-wp-hook.php:312` - `do_blocks()`
6. `wp-includes/plugin.php:205` - `apply_filters('<!-- wp:latest-posts {"categories":"","postsToShow":3,"displayPostContent":true,"displayPostDate":true,"className":"is-style-default"} /-->')`
7. `wp-includes/widgets/class-wp-widget-block.php:78` - `apply_filters('widget_block_content')`
8. `wp-includes/class-wp-widget.php:407` - `widget()`
9. `wp-includes/widgets.php:837` - `display_callback()`
10. `/themes/pub/twentysixteen/sidebar.php:13` - `dynamic_sidebar()`
11. `wp-includes/template.php:785` - `require_once('/themes/pub/twentysixteen/sidebar.php')`
12. `wp-includes/template.php:720` - `load_template()`
13. `wp-includes/general-template.php:136` - `locate_template()`
14. `/themes/pub/twentysixteen/page.php:41` - `get_sidebar()`
15. `wp-includes/template-loader.php:106` - `include('/themes/pub/twentysixteen/page.php')`
16. `wp-blog-header.php:19` - `require_once('wp-includes/template-loader.php')`
17. `index.php:49` - `require('wp-blog-header.php')`

...the block markup already has the faulty markup for the block (with a categories key with an empty string, like this <!-- wp:latest-posts {"categories":"","postsToShow":3,"displayPostContent":true,"displayPostDate":true,"className":"is-style-default"} /-->'). By tracing it back in the stacktrace, I've arrived at this function, and logging the settings in line 614 still bring up the faulty markup for the latest posts, so I realized that:

  1. The bug that caused the faulty makeup must have happened at save-time (when the block widget option was saved);
  2. Widgets are saved as wp options (TIL).

Given the additional testing and troubleshooting, I arrived at the conclusion that this patch is the best solution for it now, and it seems to work fine, too (I tested in one of the sites in our multisite instance, and the latest posts started to render correctly, you can try to reproduce it by setting categories to '' manually as mentioned in the description); unless someone else has any additional insights about it!

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Change makes sense to me. Would have been nice if we could add a unit test.

@skorasaurus skorasaurus added the [Block] Latest Posts Affects the Latest Posts Block label Aug 15, 2023
@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Aug 15, 2023

Would have been nice if we could add a unit test.

@tyxla Yep! I'm just not sure where/how to add it though. I didn't find any examples of unit-tests that test the backend of a block -- this would definitely not be a hard test to write, but not sure if GB has the infrastructure for PHP unit-tests inside the block directories? I only see JS tests there, e.g: https://github.com/WordPress/gutenberg/tree/trunk/packages/block-library/src/latest-posts/test. Happy to add it in a follow-up if someone could guide me here!

@fullofcaffeine fullofcaffeine merged commit eb35ab3 into trunk Aug 15, 2023
52 of 53 checks passed
@fullofcaffeine fullofcaffeine deleted the make/latest-posts-category-handling-more-defensive branch August 15, 2023 17:07
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 15, 2023
@tyxla
Copy link
Member

tyxla commented Aug 15, 2023

Would have been nice if we could add a unit test.

@tyxla Yep! I'm just not sure where/how to add it though. I didn't find any examples of unit-tests that test the backend of a block -- this would definitely not be a hard test to write, but not sure if GB has the infrastructure for PHP unit-tests inside the block directories? I only see JS tests there, e.g: https://github.com/WordPress/gutenberg/tree/trunk/packages/block-library/src/latest-posts/test. Happy to add it in a follow-up if someone could guide me here!

Yup, I have the same observations. The only ones I found were CodeSniffer-based coding standards ones. It's definitely not worth adding the infrastructure just for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Posts Affects the Latest Posts Block [Type] Bug An existing feature does not function as intended
Projects
None yet
3 participants