Template contents are wiped out on rename

[alshakero]

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:

1. Add an mu-plugin containing the following code: function somehook() {}; add_filter( 'hooked_block_types', 'somehook' , 10, 4 );.
2. Rename a custom template.
3. Its content will be wiped out.

Patch: ​https://github.com/WordPress/WordPress/pull/710

The ticket explains the problem.

Trac ticket: https://core.trac.wordpress.org/ticket/61550

[alshakero]

You can see a detailed bug report of this bug with a nice video ​here.

[hellofromTonya]

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.

[alshakero]

Hi @bernhard-reiter! Is there anything I can do to help get this merged?

[hellofromTonya]

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.

[hellofromTonya]

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.

[Bernhard Reiter]

Apologies, I somehow missed the pings on this ticket!
Thank you for reporting and for the patch @alshakero, I'll look into this today.

[Bernhard Reiter]

Cross-referencing ​https://github.com/WordPress/gutenberg/issues/59991, which was the same issue, but with wp_navigation post objects.

Oooh great catch! How about array_key_exists which seems even more careful?

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.

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.'
                );
        }

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 😊

[Bernhard Reiter]

In 58785:

Block Hooks: Don't erase post content if it isn't changed by client.

The inject_ignored_hooked_blocks_metadata_attributes filter that is attached to both the rest_pre_insert_wp_template and rest_pre_insert_wp_template_part hooks receives a stdClass object from the Templates REST API controller that contains all fields that the client would like to modify when making a POST request (plus the id to identify the relevant template or template part, respectively).

There are cases when the post_content field is not set, e.g. when the client would like to rename an existing template (in which case it would only set the title field).

Prior to this changeset, the filter would erroneously apply the Block Hooks algorithm to the non-existent post_content field regardless, which would result in it being set to the empty string ''. As a consequence, renaming a template would have the unwanted side effect of wiping its contents.

This changeset fixes the issue by returning early from the filter if the post_content field is not set.

Props alshakero, bernhard-reiter.
Fixes #61550.

Committed to Core in https://core.trac.wordpress.org/changeset/58785.

[Bernhard Reiter]

Re-opening to request approval from another Core Committer for backporting to the 6.6 branch.

[gziolo]

It makes perfect sense to include this fix in 6.6.x.

[Bernhard Reiter]

In 58802:

Block Hooks: Don't erase post content if it isn't changed by client.

The inject_ignored_hooked_blocks_metadata_attributes filter that is attached to both the rest_pre_insert_wp_template and rest_pre_insert_wp_template_part hooks receives a stdClass object from the Templates REST API controller that contains all fields that the client would like to modify when making a POST request (plus the id to identify the relevant template or template part, respectively).

There are cases when the post_content field is not set, e.g. when the client would like to rename an existing template (in which case it would only set the title field).

Prior to this changeset, the filter would erroneously apply the Block Hooks algorithm to the non-existent post_content field regardless, which would result in it being set to the empty string ''. As a consequence, renaming a template would have the unwanted side effect of wiping its contents.

This changeset fixes the issue by returning early from the filter if the post_content field is not set.

Reviewed by gziolo.
Merges [58785] to the 6.6 branch.

Props alshakero, bernhard-reiter.
Fixes #61550.

Backported to the 6.6 branch in https://core.trac.wordpress.org/changeset/58802.

Backported to the 6.6 branch in https://core.trac.wordpress.org/changeset/58802.