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

59318: Add unit test demonstrating render method can return null #5177

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

kadamwhite
Copy link
Contributor

@kadamwhite kadamwhite commented Sep 8, 2023

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.

@kadamwhite
Copy link
Contributor Author

@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.

@costdev
Copy link
Contributor

costdev commented Sep 9, 2023

@kadamwhite Looks like this should be patched on the Gutenberg side, in this file.

@getdave
Copy link

getdave commented Sep 27, 2023

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() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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( [] );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$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 ) ) {
Copy link

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants