Code improvements for the SSR part of the Interactivity API (#51640)

* Fix multi-line comments and add examples * Add parse_attribute_name static method to WP_Directive_Processor * Replace array functions with a foreach loop * Add explanatory comment for the negation operator check * Replace $array with $path_segments * Minor fix for the negation operator comment * Call only instances of Closure * Improve negation operator code style * Do not lower-case tags * Use static parse_attribute_name inside directive processors * Add basic error handling in wp-context * Fix hidden identation errors * Use the correct variable name * Fix test for evaluating functions * Remove references to "attribute" directives * Remove emtpy lines in multi-line function calls * Fix typo --------- Co-authored-by: Luis Herranz <[email protected]>
WordPress · Jun 23, 2023 · 8758865 · 8758865
1 parent 1ed1e34
commit 8758865
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 36 deletions.
diff --git a/lib/experimental/interactivity-api/class-wp-directive-processor.php b/lib/experimental/interactivity-api/class-wp-directive-processor.php
@@ -173,4 +173,21 @@ public static function is_html_void_element( $tag_name ) {
 				return false;
 		}
 	}
+
+	/**
+	 * Extract and return the directive type and the the part after the double
+	 * hyphen from an attribute name (if present), in an array format.
+	 *
+	 * Examples:
+	 *
+	 *     'wp-island'            => array( 'wp-island', null )
+	 *     'wp-bind--src'         => array( 'wp-bind', 'src' )
+	 *     'wp-thing--and--thang' => array( 'wp-thing', 'and--thang' )
+	 *
+	 * @param string $name The attribute name.
+	 * @return array The resulting array
+	 */
+	public static function parse_attribute_name( $name ) {
+		return explode( '--', $name, 2 );
+	}
 }
