Make WordPress Core

Opened 3 weeks ago

Last modified 3 hours ago

#61550 reopened defect (bug)

Template contents are wiped out on rename

Reported by: alshakero's profile alshakero Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: 6.6.2 Priority: normal
Severity: minor Version: 6.6
Component: Editor Keywords: has-patch dev-feedback fixed-major
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:

  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

Change History (16)

This ticket was mentioned in PR #6955 on WordPress/wordpress-develop by alshakero.


3 weeks ago
#1

The ticket explains the problem.

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

#2 @alshakero
3 weeks ago

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

#3 @hellofromTonya
3 weeks 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.

#4 @alshakero
2 weeks ago

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

#5 @hellofromTonya
13 days 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.


6 days ago

#7 @hellofromTonya
6 days 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 @Bernhard Reiter
5 days 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 @Bernhard Reiter
5 days 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:


5 days ago
#10

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

@Bernhard Reiter commented on PR #6955:


5 days 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:


25 hours 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:


10 hours 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 @Bernhard Reiter
5 hours ago

  • Owner set to Bernhard Reiter
  • Resolution set to fixed
  • Status changed from new to closed

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.

#16 @Bernhard Reiter
3 hours 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.

Note: See TracTickets for help on using tickets.