Make WordPress Core

Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#60309 closed defect (bug) (duplicate)

Indirect modification of overloaded property WP_Block_Type::$variations has no effect

Reported by: thekt12's profile thekt12 Owned by: thekt12's profile thekt12
Milestone: Priority: normal
Severity: blocker Version: 6.5
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Original bug reported here - https://github.com/WordPress/wordpress-develop/pull/5718#issuecomment-1901940214

This bug was introduced after the merging of https://core.trac.wordpress.org/ticket/59969

The issue is

The setting of WP_Block_Type::$variations from the outside in a normal way like
$block_type->variations = array( array(...), ... ) works fine.
However if we try to set it especially $block_type->variations[] = array( ... ); it throws
Indirect modification of overloaded property WP_Block_Type::$variations has no effect

The reason is WP_Block_Type::$variations is only accessible via the magic __get, __set function. __get always returns a copy of the original attribute variable and not the reference of the actual attribute variable.

So, when we do $block_type->variations = array( array(...), ... ) we are only setting the temporary variable which is then processed by __set to set the original attribute.

But this fails when we try to set the variable in a way where it's original state is checked like in ->
$block_type->variations[] = array( ... );; Here, we try to add to the original variable array.

The way to fix this is by trying to get the reference of the original variable from __get instead of copy of variable

Change History (10)

#2 @ellatrix
4 months ago

Either we'll need to get this fix in ASAP, or we'll need to revert #59969. Currently all Gutenberg PRs are blocked from merging.

This ticket was mentioned in Slack in #core-editor by wildworks. View the logs.


4 months ago

#4 @Mamaduka
4 months ago

I believe the failing test case is phpunit/blocks/block-navigation-link-variations-test.php. We can confirm that the solution works by running the same test on the trunk.

@thekt12 commented on PR #5915:


4 months ago
#5

Drafted due to test failure.

#6 @joemcgill
4 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

The PHPUnit failures in the Gutenberg repo have been resolved via https://github.com/WordPress/gutenberg/pull/58090. I'm going to close this ticket and we can continue any other back-compat concerns with the changes to block variations in #59969 so everything stays in one place.

@thekt12 I think https://github.com/WordPress/wordpress-develop/pull/5915 still has merit to explore further, do you mind updating the PR description to point to #59969 instead? I'm still not confident that we want to support this type of direct modification of overloaded properties, but if we can find an easy solution that makes the class more robust, I'm not opposed.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


4 months ago

@thekt12 commented on PR #5915:


4 months ago
#9

moshiajonta

Please don't spam here.

Flagging this as spam, and even reported this to GitHub

Note: See TracTickets for help on using tickets.