[Block Library]: Less warnings when blocks try to render themselves. #33032

ntsekouras · 2021-06-28T12:00:12Z

Description

In #31455, #28405 and #28456 an extra check was implemented for Post Content, Template Part and Reusable blocks not to render themselves and cause an infinite loop. If they do render themselves an error was triggered and was logged.

While this is wrong usage of the blocks by users and should be handled by them - with exception with the nuances of Query Loop in some cases, the check that we had now didn't take into account if the code was run from the REST API making them log more times that it should.

Testing instructions

in a post add a QueryLoop block that contains itself in the results and add the Post Content block.
check the site logs for the produced warnings
test with block based themes (I tested with tt1-blocks) and classic themes

Notes

While these changes make tt1-blocks to show no warnings at all, the warnings seem to differ per theme. Maybe some filter triggers the code?? 🤔 So in other classic blocks that I tested, some warnings are shown, but are way less.
In the below gifs I have made an extreme test post which contains all three cases/blocks containing themselves. That's why the volume of warnings are quite big. You can test with each case individually.

Screenshots

tt1-blocks before

2021.blocks.before.mov

tt1-blocks after

2021.blocks.after.mov

2021 before

2021.before.mov

2021 after

2021.after.mov

packages/block-library/src/post-content/index.php

swissspidy · 2021-06-28T12:14:35Z

lib/utils.php

+	if ( empty( $_SERVER['REQUEST_URI'] ) ) {
+		return false;
+	}
+
+	$rest_prefix         = trailingslashit( rest_get_url_prefix() );
+	$is_rest_api_request = ( false !== strpos( $_SERVER['REQUEST_URI'], $rest_prefix ) );
+	return $is_rest_api_request;


Could this just be return \defined( 'REST_REQUEST' ) && \REST_REQUEST === true? Or is this code running too early for that to suffice?

Exactly. In some cases it's not defined yet. That's how I started 😄

In what circumstances is it too early?

I'm not sure I can answer this with confidence.

While these changes make tt1-blocks to show no warnings at all, the warnings seem to differ per theme. Maybe some filter triggers the code?? 🤔

I hope someone has some thoughts on this and it might be related as I've testing with multiple themes and the logs weren't consistent and didn't pinpoint the reason for that. So it might not be needed if I find out the reason for these different number of triggered logs 🤔

Exactly. In some cases it's not defined yet.

This definitely feels like a code smell. When exactly did you experience this, or with which themes?

lib/utils.php

peterwilsoncc

The logic on when to throw an error seems a little backward to me, the warnings/notices are thrown on the front end but not in the back end.

On high traffic sites (such as a 538 or Snopes) the majority of warnings will still be logged which is a problem if the post was error free initially but has started erroring when an unrelated post is published.

ntsekouras · 2021-06-29T08:12:25Z

The logic on when to throw an error seems a little backward to me, the warnings/notices are thrown on the front end but not in the back end.

In the backend the warning is embed in the block/content, so it seems to be enough and pointing to the block with the problem.

On high traffic sites (such as a 538 or Snopes) the majority of warnings will still be logged which is a problem if the post was error free initially but has started erroring when an unrelated post is published.

This indeed is a possible problem now in this use case (Query Loop with same postType, includes PostContent and includes current post). I'm wondering if an alternative could be to show Post Excerpt and a warning when the Post Content block tries to render itself. 🤔

mcsf · 2021-06-29T16:12:59Z

This indeed is a possible problem now in this use case (Query Loop with same postType, includes PostContent and includes current post). I'm wondering if an alternative could be to show Post Excerpt and a warning when the Post Content block tries to render itself. 🤔

Nik and I discussed this privately, and I advised against replacing offending posts with their excerpts for two reasons. 1. From a technical standpoint, we either produce a poor excerpt by doing as little as possible (e.g. grab the first n characters of the content's inner HTML) or we need to do a lot to generate a better excerpt while avoiding the recursion problem (i.e. do almost everything from parsing blocks, applying content filters, and rendering dynamic blocks). 2. From a product perspective, this replacement would be a very arbitrary decision that we would be making for the user, leaving open the question of how to let users customise this behaviour while avoiding the recursion problem to creep back in.

On high traffic sites (such as a 538 or Snopes) […]

This is a good mindset to bring to the table.

I wonder if we could keep this simple by just accepting that prevented recursion situations don't always need to be logged. As Nik reminds us, these errors are always presented in the block editor, which is a good start. On the front end, the warning is only rendered when the debugging constants are enabled (see below). What if we extended that to the trigger_error call?

defined( 'WP_DEBUG' ) && WP_DEBUG && 
	defined( 'WP_DEBUG_DISPLAY' ) && WP_DEBUG_DISPLAY

TimothyBJacobs · 2021-06-29T18:33:49Z

wonder if we could keep this simple by just accepting that prevented recursion situations don't always need to be logged. As Nik reminds us, these errors are always presented in the block editor, which is a good start. On the front end, the warning is only rendered when the debugging constants are enabled (see below). What if we extended that to the trigger_error call?

Big +1 to this.

peterwilsoncc · 2021-06-29T23:42:28Z

It seems like all the options on approach have an imperfect solution. :)

If that's the case then I'm wondering if simplicity is the best approach by including something like this in packages/block-library/src/block/index.php::render_block_core_block():

if ( isset( $seen_refs[ $attributes['ref'] ] ) ) {
	// Skip this post to avoid recursion. 
	return '';
}

