Update: REST API logic in Optimization Detective to be encapsulated into a static class #1865

hbhalodia · 2025-02-11T09:15:16Z

Summary

Relevant technical choices

Converted storage/rest-api.php to class based approach and named storage/class-od-rest-api.php.
Converted the test file from test-rest-api.php to test-class-rest-api.php.
Updated the constants and functions which were directly used within files and used the static way to call the functions and class variables.

…base This commit adds new class `class-od-rest-api.php` from `rest-api.php` file and adds the necessary required changes whereever normal functions were used, that are now refrencing the class static methods and variables. Also updated the test file test-rest-api.php to test-class-od-rest-api.php file and updated the required test cases as per the class. Note: Have directly used function docs and comment that were present in the original file.

codecov · 2025-02-11T09:24:08Z

Codecov Report

Attention: Patch coverage is 97.32620% with 5 lines in your changes missing coverage. Please review.

Project coverage is 66.71%. Comparing base (d21b74d) to head (97f6f3c).

Files with missing lines	Patch %	Lines
...timization-detective/storage/class-od-rest-api.php	98.32%	3 Missing ⚠️
plugins/optimization-detective/load.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1865      +/-   ##
==========================================
+ Coverage   66.64%   66.71%   +0.07%     
==========================================
  Files          88       88              
  Lines        7015     7025      +10     
==========================================
+ Hits         4675     4687      +12     
+ Misses       2340     2338       -2

