#61550 closed defect (bug) (fixed)
Template contents are wiped out on rename
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.6.2 | Priority: | normal |
Severity: | minor | Version: | 6.6 |
Component: | Editor | Keywords: | has-patch fixed-major dev-reviewed |
Focuses: | Cc: |
Description
If you have a custom filter hooked to the hooked_block_types
, renaming a wp_template
will wipe its contents.
This stems from the callback inject_ignored_hooked_blocks_metadata_attributes
checking has_filter( 'hooked_block_types' )
and filtering the template content when this call returns true. You can see the code here.
This is fine, but it assumes that the changes request will always have the post content, which is not true in the case of renaming. This assumption results in this code running, even when there are no changes to the post content, which sets the post_content
to an empty string, overwriting the original content.
To reproduce:
- Add an
mu-plugin
containing the following code:function somehook() {}; add_filter( 'hooked_block_types', 'somehook' , 10, 4 );
. - Rename a custom template.
- Its content will be wiped out.
Change History (20)
This ticket was mentioned in PR #6955 on WordPress/wordpress-develop by alshakero.
8 months ago
#1
#3
@
8 months ago
This stems from the callback inject_ignored_hooked_blocks_metadata_attributes checking has_filter( 'hooked_block_types' ) and filtering the template content when this call returns true. You can see the code here.
This ticket identifies [58614] / #60854 as the possible change introducing the issue. Pinging @bernhard-reiter for awareness.
#5
@
7 months ago
- Milestone changed from Awaiting Review to 6.6.1
With 6.6 RC3 released yesterday, it's the last scheduled RC in the cycle. So I'll milestone this ticket for the first minor to raise its visibility.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
7 months ago
#7
@
7 months ago
- Keywords needs-testing needs-testing-info added
- Milestone changed from 6.6.1 to 6.6.2
As discussed in today's bug scrub, this ticket needs investigation. With a tentative plan to do a 6.6.1 RC as soon as tomorrow, moving this ticket to 6.6.2.
#8
@
7 months ago
Apologies, I somehow missed the pings on this ticket!
Thank you for reporting and for the patch @alshakero, I'll look into this today.
#9
@
7 months ago
Cross-referencing https://github.com/WordPress/gutenberg/issues/59991, which was the same issue, but with wp_navigation
post objects.
@alshakero commented on PR #6955:
7 months ago
#10
Oooh great catch! How about array_key_exists
which seems even more careful?
@Bernhard Reiter commented on PR #6955:
7 months ago
#11
Oooh great catch! How about ~
array_key_exists
~property_exists
which seems even more careful?
So the only difference between property_exists()
and isset()
is that the latter returns false
if the property exists but is null
, right? In that case, I'd stick with if ( ! isset( $changes->post_content ) ) { return $changes; }
, since it'll bail if $changes->post_content === null
. This is fine IMO, since it's unclear how we'd otherwise apply the Block Hooks algorithm to a post content that's null
; we'd effectively have to cast it to ''
after all.
It's also more consistent with what we're doing with `wp_navigation` 🙂 Finally, it's hopefully a rather academic scenario only that $changes->post_content === null
.
@alshakero commented on PR #6955:
7 months ago
#12
Hi @ockham! Thanks for addressing your feedback. I was working on it at the same time and only realized because I couldn't push. Here's my take on the test in case it helps
/**
* @ticket 61550
*/
public function test_hooked_block_types_filter_with_renamed_template_part() {
$action = new MockAction();
add_filter( 'hooked_block_types', array( $action, 'filter' ), 10, 4 );
$changes = new stdClass();
$changes->ID = 1;
$changes->post_type = 'wp_template_part';
$changes->post_title = 'new title';
$cloned_changes = unserialize( serialize( $changes ) );
$result = inject_ignored_hooked_blocks_metadata_attributes( $changes );
// Ensure same reference.
$this->assertSame(
$changes,
$result,
'Should leave the changes object untouched if `post_content` is not set.'
);
// Ensure same values.
$this->assertTrue(
$result == $cloned_changes, // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual -- == compares all props and their values.
'Should leave the changes object untouched if `post_content` is not set.'
);
$changes = new stdClass();
$changes->ID = 1;
$changes->post_type = 'wp_template_part';
$changes->post_title = 'new title';
$changes->post_content = 'new content';
$cloned_changes = unserialize( serialize( $changes ) );
$result = inject_ignored_hooked_blocks_metadata_attributes( $changes );
$this->assertNotSame(
$changes,
$result,
'Should process the changes object if `post_content` is set.'
);
$changes = new stdClass();
$changes->ID = 1;
$changes->post_type = 'wp_template_part';
$changes->post_title = 'new title';
$changes->post_content = '';
$cloned_changes = unserialize( serialize( $changes ) );
$result = inject_ignored_hooked_blocks_metadata_attributes( $changes );
$this->assertNotSame(
$changes,
$result,
'Should process the changes object if `post_content` is set to an empty string.'
);
}
@Bernhard Reiter commented on PR #6955:
7 months ago
#13
Hi @ockham! Thanks for addressing your feedback. I was working on it at the same time and only realized because I couldn't push.
Oh no, I should've posted a comment to let you know! Apologies, I didn't mean to make you duplicate work.
Anyway, thank you for sharing your patch! I'll have a look if there's anything to incorporate into the tests and will probably land the PR later today 😊
#14
@
7 months ago
- Owner set to Bernhard Reiter
- Resolution set to fixed
- Status changed from new to closed
In 58785:
@Bernhard Reiter commented on PR #6955:
7 months ago
#15
Committed to Core in https://core.trac.wordpress.org/changeset/58785.
#16
@
7 months ago
- Keywords dev-feedback fixed-major added; needs-testing needs-testing-info removed
- Resolution fixed deleted
- Status changed from closed to reopened
Re-opening to request approval from another Core Committer for backporting to the 6.6 branch.
#17
@
7 months ago
- Keywords dev-reviewed added; dev-feedback removed
It makes perfect sense to include this fix in 6.6.x.
@Bernhard Reiter commented on PR #6955:
7 months ago
#19
Backported to the 6.6 branch in https://core.trac.wordpress.org/changeset/58802.
@Bernhard Reiter commented on PR #6955:
7 months ago
#20
Backported to the 6.6 branch in https://core.trac.wordpress.org/changeset/58802.
The ticket explains the problem.
Trac ticket: https://core.trac.wordpress.org/ticket/61550