Solve #18704 on front-end

[carlomanf]

Description

Partially resolves #18704, only on front-end. Also resolves #26923, further info about this solution is available in my comments on that issue.

How has this been tested?

Tested it according to #25472, and saw the fatal error disappear and the front-end page load successfully. Without this patch, there was a fatal error.

I didn't attempt to fix the admin experience, but confirmed that it's NOT fixed by this.

Screenshots

Types of changes

Bug fix

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.

[gziolo]

Is this implementation of the WP_Block class loaded in the plugin? It only exists for backward compatibility:

gutenberg/lib/class-wp-block.php

Line 11 in 1d21b92

* This class can be removed when plugin support requires WordPress 5.5.0+.

Once we enforce WordPress 5.5.0 as a requirement for the plugin, it will be removed.

[carlomanf]

@gziolo to be honest, I actually tested this by applying these changes to the core class in wp-includes, but I get very confused about duplication of code between the core and the plugin and wasn't sure where was the correct place to submit the patch. Should I open a ticket on trac instead?

[gziolo]

Assuming that WP_Block class needs to be modified, it will require a patch in WP core so it wouldn't be that bad idea to open a ticket in Trac. Once we have a fix there we can try to use one of the existing hooks to temporarily (until released in WP Core) patch it in the plugin.

[carlomanf]

Note that I've just detected a side effect of the patch. Although it successfully escapes fatal errors, it also appears to wrongly escape some blocks that were not causing any fatal errors. I will have to investigate when I get time.

[bobbingwide]

Although it successfully escapes fatal errors, it also appears to wrongly escape some blocks that were not causing any fatal errors.

I too noticed that problem.

Before attempting to implement my own solution to the problem, which is individually applied to potentially recursive block types, I tried this solution. It incorrectly returned too early for blocks in a query-loop.

This was running my test case referenced here: bobbingwide/fizzie#27 (comment)

[bobbingwide]

Assuming that WP_Block class needs to be modified

Is there any mileage in Gutenberg extending the WP_Block related classes to extend the WordPress core functionality when it's required?

And/or instead of using new WP_Block potentially recursive blocks could use new gutenberg_WP_Block_recursive

[carlomanf]

@bobbingwide thank you for pointing out that unexpected result involving the query loop. I just added some commits that appear to fix this.

[bobbingwide]

@carlomanf I tried your fix. It got a bit further but still detected recursion too early.

Compare this output with my expected output.

BTW. I'm making progress on a very similar solution with the ability to report where the problem occurred.
I've now refactored my original prototype to use as a class. Now I'm looking into the possibility of extending the class in order to implement context sensitive errors - ie To extend the solution to override the logic for report_recursion_error().

See https://github.com/bobbingwide/fizzie/tree/main/includes

This solution won't require changes to the WP_Block class, but will still require shimming once it's become part of core.

[carlomanf]

@bobbingwide It looks like it's the post content and post title block that are getting caught this time. I was able to replicate it with the post content and added another commit to fix this, but I couldn't replicate it with the post title. Hopefully that too gets solved with this new commit. Thanks for testing this out.

[bobbingwide]

@carlomanf I retested latest your solution and still got too many false positives.

I changed your recursion detection logic to return a recursion message:

if ( in_array( ( array ) $this, self::$currently_rendering)  && $is_dynamic  ) {
                return $this->recursion_message();
            } else {
                self::$currently_rendering[] = (array) $this;
            }

Implemented as below:

/**
     * Returns a message about the recursion that's occurred.
     *
     * @return string
     */
	function recursion_message() {
        $recursion_message = '';
        // @TODO Test on WP_DEBUG, user is logged in, user has capability to fix etc.
        if (true) {
            // Only echo when safe to do so.
            if ( false ) {
                echo '<br />';
                echo "Recursion detected: ";
                echo $this->parsed_block['blockName'];
                //echo ':';
                //echo $this->block_type;
                //print_r($this->parsed_block);
                echo ' attrs: ';
                echo implode(',', $this->parsed_block['attrs']);
                echo $this->render_context( $this->context );
                //echo '<br />';
            }
            $recursion_message = "<br />Recursion detected:" . $this->parsed_block['blockName'];
            $recursion_message .= '<br />Attrs: ';
            $recursion_message .= $this->render_context( $this->parsed_block['attrs']);
            $recursion_message .= '<br />Context: ';
            $recursion_message .= $this->render_context( $this->context );
            $recursion_message .= '<br />';
            $recursion_message .= $this->render_stack();
        }
        return $recursion_message;
    }

    /**
     * Renders the currently_rendering stack.
     *
     * Aids problem determination by showing the stack of nested blocks
     * where the recursion was detected.
     * @return string
     */
	function render_stack() {
	    $content = '<ol>';
	    foreach ( self::$currently_rendering as $key => $block ) {
	        //print_r( $block );
	            $content .= '<li>';
                $content .= $block['name'];
                $content .= ' ';
                //print_r( $block['context']);
                $content .= $this->render_context( $block['context'] );
                //$content .= ' ';
                //$content .= implode(':', $block['* available_context'] );
                $content .= '</li>';

        }
	    $content .= '</ol>';
	    //echo $content;
	    return $content;
    }

    /**
     * Renders the top level of an array.
     *
     * @param $context
     * @return string
     */
    function render_context( $context ) {
        $content = '';
        foreach ($context as $key => $value) {
            $content .= "$key=";
            if ( is_array( $value )) {
                $content .= '[]';
            } else {
                $content .= $value;
                $content .= ' ';
            }
        }
        return $content;

    }

My test case shows the call stack where the post-title block was not processed.

In the test results for PR #27012 the post titles are displayed because they don't need recursion checking.
If a block could be given a property that indicates it could go recursive then this solution might be able to satisfy the requirements. If this property were the className that handled the recursion error and the recursion checking was only performed when this property was not null, then this solution would work.

For both our solutions, there's still the problem that once the logic is in core it can't easily be altered in a newer version of Gutenberg.

[carlomanf]

@bobbingwide I'm not able to reproduce the false positives with the post title. They're rendering fine for me.

Maybe someone else could test to confirm whether they get these false positives?

[carlomanf]

Closing this because it needs to be targeted at core instead.