Flag	Coverage Δ
multisite	`66.71% <97.32%> (+0.07%)`	⬆️
single	`36.42% <6.95%> (-0.86%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-02-11T09:36:02Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

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

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter · 2025-02-11T20:35:55Z

plugins/image-prioritizer/helper.php

-		OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) )
+		OD_Rest_API::OD_REST_API_NAMESPACE . OD_Rest_API::OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) )


We'll have to be careful here. If someone updates Image Prioritizer first without also updating Optimization Detective, then they'll start getting fatal errors. The same would happen if they update Optimization Detective first without updating Image Prioritizer.

We probably shouldn't make this change then, at least not yet. Unfortunately we can't catch when someone uses a constant to show a deprecation warning, but I wouldn't be too worried about other extensions outside of our own ones at this point.

So maybe we should just keep the global constants around a bit longer and add a TODO to remove them eventually, while already introducing the new ones and updating our extensions to use the new ones. It would probably be fine to remove the global constants in 1-2 months from now.

We could even put a big Do not use them! above in the code so that someone looking at the code doesn't get the wrong idea.

We can add @deprecated tags to the constants so they show up crossed-out when someone tries to use them.

westonruter · 2025-02-11T22:50:19Z

plugins/optimization-detective/storage/class-od-rest-api.php

+	/**
+	 * Namespace for optimization-detective.
+	 *
+	 * @since 0.1.0
+	 * @access private
+	 * @var string
+	 */
+	const OD_REST_API_NAMESPACE = 'optimization-detective/v1';
+
+	/**
+	 * Route for storing a URL Metric.
+	 *
+	 * Note the `:store` art of the endpoint follows Google's guidance in AIP-136 for the use of the POST method in a way
+	 * that does not strictly follow the standard usage. Namely, submitting a POST request to this endpoint will either
+	 * create a new `od_url_metrics` post, or it will update an existing post if one already exists for the provided slug.
+	 *
+	 * @since 0.1.0
+	 * @access private
+	 * @link https://google.aip.dev/136
+	 * @var string
+	 */
+	const OD_URL_METRICS_ROUTE = '/url-metrics:store';


Maybe we should learn from the trouble we had the global constants with regard to there not being a way to indicate these being deprecated and change them to instead be methods like get_namespace() and get_route() that return the strings. This will give us more flexibility in the future.

Hi @westonruter, So I should revert the global constant what it was before OD_REST_API_NAMESPACE and OD_URL_METRICS_ROUTE and also add these 2 constants in class as well and then create a static function in class like get_namespace() and get_route() so that we can use that function inside Optimization detective and for Image Priortizer for now we should just use the global constant, so it does not break that.

Also, Can we add a check to Image Priortizer to first check if global constant exists, if yes use that if not, check for the OD_Rest_API class and call these 2 static methods to get this value?

I think these constants could be moved to a new deprecated.php in Optimization Detective and then they can continue to be used in Image Prioritizer for the time being. In a subsequent release we'll update Image Prioritizer to use what whatever new method we determine for accessing the namespace and route, such as methods on the new class.

We can also specify the specific minimum version of Optimization Detective that Image Prioritizer requires here:

performance/plugins/image-prioritizer/helper.php

Line 24 in d21b74d

$required_od_version = '0.9.0';

But we can't go the other way around. So we have to keep those private deprecated constants in Optimization Detective around for awhile.

Okay, So creating a new method to access this from class is out of scope in this PR, correct?

So I also need to remove this constant from class as well for the time being and use global constant everywhere.

Within Optimization Detective itself we'll use the new method on the class. In fact, the methods can just return the constants for now. Eventually we'll get rid of the constants and have the new methods return strings directly.

Sounds Good, Would update the PR with required changes.

… for optimization detective This commit adds the deprecated.php file which has the global constant that are being inside image priortizer and updated the OD plugin to use this constants via a dedicated methods.

hbhalodia · 2025-02-12T04:45:42Z

Hi @westonruter, I have updated the PR with the required change, I need some inputs on,

I have used @since n.e.x.t, since I am not sure what exact version should be added.
Also, when I ran phpstan, it throws an error in image priortizer to use OD_Rest_API class methods to call those constant, as we have added a deprecated tag there, so how to remove that, for now I have committed using --no-verify.

Can you please suggest what should be there in both the cases?

Thank You,

westonruter · 2025-02-12T05:16:45Z

1. I have used @since n.e.x.t, since I am not sure what exact version should be added.

That is correct.

2. Also, when I ran phpstan, it throws an error in image priortizer to use OD_Rest_API class methods to call those constant, as we have added a deprecated tag there, so how to remove that, for now I have committed using --no-verify.

Here's one way: bd3f309.

plugins/image-prioritizer/tests/test-helper.php

plugins/optimization-detective/storage/class-od-rest-api.php

westonruter · 2025-02-12T05:21:37Z

plugins/optimization-detective/storage/class-od-rest-api.php

+ * @since 0.1.0
+ * @access private
+ */
+class OD_Rest_API {


Instead of OD_Rest_API how about OD_URL_Metric_Store_Endpoint. Or maybe to follow core's naming convention more: OD_REST_URL_Metric_Storage_Controller. Let's see what @felixarntz thinks before you rename it.

Yes, would wait for the confirmation from @felixarntz.

… prefix from methods

hbhalodia marked this pull request as ready for review February 11, 2025 09:35

hbhalodia requested review from westonruter and felixarntz as code owners February 11, 2025 09:35

westonruter reviewed Feb 11, 2025

View reviewed changes

Update the class to create new methods to get the namespace and route…

47e5ee8

… for optimization detective This commit adds the deprecated.php file which has the global constant that are being inside image priortizer and updated the OD plugin to use this constants via a dedicated methods.

hbhalodia and others added 2 commits February 12, 2025 10:34

Remove useless @return annotation

5edaddf

Remove class constants and address deprecated errors

bd3f309

hbhalodia commented Feb 12, 2025

View reviewed changes

plugins/image-prioritizer/tests/test-helper.php Show resolved Hide resolved

westonruter reviewed Feb 12, 2025

View reviewed changes

Update the class to remove @access private on methods and removed od_…

97f6f3c

… prefix from methods

westonruter mentioned this pull request Feb 12, 2025

REST API logic in Optimization Detective can be encapsulated into a static class #1859

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: REST API logic in Optimization Detective to be encapsulated into a static class #1865

Update: REST API logic in Optimization Detective to be encapsulated into a static class #1865

hbhalodia commented Feb 11, 2025

codecov bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

westonruter Feb 11, 2025

felixarntz Feb 11, 2025 •

edited

Loading

westonruter Feb 11, 2025

westonruter Feb 11, 2025

hbhalodia Feb 12, 2025

westonruter Feb 12, 2025

westonruter Feb 12, 2025

hbhalodia Feb 12, 2025

westonruter Feb 12, 2025

hbhalodia Feb 12, 2025

hbhalodia commented Feb 12, 2025

westonruter commented Feb 12, 2025

westonruter Feb 12, 2025

hbhalodia Feb 12, 2025

		OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) )
		OD_Rest_API::OD_REST_API_NAMESPACE . OD_Rest_API::OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) )

Update: REST API logic in Optimization Detective to be encapsulated into a static class #1865

Are you sure you want to change the base?

Update: REST API logic in Optimization Detective to be encapsulated into a static class #1865

Conversation

hbhalodia commented Feb 11, 2025

Summary

Relevant technical choices

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

github-actions bot commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

felixarntz Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbhalodia commented Feb 12, 2025

westonruter commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

felixarntz Feb 11, 2025 •

edited

Loading