Merge pull request #1824 from ShyamGadde/update/make-etag-required

Make ETag a required property of the URL Metric
WordPress · Jan 27, 2025 · 93257ee · 93257ee
2 parents 522660b + bd642b4
commit 93257ee
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 46 deletions.
diff --git a/plugins/optimization-detective/class-od-url-metric-group.php b/plugins/optimization-detective/class-od-url-metric-group.php
@@ -286,11 +286,6 @@ public function is_complete(): bool {
 					return false;
 				}
 
-				// The ETag is not populated yet, so this is stale. Eventually this will be required.
-				if ( $url_metric->get_etag() === null ) {
-					return false;
-				}
-
 				// The ETag of the URL Metric does not match the current ETag for the collection, so it is stale.
 				if ( ! hash_equals( $url_metric->get_etag(), $this->collection->get_current_etag() ) ) {
 					return false;

diff --git a/plugins/optimization-detective/class-od-url-metric.php b/plugins/optimization-detective/class-od-url-metric.php
@@ -39,7 +39,7 @@
  *                            }
  * @phpstan-type Data         array{
  *                                uuid: non-empty-string,
- *                                etag?: non-empty-string,
+ *                                etag: non-empty-string,
  *                                url: non-empty-string,
  *                                timestamp: float,
  *                                viewport: ViewportRect,
@@ -171,6 +171,7 @@ public function set_group( OD_URL_Metric_Group $group ): void {
 	 *
 	 * @since 0.1.0
 	 * @since 0.9.0 Added the 'etag' property to the schema.
+	 * @since n.e.x.t The 'etag' property is now required.
 	 *
 	 * @todo Cache the return value?
 	 *
@@ -230,7 +231,7 @@ public static function get_json_schema(): array {
 					'pattern'     => '^[0-9a-f]{32}\z',
 					'minLength'   => 32,
 					'maxLength'   => 32,
-					'required'    => false, // To be made required in a future release.
+					'required'    => true,
 					'readonly'    => true, // Omit from REST API.
 				),
 				'url'       => array(
@@ -446,12 +447,12 @@ public function get_uuid(): string {
 	 * Gets ETag.
 	 *
 	 * @since 0.9.0
+	 * @since n.e.x.t No longer returns null as 'etag' is now required.
 	 *
-	 * @return non-empty-string|null ETag.
+	 * @return non-empty-string ETag.
 	 */
-	public function get_etag(): ?string {
-		// Since the ETag is optional for now, return null for old URL Metrics that do not have one.
-		return $this->data['etag'] ?? null;
+	public function get_etag(): string {
+		return $this->data['etag'];
 	}
 
 	/**

diff --git a/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php b/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php
@@ -226,13 +226,6 @@ public static function store_url_metric( string $slug, OD_URL_Metric $new_url_me
 		}
 
 		$etag = $new_url_metric->get_etag();
-		if ( null === $etag ) {
-			// This case actually will never occur in practice because the store_url_metric function is only called
-			// in the REST API endpoint where the ETag parameter is required. It is here exclusively for the sake of
-			// PHPStan's static analysis. This entire condition can be removed in a future release when the 'etag'
-			// property becomes required.
-			return new WP_Error( 'missing_etag' );
-		}
 
 		$group_collection = new OD_URL_Metric_Group_Collection(
 			$url_metrics,

diff --git a/plugins/optimization-detective/tests/storage/test-class-od-url-metrics-post-type.php b/plugins/optimization-detective/tests/storage/test-class-od-url-metrics-post-type.php
@@ -90,6 +90,7 @@ public function data_provider_test_get_url_metrics_from_post(): array {
 		$valid_content = array(
 			array(
 				'url'       => home_url( '/' ),
+				'etag'      => md5( '' ),
 				'viewport'  => array(
 					'width'  => 640,
 					'height' => 480,

diff --git a/plugins/optimization-detective/tests/test-class-od-url-metric.php b/plugins/optimization-detective/tests/test-class-od-url-metric.php
@@ -31,8 +31,8 @@ public function data_provider_to_test_constructor(): array {
 		return array(
 			'valid_minimal'                   => array(
 				'data' => array(
-					// Note: The 'etag' field is currently optional, so this data is still valid without it.
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
@@ -127,14 +127,14 @@ static function ( $value ) {
 				'error' => 'OD_URL_Metric[etag] must be at most 32 characters long.',
 			),
 			'missing_etag'                    => array(
-				'data' => array(
+				'data'  => array(
 					'uuid'      => wp_generate_uuid4(),
 					'url'       => home_url( '/' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
 				),
-				// Note: Add error message 'etag is a required property of OD_URL_Metric.' when 'etag' becomes mandatory.
+				'error' => 'etag is a required property of OD_URL_Metric.',
 			),
 			'missing_viewport'                => array(
 				'data'  => array(
@@ -327,14 +327,9 @@ static function ( OD_Element $element ) {
 		$this->assertSame( $data['url'], $url_metric->get_url() );
 		$this->assertSame( $data['url'], $url_metric->get( 'url' ) );
 
-		// Note: When the 'etag' field becomes required, the else statement can be removed.
-		if ( array_key_exists( 'etag', $data ) ) {
-			$this->assertSame( $data['etag'], $url_metric->get_etag() );
-			$this->assertSame( $data['etag'], $url_metric->get( 'etag' ) );
-			$this->assertTrue( 1 === preg_match( '/^[a-f0-9]{32}$/', $url_metric->get_etag() ) );
-		} else {
-			$this->assertNull( $url_metric->get_etag() );
-		}
+		$this->assertSame( $data['etag'], $url_metric->get_etag() );
+		$this->assertSame( $data['etag'], $url_metric->get( 'etag' ) );
+		$this->assertTrue( 1 === preg_match( '/^[a-f0-9]{32}$/', $url_metric->get_etag() ) );
 
 		$this->assertTrue( wp_is_uuid( $url_metric->get_uuid() ) );
 		$this->assertSame( $url_metric->get_uuid(), $url_metric->get( 'uuid' ) );
@@ -370,6 +365,7 @@ public function data_provider_to_test_constructor_with_extended_schema(): array
 
 		$data = array(
 			'url'       => home_url( '/' ),
+			'etag'      => md5( '' ),
 			'viewport'  => $viewport,
 			'timestamp' => microtime( true ),
 			'elements'  => array( $valid_element ),
@@ -390,6 +386,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
@@ -424,6 +421,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
@@ -455,6 +453,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(),
@@ -478,6 +477,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(
@@ -516,6 +516,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array( $valid_element ),
@@ -545,6 +546,7 @@ static function ( array $properties ): array {
 				},
 				'data'   => array(
 					'url'       => home_url( '/' ),
+					'etag'      => md5( '' ),
 					'viewport'  => $viewport,
 					'timestamp' => microtime( true ),
 					'elements'  => array(
@@ -917,8 +919,7 @@ public function test_get_json_schema_extensibility( Closure $set_up, Closure $as
 	 */
 	protected function check_schema_subset( array $schema, string $path, bool $extended = false ): void {
 		$this->assertArrayHasKey( 'required', $schema, $path );
-		// Skipping the check for 'root/etag' as it is currently optional.
-		if ( ! $extended && 'root/etag' !== $path ) {
+		if ( ! $extended ) {
 			$this->assertTrue( $schema['required'], $path );
 		}
 		$this->assertArrayHasKey( 'type', $schema, $path );

diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group.php
@@ -73,6 +73,7 @@ public function data_provider_test_construction(): array {
 					new OD_URL_Metric(
 						array(
 							'url'       => home_url( '/' ),
+							'etag'      => md5( '' ),
 							'viewport'  => array(
 								'width'  => 1,
 								'height' => 2,
@@ -263,21 +264,6 @@ public function data_provider_test_is_complete(): array {
 				),
 				'expected_is_group_complete' => false,
 			),
-			// Note: The following test case will not be required once the ETag is mandatory in a future release.
-			'etag_missing'   => array(
-				'url_metric'                 => new OD_URL_Metric(
-					array(
-						'url'       => home_url( '/' ),
-						'viewport'  => array(
-							'width'  => 400,
-							'height' => 700,
-						),
-						'timestamp' => microtime( true ),
-						'elements'  => array(),
-					)
-				),
-				'expected_is_group_complete' => false,
-			),
 			'etag_mismatch'  => array(
 				'url_metric'                 => $this->get_sample_url_metric( array( 'etag' => md5( 'different_etag' ) ) ),
 				'expected_is_group_complete' => false,