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

[kt-12]

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

This ticket addresses - https://core.trac.wordpress.org/ticket/59969#comment:63

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[kt-12]

Drafted due to test failure.

[joemcgill]

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

[kt-12]

@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.

[joemcgill]

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.

[westonruter]

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 );
+		}
 	}
 }

[joemcgill]

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.

[kt-12]

moshiajonta
Please don't spam here.

Flagging this as spam, and even reported this to GitHub