diff --git a/lib/experimental/interactivity-api/directive-processing.php b/lib/experimental/interactivity-api/directive-processing.php
@@ -69,7 +69,7 @@ function gutenberg_interactivity_process_directives( $tags, $prefix, $directives
 	$tag_stack = array();
 
 	while ( $tags->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
-		$tag_name = strtolower( $tags->get_tag() );
+		$tag_name = $tags->get_tag();
 
 		// Is this a tag that closes the latest opening tag?
 		if ( $tags->is_tag_closer() ) {
@@ -81,27 +81,31 @@ function gutenberg_interactivity_process_directives( $tags, $prefix, $directives
 			if ( $latest_opening_tag_name === $tag_name ) {
 				array_pop( $tag_stack );
 
-				// If the matching opening tag didn't have any attribute directives,
-				// we move on.
+				// If the matching opening tag didn't have any directives, we move on.
 				if ( 0 === count( $attributes ) ) {
 					continue;
 				}
 			}
 		} else {
-			// Helper that removes the part after the double hyphen before looking for
-			// the directive processor inside `$attribute_directives`.
-			$get_directive_type = function ( $attr ) {
-				return explode( '--', $attr )[0];
-			};
-
-			$attributes = $tags->get_attribute_names_with_prefix( $prefix );
-			$attributes = array_map( $get_directive_type, $attributes );
-			$attributes = array_intersect( $attributes, array_keys( $directives ) );
-
-			// If this is an open tag, and if it either has attribute directives, or
-			// if we're inside a tag that does, take note of this tag and its
-			// attribute directives so we can call its directive processor once we
-			// encounter the matching closing tag.
+			$attributes = array();
+			foreach ( $tags->get_attribute_names_with_prefix( $prefix ) as $name ) {
+				/*
+				 * Removes the part after the double hyphen before looking for
+				 * the directive processor inside `$directives`, e.g., "wp-bind"
+				 * from "wp-bind--src" and "wp-context" from "wp-context" etc...
+				 */
+				list( $type ) = WP_Directive_Processor::parse_attribute_name( $name );
+				if ( array_key_exists( $type, $directives ) ) {
+					$attributes[] = $type;
+				}
+			}
+
+			/*
+			 * If this is an open tag, and if it either has directives, or if
+			 * we're inside a tag that does, take note of this tag and its
+			 * directives so we can call its directive processor once we
+			 * encounter the matching closing tag.
+			 */
 			if (
 				! WP_Directive_Processor::is_html_void_element( $tags->get_tag() ) &&
 				( 0 !== count( $attributes ) || 0 !== count( $tag_stack ) )
@@ -131,26 +135,36 @@ function gutenberg_interactivity_evaluate_reference( $path, array $context = arr
 		array( 'context' => $context )
 	);
 
-	if ( strpos( $path, '!' ) === 0 ) {
-		$path                  = substr( $path, 1 );
-		$has_negation_operator = true;
-	}
-
-	$array   = explode( '.', $path );
-	$current = $store;
-	foreach ( $array as $p ) {
+	/*
+	 * Check first if the directive path is preceded by a negator operator (!),
+	 * indicating that the value obtained from the Interactivity Store (or the
+	 * passed context) using the subsequent path should be negated.
+	 */
+	$should_negate_value = '!' === $path[0];
+
+	$path          = $should_negate_value ? substr( $path, 1 ) : $path;
+	$path_segments = explode( '.', $path );
+	$current       = $store;
+	foreach ( $path_segments as $p ) {
 		if ( isset( $current[ $p ] ) ) {
 			$current = $current[ $p ];
 		} else {
 			return null;
 		}
 	}
 
-	// Check if $current is a function and if so, call it passing the store.
-	if ( is_callable( $current ) ) {
+	/*
+	 * Check if $current is an anonymous function or an arrow function, and if
+	 * so, call it passing the store. Other types of callables are ignored on
+	 * purpose, as arbitrary strings or arrays could be wrongly evaluated as
+	 * "callables".
+	 *
+	 * E.g., "file" is an string and a "callable" (the "file" function exists).
+	 */
+	if ( $current instanceof Closure ) {
 		$current = call_user_func( $current, $store );
 	}
 
 	// Return the opposite if it has a negator operator (!).
-	return isset( $has_negation_operator ) ? ! $current : $current;
+	return $should_negate_value ? ! $current : $current;
 }
diff --git a/lib/experimental/interactivity-api/directives/wp-bind.php b/lib/experimental/interactivity-api/directives/wp-bind.php
@@ -20,7 +20,7 @@ function gutenberg_interactivity_process_wp_bind( $tags, $context ) {
 	$prefixed_attributes = $tags->get_attribute_names_with_prefix( 'data-wp-bind--' );
 
 	foreach ( $prefixed_attributes as $attr ) {
-		list( , $bound_attr ) = explode( '--', $attr );
+		list( , $bound_attr ) = WP_Directive_Processor::parse_attribute_name( $attr );
 		if ( empty( $bound_attr ) ) {
 			continue;
 		}

diff --git a/lib/experimental/interactivity-api/directives/wp-class.php b/lib/experimental/interactivity-api/directives/wp-class.php
@@ -20,7 +20,7 @@ function gutenberg_interactivity_process_wp_class( $tags, $context ) {
 	$prefixed_attributes = $tags->get_attribute_names_with_prefix( 'data-wp-class--' );
 
 	foreach ( $prefixed_attributes as $attr ) {
-		list( , $class_name ) = explode( '--', $attr );
+		list( , $class_name ) = WP_Directive_Processor::parse_attribute_name( $attr );
 		if ( empty( $class_name ) ) {
 			continue;
 		}

diff --git a/lib/experimental/interactivity-api/directives/wp-context.php b/lib/experimental/interactivity-api/directives/wp-context.php
@@ -24,7 +24,10 @@ function gutenberg_interactivity_process_wp_context( $tags, $context ) {
 	}
 
 	$new_context = json_decode( $value, true );
-	// TODO: Error handling.
+	if ( null === $new_context ) {
+		// Invalid JSON defined in the directive.
+		return;
+	}
 
 	$context->set_context( $new_context );
 }
diff --git a/lib/experimental/interactivity-api/directives/wp-style.php b/lib/experimental/interactivity-api/directives/wp-style.php
@@ -20,7 +20,7 @@ function gutenberg_interactivity_process_wp_style( $tags, $context ) {
 	$prefixed_attributes = $tags->get_attribute_names_with_prefix( 'data-wp-style--' );
 
 	foreach ( $prefixed_attributes as $attr ) {
-		list( , $style_name ) = explode( '--', $attr );
+		list( , $style_name ) = WP_Directive_Processor::parse_attribute_name( $attr );
 		if ( empty( $style_name ) ) {
 			continue;
 		}

diff --git a/phpunit/experimental/interactivity-api/directive-processing-test.php b/phpunit/experimental/interactivity-api/directive-processing-test.php
@@ -127,7 +127,7 @@ public function test_evaluate_function_should_return_null_for_unresolved_paths()
 		$this->assertNull( gutenberg_interactivity_evaluate_reference( 'this.property.doesnt.exist' ) );
 	}
 
-	public function test_evaluate_function_should_execute_functions() {
+	public function test_evaluate_function_should_execute_anonymous_functions() {
 		$context = new WP_Directive_Context( array( 'count' => 2 ) );
 		$helper  = new Helper_Class;
 
@@ -140,6 +140,7 @@ public function test_evaluate_function_should_execute_functions() {
 					'anonymous_function'           => function( $store ) {
 						return $store['state']['count'] + $store['context']['count'];
 					},
+					// Other types of callables should not be executed.
 					'function_name'                => 'gutenberg_test_process_directives_helper_increment',
 					'class_method'                 => array( $helper, 'increment' ),
 					'class_static_method'          => 'Helper_Class::static_increment',
@@ -149,8 +150,21 @@ public function test_evaluate_function_should_execute_functions() {
 		);
 
 		$this->assertSame( 5, gutenberg_interactivity_evaluate_reference( 'selectors.anonymous_function', $context->get_context() ) );
-		$this->assertSame( 5, gutenberg_interactivity_evaluate_reference( 'selectors.function_name', $context->get_context() ) );
-		$this->assertSame( 5, gutenberg_interactivity_evaluate_reference( 'selectors.class_static_method', $context->get_context() ) );
-		$this->assertSame( 5, gutenberg_interactivity_evaluate_reference( 'selectors.class_static_method_as_array', $context->get_context() ) );
+		$this->assertSame(
+			'gutenberg_test_process_directives_helper_increment',
+			gutenberg_interactivity_evaluate_reference( 'selectors.function_name', $context->get_context() )
+		);
+		$this->assertSame(
+			array( $helper, 'increment' ),
+			gutenberg_interactivity_evaluate_reference( 'selectors.class_method', $context->get_context() )
+		);
+		$this->assertSame(
+			'Helper_Class::static_increment',
+			gutenberg_interactivity_evaluate_reference( 'selectors.class_static_method', $context->get_context() )
+		);
+		$this->assertSame(
+			array( 'Helper_Class', 'static_increment' ),
+			gutenberg_interactivity_evaluate_reference( 'selectors.class_static_method_as_array', $context->get_context() )
+		);
 	}
 }