Eliminate boolean arg to get_xpath in favor of new get_stored_xpath method

Add todos to remove transitional XPath logic

Co-authored-by: felixarntz <[email protected]>

Author: westonruter

plugins/optimization-detective/class-od-element.php

@@ -37,6 +37,7 @@ class OD_Element implements ArrayAccess, JsonSerializable {
 	 * Transitional XPath.
 	 *
 	 * @since n.e.x.t
+	 * @todo Remove logic related to transitional_xpath in a subsequent release once URL Metrics have been collected with the new format.
 	 * @var non-empty-string|null
 	 */
 	protected $transitional_xpath = null;
@@ -131,6 +132,7 @@ public function is_lcp_candidate(): bool {
 	 *
 	 * @since 0.7.0
 	 * @since n.e.x.t Returns the transitional XPath format. To access the underlying raw XPath, access the 'xpath' key of the jsonSerialize response.
+	 * @todo Remove logic related to transitional_xpath in a subsequent release once URL Metrics have been collected with the new format.
 	 *
 	 * @return non-empty-string XPath.
 	 */

plugins/optimization-detective/class-od-html-tag-processor.php

@@ -186,26 +186,28 @@ final class OD_HTML_Tag_Processor extends WP_HTML_Tag_Processor {
 	private $bookmarked_open_stacks = array();
 
 	/**
-	 * XPath for the current tag.
+	 * Stored XPath for the current tag.
 	 *
-	 * This is used so that repeated calls to {@see self::get_xpath()} won't needlessly reconstruct the string. This
-	 * gets cleared whenever {@see self::open_tags()} iterates to the next tag.
+	 * This is used so that repeated calls to {@see self::get_stored_xpath()} won't needlessly reconstruct the string.
+	 * This gets cleared whenever {@see self::open_tags()} iterates to the next tag.
+	 *
+	 * @todo Remove this once the XPath transitional period is over.
 	 *
 	 * @since 0.4.0
 	 * @var string|null
 	 */
-	private $current_xpath = null;
+	private $current_stored_xpath = null;
 
 	/**
-	 * Transitional XPath for the current tag.
+	 * (Transitional) XPath for the current tag.
 	 *
 	 * This is used to store the old XPath format in a transitional period until which new URL Metrics are expected to
 	 * have been collected to purge out references to the old format.
 	 *
 	 * @since n.e.x.t
 	 * @var string|null
 	 */
-	private $transitional_current_xpath = null;
+	private $current_xpath = null;
 
 	/**
 	 * Whether the previous tag does not expect a closer.
@@ -312,8 +314,8 @@ public function expects_closer( ?string $tag_name = null ): bool {
 	 * @return bool Whether a token was parsed.
 	 */
 	public function next_token(): bool {
-		$this->current_xpath              = null; // Clear cache.
-		$this->transitional_current_xpath = null; // Clear cache.
+		$this->current_stored_xpath = null; // Clear cache.
+		$this->current_xpath        = null; // Clear cache.
 		++$this->cursor_move_count;
 		if ( ! parent::next_token() ) {
 			$this->open_stack_tags       = array();
@@ -639,40 +641,55 @@ private function is_foreign_element(): bool {
 	 * index of the preceding node set. So it has to rather be written `.../*[1][self::DIV]/*[2][self::DIV]`.
 	 * Note that the first three levels lack any node index whereas the third level includes a disambiguating
 	 * attribute predicate (e.g. `/HTML/BODY/DIV[@id="page"]`) for the reasons explained in {@see self::XPATH_PATTERN}.
+	 * This predicate will be included once the transitional period is over.
 	 *
 	 * @since 0.4.0
+	 * @todo Replace the logic herein with what is in get_stored_xpath() once the transitional period is over.
 	 *
-	 * @param bool $transitional_format Whether to use the transitional XPath format. Default true.
 	 * @return string XPath.
 	 */
-	public function get_xpath( bool $transitional_format = true ): string {
+	public function get_xpath(): string {
 		/*
 		 * This transitional format is used by default for all extensions. The non-transitional format is used only in
 		 * od_optimize_template_output_buffer() when setting the data-od-xpath attribute. This is so that the new format
 		 * will replace the old format as new URL Metrics are collected. After a month of the new format being live, the
 		 * transitional format can be eliminated. See the corresponding logic in OD_Element for normalizing both the
 		 * old and new XPath formats to use the transitional format.
 		 */
-		if ( $transitional_format ) {
-			if ( null === $this->transitional_current_xpath ) {
-				$this->transitional_current_xpath = '';
-				foreach ( $this->get_indexed_breadcrumbs() as $i => list( $tag_name, $index, $attributes ) ) {
-					if ( $i < 2 || ( 2 === $i && '/HTML/BODY' === $this->transitional_current_xpath ) ) {
-						$this->transitional_current_xpath .= "/$tag_name";
-					} else {
-						$this->transitional_current_xpath .= sprintf( '/*[%d][self::%s]', $index + 1, $tag_name );
-					}
+		if ( null === $this->current_xpath ) {
+			$this->current_xpath = '';
+			foreach ( $this->get_indexed_breadcrumbs() as $i => list( $tag_name, $index, $attributes ) ) {
+				if ( $i < 2 || ( 2 === $i && '/HTML/BODY' === $this->current_xpath ) ) {
+					$this->current_xpath .= "/$tag_name";
+				} else {
+					$this->current_xpath .= sprintf( '/*[%d][self::%s]', $index + 1, $tag_name );
 				}
 			}
-			return $this->transitional_current_xpath;
 		}
+		return $this->current_xpath;
+	}
 
-		if ( null === $this->current_xpath ) {
-			$this->current_xpath = '';
+	/**
+	 * Gets stored XPath for the current open tag.
+	 *
+	 * This method is temporary for a transition period while new URL Metrics are collected for active installs. Once
+	 * the transition period is over, the logic in this method can be moved to {@see self::get_xpath()} and this method
+	 * can simply be an alias for that one. See related logic in {@see OD_Element::get_xpath()}. This function is only
+	 * used internally by Optimization Detective in {@see od_optimize_template_output_buffer()}.
+	 *
+	 * @since n.e.x.t
+	 * @todo Move the logic in this method to the get_xpath() method and let this be an alias for that method once the transitional period is over.
+	 * @access private
+	 *
+	 * @return string XPath.
+	 */
+	public function get_stored_xpath(): string {
+		if ( null === $this->current_stored_xpath ) {
+			$this->current_stored_xpath = '';
 			foreach ( $this->get_indexed_breadcrumbs() as $i => list( $tag_name, $index, $attributes ) ) {
 				if ( $i < 2 ) {
-					$this->current_xpath .= "/$tag_name";
-				} elseif ( 2 === $i && '/HTML/BODY' === $this->current_xpath ) {
+					$this->current_stored_xpath .= "/$tag_name";
+				} elseif ( 2 === $i && '/HTML/BODY' === $this->current_stored_xpath ) {
 					$segment = "/$tag_name";
 					foreach ( $attributes as $attribute_name => $attribute_value ) {
 						$segment .= sprintf(
@@ -681,13 +698,13 @@ public function get_xpath( bool $transitional_format = true ): string {
 							$attribute_value // Note: $attribute_value has already been validated to only contain safe characters /^[a-zA-Z0-9_.\s:-]*/ which do not need escaping.
 						);
 					}
-					$this->current_xpath .= $segment;
+					$this->current_stored_xpath .= $segment;
 				} else {
-					$this->current_xpath .= sprintf( '/*[%d][self::%s]', $index + 1, $tag_name );
+					$this->current_stored_xpath .= sprintf( '/*[%d][self::%s]', $index + 1, $tag_name );
 				}
 			}
 		}
-		return $this->current_xpath;
+		return $this->current_stored_xpath;
 	}
 
 	/**

plugins/optimization-detective/optimization.php

@@ -249,8 +249,8 @@ function od_optimize_template_output_buffer( string $buffer ): string {
 		$processor->release_bookmark( $current_tag_bookmark );
 
 		if ( $tracked_in_url_metrics && $needs_detection ) {
-			// Note: This is explicitly using the non-transitional XPath format since this is what will get saved in newly-submitted URL Metrics.
-			$xpath = $processor->get_xpath( false );
+			// TODO: Replace get_stored_xpath with get_xpath once the transitional period is over.
+			$xpath = $processor->get_stored_xpath();
 			$processor->set_meta_attribute( 'xpath', $xpath );
 		}
 	} while ( $processor->next_open_tag() );

plugins/optimization-detective/tests/test-class-od-html-tag-processor.php

@@ -433,18 +433,18 @@ public function data_provider_sample_documents(): array {
 	 */
 	public function test_next_tag_and_get_xpath( string $document, array $open_tags, array $xpath_breadcrumbs ): void {
 		$p = new OD_HTML_Tag_Processor( $document );
-		$this->assertSame( '', $p->get_xpath( false ), 'Expected empty XPath since iteration has not started.' );
+		$this->assertSame( '', $p->get_stored_xpath(), 'Expected empty XPath since iteration has not started.' );
 		$actual_open_tags                 = array();
 		$actual_xpath_breadcrumbs_mapping = array();
 		while ( $p->next_open_tag() ) {
 			$actual_open_tags[] = $p->get_tag();
 
-			$xpath = $p->get_xpath( false );
+			$xpath = $p->get_stored_xpath();
 			$this->assertArrayNotHasKey( $xpath, $actual_xpath_breadcrumbs_mapping, 'Each tag must have a unique XPath.' );
 
 			$actual_xpath_breadcrumbs_mapping[ $xpath ] = $p->get_breadcrumbs();
 
-			$transitional_xpath = $p->get_xpath( true );
+			$transitional_xpath = $p->get_xpath();
 			$this->assertRegExp(
 				'#^/HTML(
 					/HEAD(/\*\[\d+]\[self::\w+])?
@@ -625,7 +625,7 @@ public function test_bookmarking_and_seeking(): void {
 						$bookmarks[]              = $bookmark;
 						$actual_figure_contents[] = array(
 							'tag'   => $processor->get_tag(),
-							'xpath' => $processor->get_xpath( false ),
+							'xpath' => $processor->get_stored_xpath(),
 							'depth' => $processor->get_current_depth(),
 						);
 					}
@@ -666,7 +666,7 @@ public function test_bookmarking_and_seeking(): void {
 			$processor->seek( $bookmark );
 			$sought_actual_contents[] = array(
 				'tag'   => $processor->get_tag(),
-				'xpath' => $processor->get_xpath( false ),
+				'xpath' => $processor->get_stored_xpath(),
 				'depth' => $processor->get_current_depth(),
 			);
 		}