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

Blocks: Implement recursion checking and reporting for template-parts, post-content and reusable blocks (#26923) #27012

Closed
wants to merge 8 commits into from

Conversation

bobbingwide
Copy link
Contributor

@bobbingwide bobbingwide commented Nov 16, 2020

Description

Both solutions rely on a test to see if the block that's about to be rendered is already being processed.

The logic implemented in each solution is:

  • Check if the block that's about to be rendered is already being rendered.

  • If it isn't already being rendered. ie It's safe to render

    • add it to the stack of blocks being rendered.
    • render the block
    • pop it from the stack
  • else don't render it

  • In the WP_Block fix the recursive block is rendered as an empty string. Neither the end user nor the site administrator is made aware that there was a recursion problem.

  • In this solution, there is an option to report the recursion error.

  • The level of detail reported depends on the setting for WP_DEBUG

  • This solution also supports extending the base solution for reporting recursion errors.

Regarding the reusable blocks issue ( #18704 )

  • I've developed an extension class for the reusable block recursion problem that attempts to deal with the problem when encountered in a REST request.
  • This error reporting extension for reusable blocks is not (yet) part of this PR.
  • While it is able to deal with a self recursive reusable block
  • It is unable to prevent the block editor from going recursive when a user inserts a reusable block that's been returned in the request to fetch reusable blocks.

How has this been tested?

Environment

  • Tested with WordPress 5.6-beta3
  • Active theme: Twenty Twenty-One Blocks, with some additional template parts, some template parts commented out and modified CSS.
  • Active plugins: Gutenberg
  • Plus, used to aid understanding of the post-content output:
  • oik - for the [bw_list] shortcode
  • And during development - oik-bwtrace - used to trace server events

Screenshots

Scenarios

Front end

  • Tested recursion detection and reporting for template-parts using the 404.html template and a couple of recursive template parts.
  • Tested recursion detection and reporting for post-content within the query / query loop in multiple pages
  • Tested recursion detection for a self recursive reusable block.
    image

image

image

Types of changes

These changes also include the fixes applied for:

The solution introduces

  • 4 new files included in load.php
  • which implement 2 classes
    • WP_Block_Recursion_Control
    • WP_Block_Recursion_Error
  • and 3 APIs:
    • gutenberg_process_this_content - performs the check for recursion
    • gutenberg_clear_processed_content - pops the last item processed
    • gutenberg_report_recursion_error

Checklist:

  • My code is tested.

  • My code follows the WordPress code style.

  • My code follows the accessibility standards.

  • My code has proper inline documentation.

  • I've included developer documentation if appropriate.

  • I've updated all React Native files affected by any refactorings/renamings in this PR.

  • I've not delivered any automated tests.

  • I imagine the new functions will need documenting so that they can be used by other block developers who may have had to develop their own recursion checking.

@carlomanf
Copy link

So, compared to my PR, the only advantage of this one is it reports the error depending on WP_DEBUG? I just want to make sure I'm not missing anything else.

I'm happy to add that feature to my PR, considering mine solves the same bug with 4% of the amount of code that yours does.

  • In Solve #18704 on front-end #26951 @carlomanf has developed a solution that checks for recursion for any block within WP_Block class.
  • In my solution the recursion checking is only performed by block types which can go infinitely recursive.

Regarding this point, I should also point out that any third-party plugin can add a block type that has the potential for infinite recursion, and my PR proactively prevents those as well.

@youknowriad
Copy link
Contributor

@carlomanf depending on how you're checking for recursions, I'm not sure catching all blocks is a good thing. For example, we can use group blocks inside group blocks that's fine and potentially we could build dynamic group blocks that render inside dynamic group blocks, that's fine too.

@bobbingwide bobbingwide mentioned this pull request Nov 17, 2020
6 tasks
@bobbingwide
Copy link
Contributor Author

I'm not sure catching all blocks is a good thing

@youknowriad I've added a comment to @carlomanf's PR that covers this problem - for the core/post-title block.

#26951 (comment)

I just want to make sure I'm not missing anything else.

Whichever solution is chosen, we will need to ensure that it's usable and extendable by third party plugins.

@carlomanf
Copy link

@youknowriad

@carlomanf depending on how you're checking for recursions, I'm not sure catching all blocks is a good thing. For example, we can use group blocks inside group blocks that's fine and potentially we could build dynamic group blocks that render inside dynamic group blocks, that's fine too.

The only things my patch is designed to "catch" are fatal errors. Everything else, it should let pass.

That said, @bobbingwide has been very helpful in discovering some false positives, which I have since corrected. He believes he's found another one with the post title block, but I'm not able to reproduce it.

It would be much appreciated if you could test the patch at #26951 and confirm whether you can reproduce Herb's false positive with the post title, or this other hypothetical scenario with group blocks. As far as I can tell, the patch works correctly and doesn't escape anything that wouldn't have otherwise caused a fatal error.

@mcsf
Copy link
Contributor

mcsf commented Jan 25, 2021

@bobbingwide: thanks for the PR. However, in light of #28405 (merged) and #28456 (to be approved), is there anything left before it can be closed?

@mcsf mcsf added the [Status] Needs More Info Follow-up required in order to be actionable. label Jan 27, 2021
@mcsf mcsf closed this Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs More Info Follow-up required in order to be actionable.
Projects
None yet
4 participants