Fix #54352 prevent php 8.1 fatal when template parts are not found in non-debug environments #54354

tomjn · 2023-09-11T13:16:55Z

What?

If the template part is not found content will be null which will cause fatal errors in PHP 8.1 when passed to shortcode_unautop, but this case is already handled if debug mode is turned on. This changeset allows it to be handled in all environments, not just when the admin has turned on WP_DEBUG and display debug messages.

Closes #54352

Why?

Because it's fixing a fatal error on PHP 8.1+

How?

This change separates the debug part into a sub-condition so that it can return '' if we are not in debugging mode.

Testing Instructions

Create a template with a template part
Go into the code editor and change the slug to something that does not exist on that site
ensure you are using PHP 8.1+
ensure that WP_DEBUG is false, it is essential that the $debug in the code has the value of false not true
visit a page with that template on the frontend
you should see that the page loads with this change, but crashes without it with a deprecation fatal error

tomjn · 2023-09-11T13:18:01Z

Noting that a PR was originally raised by @kadamwhite here WordPress/wordpress-develop#5177 which contains a small unit test for WP core

getdave · 2023-09-27T12:34:35Z

@tomjn Thanks for the PR.

Can I confirm whether this one is the canonical PR to fix #54352?

I believe it is which is why I've now linked it up with the Issue and then moved the Issue to "Needs Review" on the WP 6.4 Board but I felt it was wise to check given your comment above.

Also, apologies, but I was unable to replicate the original error. Perhaps it's due to my environment (I'm using wp-env) but despite following your instructions I couldn't get the original situation described in the Issue to occur. Could you think what I might be missing here?

tomjn · 2023-09-27T12:55:03Z

It's likely the template part you're referencing exists, a reading of the code clearly indicates a failure point where a non string value could be passed through. Note that no testing has been done on 6.4, only 6.3.

Note that the associated WP Trac ticket has a failing unit test that is fixed by this code change that was written by KAdam

getdave · 2023-09-27T13:26:27Z

Thanks for the update 👍

It's likely the template part you're referencing exists

Not unless I have a template part called thistempaltepartdoesnotexistsomakesurebytypingalongname.

I agree with your reading of the code, but I would like to be able to replicate. I'll continue in my efforts...

tomjn · 2023-09-27T13:57:43Z

In my original tests, the block template part was missing the theme attribute when I had the issue, which was the main reason it couldn't find the part

carolinan · 2024-01-15T12:06:36Z

I am not able to reproduce the fatal error, but I see a PHP warning for a different line, unaffected by the PR. So the warning shows when my template part slug is a non-existing template part, no matter if the PR is applied or not.

Warning: Attempt to read property "content" on null in /gutenberg/build/block-library/blocks/template-part.php on line 69

tomjn · 2024-02-08T09:27:14Z

I would note that $debug must resolve to false in order for testing of this PR to be possible. If it is true then the original bug will be handled by the original code which was the root of the problem. This is to prevent non-debug production sites from crashing in this situation.

github-actions · 2024-02-08T09:27:25Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tomjn <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: anton-vlasenko <[email protected]>
Co-authored-by: hellofromtonya <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: annezazu <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

getdave · 2024-02-08T11:53:44Z

I'm going to re-test this.

getdave

Thank you for the additional testing instructions.

I felt this was worth revisiting as a bug so I re-tested this by:

using PHP 8.1+
setting WP_DEBUG to false.
creating a template part with a random slug to ensure it would not exist

I then loaded the page with that template on the front of the site.

Testing on `trunk`

Without the change in this PR, I didn't see an error, but I did see an empty template part output to the front of the site. This is because the code managed to reach the return which outputs the HTML without erroring:

gutenberg/packages/block-library/src/template-part/index.php

Line 171 in 884c494

    
           return "<$html_tag $wrapper_attributes>" . str_replace( ']]>', ']]&gt;', $content ) . "</$html_tag>";

I consider the empty template part to be a bug. Not sure if something changed and why I don't get the error that you saw.

Testing on this PR

With the change in this PR the code correctly exits execution of the remainder of the function regardless of the value of the WP_DEBUG.

On the front of the site no template part markup was output at all which is what I would consider correct.

I believe the fix is good, but I would appreciate a confidence check (maybe @anton-vlasenko ?).

I think we should also land the unit test coverage for this that you referenced. This can land directly in WP Core once this patch is in Core.

anton-vlasenko

Just a minor suggestion.

packages/block-library/src/template-part/index.php

getdave · 2024-02-09T13:54:41Z

@tomjn Most of the test failures here look to be due to issues that were previously in evidence in Gutenberg trunk but have since been resolved.

If you:

update your fork
rebase this against the updated trunk

...then the tests should pass.

If would be ideal if this landed today so it was included in the final Gutenberg release for WP 6.5 which is happening at 16:00 UTC today.

If it doesn't make it it will need a manual backport as a bug.

… found in non-debug environments If the template part is not found content will be null which will cause fatal errors in PHP 8.1 when passed to shortcode_unautop, but this case is already handled if debug mode is turned on. This changeset allows it to be handled in all environments, not just when the admin has turned on WP_DEBUG and display debug messages

Co-authored-by: Anton Vlasenko <[email protected]>