The imperfection of this solution is an off-by-one error if the current post is included in the query added by the user. Given the error can be triggered by the publication/modification of unrelated posts, meaning the warning won't be seen in the admin, I think it's the best of a bad lot.

ntsekouras · 2021-06-30T07:54:07Z

I wonder if we could keep this simple by just accepting that prevented recursion situations don't always need to be logged. As Nik reminds us, these errors are always presented in the block editor, which is a good start. On the front end, the warning is only rendered when the debugging constants are enabled (see below). What if we extended that to the trigger_error call?

I updated the code upon the above comment from Miguel.

I'm not opposed though to maybe losing the log entirely as Peter suggests..

peterwilsoncc · 2021-06-30T23:27:12Z

Thanks for your work on this Nik.

The more I think about it, the better I think it is to remove the log entirely and only display warnings to the user.

The error is triggered by user action so there isn't anything a sysadmin/developer can action in code to fix it. Generally PHP logs should warn of developer error: use of deprecated code, incorrect types, etc.

I also would say this, it was my suggestion in the first place.

mcsf · 2021-07-01T09:27:54Z

Peter makes a good case. Alright, I'm on board, let's just remove the reporting.

…33032) * Less warnings when blocks try to render themeselves. * rename util to gutenberg_is_rest_api_request * Log only if is `is_debug` and not in admin or REST request * remove log entirely

ntsekouras added [Type] Bug An existing feature does not function as intended [Block] Block The "Reusable Block" Block [Block] Post Content Affects the Post Content Block [Block] Template Part Affects the Template Parts Block labels Jun 28, 2021

ntsekouras requested review from ockham, mcsf, youknowriad, aristath, gziolo and a team June 28, 2021 12:00

ntsekouras self-assigned this Jun 28, 2021

ntsekouras requested review from ajitbohra, spacedmonkey and TimothyBJacobs as code owners June 28, 2021 12:00

ntsekouras commented Jun 28, 2021

View reviewed changes

packages/block-library/src/post-content/index.php Show resolved Hide resolved

swissspidy reviewed Jun 28, 2021

View reviewed changes

lib/utils.php Outdated Show resolved Hide resolved

peterwilsoncc reviewed Jun 29, 2021

View reviewed changes

ntsekouras force-pushed the fix/less-warnings-when-blocks-try-to-render-themselves branch from e202798 to aac5aaf Compare June 30, 2021 07:46

ntsekouras added 3 commits July 1, 2021 08:30

Less warnings when blocks try to render themeselves.

8240280

rename util to gutenberg_is_rest_api_request

69e68ce

Log only if is is_debug and not in admin or REST request

7d7ee8f

ntsekouras force-pushed the fix/less-warnings-when-blocks-try-to-render-themselves branch from aac5aaf to 7d7ee8f Compare July 1, 2021 05:31

remove log entirely

726f538

ntsekouras force-pushed the fix/less-warnings-when-blocks-try-to-render-themselves branch from a221123 to 726f538 Compare July 1, 2021 05:38

ntsekouras added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 1, 2021

mcsf approved these changes Jul 1, 2021

View reviewed changes

ntsekouras merged commit a964525 into trunk Jul 1, 2021

ntsekouras deleted the fix/less-warnings-when-blocks-try-to-render-themselves branch July 1, 2021 09:41

github-actions bot added this to the Gutenberg 11.1 milestone Jul 1, 2021

youknowriad mentioned this pull request Jul 5, 2021

Backport fixes for WordPress 5.8 RC2 #33199

Merged

youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 5, 2021

youknowriad modified the milestones: Gutenberg 11.1, Gutenberg 11.0 Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Block Library]: Less warnings when blocks try to render themselves. #33032

[Block Library]: Less warnings when blocks try to render themselves. #33032

ntsekouras commented Jun 28, 2021 •

edited

Loading

swissspidy Jun 28, 2021

ntsekouras Jun 28, 2021

TimothyBJacobs Jun 28, 2021

ntsekouras Jun 28, 2021

mcsf Jun 29, 2021

peterwilsoncc left a comment

ntsekouras commented Jun 29, 2021

mcsf commented Jun 29, 2021

TimothyBJacobs commented Jun 29, 2021

peterwilsoncc commented Jun 29, 2021

ntsekouras commented Jun 30, 2021

peterwilsoncc commented Jun 30, 2021

mcsf commented Jul 1, 2021

[Block Library]: Less warnings when blocks try to render themselves. #33032

[Block Library]: Less warnings when blocks try to render themselves. #33032

Conversation

ntsekouras commented Jun 28, 2021 • edited Loading

Description

Testing instructions

Notes

Screenshots

tt1-blocks before

tt1-blocks after

2021 before

2021 after

swissspidy Jun 28, 2021

Choose a reason for hiding this comment

ntsekouras Jun 28, 2021

Choose a reason for hiding this comment

TimothyBJacobs Jun 28, 2021

Choose a reason for hiding this comment

ntsekouras Jun 28, 2021

Choose a reason for hiding this comment

mcsf Jun 29, 2021

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

ntsekouras commented Jun 29, 2021

mcsf commented Jun 29, 2021

TimothyBJacobs commented Jun 29, 2021

peterwilsoncc commented Jun 29, 2021

ntsekouras commented Jun 30, 2021

peterwilsoncc commented Jun 30, 2021

mcsf commented Jul 1, 2021

ntsekouras commented Jun 28, 2021 •

edited

Loading