-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
59318: Add unit test demonstrating render method can return null #5177
base: trunk
Are you sure you want to change the base?
59318: Add unit test demonstrating render method can return null #5177
Conversation
@tomjn I'm not sure what to make of the "Ensure version-controlled files are not modified or deleted" error, that seems... tautological, if we're submitting a code change... but, I think the other PHPCS error is unrelated to this patch. May not be a perfect solution but hope it helps your advocacy for the ticket. |
@kadamwhite Looks like this should be patched on the Gutenberg side, in this file. |
Please see WordPress/gutenberg#54354 which appears to fix this. Would it be good to include the unit test coverage in that PR and then they can be both backported from the Plugin to Core as part of 6.4? |
/** | ||
* @ticket 59318 | ||
*/ | ||
public function test_render_block_core_template_part_missing_template() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function test_render_block_core_template_part_missing_template() { | |
public function test_returns_empty_string_when_template_is_missing() { |
Question: would it be better to rename test to describe what it is testing?
Also should we add test coverage for when debug is truthy to assert that one of the following happens (depending on what is missing - see code):
- empty string (placeholder)
- error message
* @ticket 59318 | ||
*/ | ||
public function test_render_block_core_template_part_missing_template() { | ||
$output = render_block_core_template_part( [] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$output = render_block_core_template_part( [] ); | |
$output = render_block_core_template_part( array() ); |
if ( ! isset( $attributes['slug'] ) ) { | ||
// If there is no slug this is a placeholder and we dont want to return any message. | ||
return; | ||
if ( is_null( $content ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me if I'm telling you something you already know but just in case - this change should be implemented upstream in the Gutenberg repo and backported to Core 🙇
I think we may already be looking to do this in WordPress/gutenberg#54354 ?
Implements failing unit test demonstrating that render_block_core_template_part method can return NULL in some situations. This can go on to cause a fatal elsewhere, as detailed in 59318.
Trac ticket: https://core.trac.wordpress.org/ticket/59318
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.