@hellofromtonya

Incorporate feedback from @hellofromtonya https://github.com/WordPress/gutenberg/pull/54354/files#r1483032128

tomjn · 2024-02-09T14:48:22Z

@getdave I rebased, just waiting for CI, also happy for any intervention that needs to happen so this isn't blocked by me

getdave · 2024-02-09T14:57:35Z

@getdave I rebased, just waiting for CI, also happy for any intervention that needs to happen so this isn't blocked by me

Thank you. Unfortunately I cannot push to your fork otherwise I'd happily wrangle this on your behalf.

getdave · 2024-02-09T15:45:13Z

@MaggieCabrera Can we bring this in to 17.7.0 RC? If it didn't make the cut please do add the Needs PHP Backport label.

MaggieCabrera · 2024-02-09T16:32:26Z

@MaggieCabrera Can we bring this in to 17.7.0 RC? If it didn't make the cut please do add the Needs PHP Backport label.

This made it to the RC

tomjn requested a review from ajitbohra as a code owner September 11, 2023 13:16

carolinan added the [Type] Bug An existing feature does not function as intended label Sep 12, 2023

This was referenced Sep 27, 2023

59318: Add unit test demonstrating render method can return null WordPress/wordpress-develop#5177

Open

Fatal Error in PHP 8.1 when a template part reference is missing #54352

Closed

getdave added the [Block] Template Part Affects the Template Parts Block label Sep 27, 2023

mikachan 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 Oct 2, 2023

SiobhyB 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 Oct 19, 2023

getdave reviewed Feb 8, 2024

View reviewed changes

anton-vlasenko reviewed Feb 8, 2024

View reviewed changes

packages/block-library/src/template-part/index.php Outdated Show resolved Hide resolved

hellofromtonya reviewed Feb 8, 2024

View reviewed changes

packages/block-library/src/template-part/index.php Outdated Show resolved Hide resolved

tomjn and others added 3 commits February 9, 2024 14:47

Update packages/block-library/src/template-part/index.php

5bdd0bf

Co-authored-by: Anton Vlasenko <[email protected]>

Simplifies the check for a non-existent template part on render

b9686d6

Incorporate feedback from @hellofromtonya https://github.com/WordPress/gutenberg/pull/54354/files#r1483032128

tomjn force-pushed the patch-8 branch from fa02aa8 to b9686d6 Compare February 9, 2024 14:47

getdave approved these changes Feb 9, 2024

View reviewed changes

getdave enabled auto-merge (squash) February 9, 2024 14:59

getdave merged commit 1174479 into WordPress:trunk Feb 9, 2024
55 checks passed

github-actions bot added this to the Gutenberg 17.7 milestone Feb 9, 2024

hellofromtonya mentioned this pull request Jun 13, 2024

[Template Parts] PHP Warning thrown when get_block_file_template() returns null. #62559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #54352 prevent php 8.1 fatal when template parts are not found in non-debug environments #54354

Fix #54352 prevent php 8.1 fatal when template parts are not found in non-debug environments #54354

tomjn commented Sep 11, 2023 •

edited

Loading

tomjn commented Sep 11, 2023

getdave commented Sep 27, 2023 •

edited

Loading

tomjn commented Sep 27, 2023 •

edited

Loading

getdave commented Sep 27, 2023

tomjn commented Sep 27, 2023 •

edited

Loading

carolinan commented Jan 15, 2024

tomjn commented Feb 8, 2024 •

edited by getdave

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

getdave commented Feb 8, 2024

getdave left a comment

anton-vlasenko left a comment

getdave commented Feb 9, 2024

tomjn commented Feb 9, 2024 •

edited

Loading

getdave commented Feb 9, 2024

getdave commented Feb 9, 2024

MaggieCabrera commented Feb 9, 2024

Fix #54352 prevent php 8.1 fatal when template parts are not found in non-debug environments #54354

Fix #54352 prevent php 8.1 fatal when template parts are not found in non-debug environments #54354

Conversation

tomjn commented Sep 11, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

tomjn commented Sep 11, 2023

getdave commented Sep 27, 2023 • edited Loading

tomjn commented Sep 27, 2023 • edited Loading

getdave commented Sep 27, 2023

tomjn commented Sep 27, 2023 • edited Loading

carolinan commented Jan 15, 2024

tomjn commented Feb 8, 2024 • edited by getdave Loading

github-actions bot commented Feb 8, 2024 • edited Loading

getdave commented Feb 8, 2024

getdave left a comment

Choose a reason for hiding this comment

Testing on trunk

Testing on this PR

anton-vlasenko left a comment

Choose a reason for hiding this comment

getdave commented Feb 9, 2024

tomjn commented Feb 9, 2024 • edited Loading

getdave commented Feb 9, 2024

getdave commented Feb 9, 2024

MaggieCabrera commented Feb 9, 2024

tomjn commented Sep 11, 2023 •

edited

Loading

getdave commented Sep 27, 2023 •

edited

Loading

tomjn commented Sep 27, 2023 •

edited

Loading

tomjn commented Sep 27, 2023 •

edited

Loading

tomjn commented Feb 8, 2024 •

edited by getdave

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

Testing on `trunk`

tomjn commented Feb 9, 2024 •

edited

Loading