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.

Sign up for GitHub

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

[Draft] Fix - Indirect modification of overloaded property WP_Block_Type::$variations has no effect #5915

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft

[Draft] Fix - Indirect modification of overloaded property WP_Block_Type::$variations has no effect #5915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/wp-includes/class-wp-block-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,10 @@ public function __construct( $block_type, $args = array() ) {
* @return string|string[]|null|void The value read from the new property if the first item in the array provided,
* null when value not found, or void when unknown property name provided.
*/
public function __get( $name ) {
public function &__get( $name ) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than making the whole magic __get() method return properties by reference, perhaps $this->get_variations could return the value by reference instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joemcgill I have tried that. The issue was even though $this->get_variations returns back the reference, __get creates a copy of $variations and only that copy is available to the outside world, giving rise to the same reference problem.

Copy link
Member

Choose a reason for hiding this comment

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

Right. That makes sense. Applying this change will cause whatever is returned from the get_block_type_variations filter to be written to the variations property, which could lead to a situation where the same modifications are recursively applied each time the property is accessed, which we should try to avoid.

I'm leaning towards marking this issue as a wontfix and writing documentation to show how to modify variations properly, and possibly even adding a PHP API to add/remove variations for a block rather than directly modifying the property.

Looking into the WP Directory, the only place that I could find direct modification of block variations was the instance we fixed for the Nav Link Block in Gutenberg.

Copy link
Member

Choose a reason for hiding this comment

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

What if the filter were just applied the first time the method is called? That is, if the get_block_type_variations filter were applied inside the if statement for when the $variations member variable is null? The whole method could be made private and only called when $variations is accessed the first time.

diff --git a/src/wp-includes/class-wp-block-type.php b/src/wp-includes/class-wp-block-type.php
index 33825a7888..11b195b52a 100644
--- a/src/wp-includes/class-wp-block-type.php
+++ b/src/wp-includes/class-wp-block-type.php
@@ -11,6 +11,7 @@
  * Core class representing a block type.
  *
  * @since 5.0.0
+ * @property array[] $variations Block variations.
  *
  * @see register_block_type()
  */
@@ -350,12 +351,13 @@ class WP_Block_Type {
 	 *
 	 * @param string $name Deprecated property name.
 	 *
-	 * @return string|string[]|null|void The value read from the new property if the first item in the array provided,
-	 *                                   null when value not found, or void when unknown property name provided.
+	 * @return string|string[]|array[]|void The value read from the new property if the first item in the array provided,
+	 *                                      null when value not found, or void when unknown property name provided.
 	 */
-	public function __get( $name ) {
+	public function &__get( $name ) {
 		if ( 'variations' === $name ) {
-			return $this->get_variations();
+			$this->set_variations();
+			return $this->variations;
 		}
 
 		if ( ! in_array( $name, $this->deprecated_properties, true ) ) {
@@ -365,13 +367,15 @@ class WP_Block_Type {
 		$new_name = $name . '_handles';
 
 		if ( ! property_exists( $this, $new_name ) || ! is_array( $this->{$new_name} ) ) {
-			return null;
+			return;
 		}
 
 		if ( count( $this->{$new_name} ) > 1 ) {
 			return $this->{$new_name};
 		}
-		return isset( $this->{$new_name}[0] ) ? $this->{$new_name}[0] : null;
+		if ( isset( $this->{$new_name}[0] ) ) {
+			return $this->{$new_name}[0];
+		}
 	}
 
 	/**
@@ -584,28 +588,26 @@ class WP_Block_Type {
 	}
 
 	/**
-	 * Get block variations.
+	 * Sets block variations.
 	 *
 	 * @since 6.5.0
-	 *
-	 * @return array[]
 	 */
-	public function get_variations() {
+	private function set_variations() {
 		if ( ! isset( $this->variations ) ) {
 			$this->variations = array();
 			if ( is_callable( $this->variation_callback ) ) {
 				$this->variations = call_user_func( $this->variation_callback );
 			}
-		}
 
-		/**
-		 * Filters the registered variations for a block type.
-		 *
-		 * @since 6.5.0
-		 *
-		 * @param array         $variations Array of registered variations for a block type.
-		 * @param WP_Block_Type $block_type The full block type object.
-		 */
-		return apply_filters( 'get_block_type_variations', $this->variations, $this );
+			/**
+			 * Filters the registered variations for a block type.
+			 *
+			 * @since 6.5.0
+			 *
+			 * @param array         $variations Array of registered variations for a block type.
+			 * @param WP_Block_Type $block_type The full block type object.
+			 */
+			$this->variations = apply_filters( 'get_block_type_variations', $this->variations, $this );
+		}
 	}
 }

Copy link
Member

Choose a reason for hiding this comment

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

My expectation of a filter would be that it would allow integrations to modify the registered variations on retrieval rather than modifying the stored variation object. For example, what if some code wanted to remove registered variations based on some context about the request? That could result in variations getting lost, particularly if there is any caching of registered variations in play. I would prefer to move the logic that prepares the variations by running callbacks to a separate private method that is then referenced here and in get_variations() if we wanted to go that route.

if ( 'variations' === $name ) {
return $this->get_variations();
$this->variations = $this->get_variations();
return $this->variations;
}

if ( ! in_array( $name, $this->deprecated_properties, true ) ) {
Expand Down
14 changes: 14 additions & 0 deletions tests/phpunit/tests/blocks/wpBlockType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,20 @@ public function test_get_block_type_variations_filter_variations() {
$this->assertSameSets( $obtained_variations, $expected_variations, 'The variations that was initially set should be filtered.' );
}

/**
* @ticket 60309
*/
public function test_can_access_original_varations_variable() {
$block_type = new WP_Block_Type(
'test/block',
array(
'title' => 'Test title',
)
);
$block_type->variations[] = array( 'name' => 'var1' );
$this->assertSameSets( array( array( 'name' => 'var1' ) ), $block_type->variations );
}

/**
* Mock variation callback.
*
Expand Down
Loading