Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into 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.

TODO

Incorporate add_action( 'od_trigger_page_cache_invalidation', 'od_trigger_page_cache_invalidation' ) after refactor.

…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.63033% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.25%. Comparing base (da320f3) to head (70002a5).
Report is 34 commits behind head on trunk.

Files with missing lines	Patch %	Lines
plugins/optimization-detective/load.php	0.00%	2 Missing ⚠️
...orage/class-od-rest-url-metrics-store-endpoint.php	98.93%	2 Missing ⚠️
plugins/image-prioritizer/helper.php	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1865      +/-   ##
==========================================
+ Coverage   71.18%   71.25%   +0.06%     
==========================================
  Files          85       85              
  Lines        7000     7006       +6     
==========================================
+ Hits         4983     4992       +9     
+ Misses       2017     2014       -3

Flag	Coverage Δ
multisite	`71.25% <97.63%> (+0.06%)`	⬆️
single	`40.65% <3.79%> (-0.04%)`	⬇️

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

plugins/image-prioritizer/helper.php

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

… 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

… prefix from methods

hbhalodia · 2025-02-13T04:18:20Z

Hi @westonruter, adding this for visibility, I would be going for long vacation starting tomorrow, so if we can land this PR before that, it would be very helpful, else I would not able to work on feedbacks after tomorrow 🙇. Apologies!

westonruter · 2025-02-13T04:28:41Z

@hbhalodia got it! Unfortunately we may not be able to finish this before you go on vacation since my availability is limited now for the next week and a half per #1850 (comment).

hbhalodia · 2025-02-13T04:31:49Z

@hbhalodia got it! Unfortunately we may not be able to finish this before you go on vacation since my availability is limited now for the next week and a half per #1850 (comment).

Hey @westonruter, No issues. I will cover once back. Also, let's meet at WC Asia 🙌, I would be also there.

… fix/issue-1859

westonruter · 2025-02-28T00:31:29Z

@hbhalodia Did we end up crossing paths at WC Asia? I'm sorry to say I can't recall chatting with you, although there were so many people my memory may have been overloaded.

I've resolved the merge conflicts introduced by #1867 by porting the change to rest-api.php in that PR over to this class in 4f9b1c9.

hbhalodia · 2025-03-04T04:18:11Z

@hbhalodia Did we end up crossing paths at WC Asia? I'm sorry to say I can't recall chatting with you, although there were so many people my memory may have been overloaded.

I've resolved the merge conflicts introduced by #1867 by porting the change to rest-api.php in that PR over to this class in 4f9b1c9.

Hi @westonruter, unfortunately not.

Thanks for resolving the conflicts.

Also is there anything pending in this PR other than, #1865 (comment)?

Let me know as I am back from vacation and would work on the same.

Thank You,

westonruter · 2025-03-04T04:30:56Z

@hbhalodia ah, sorry we couldn't connect in Manila.

For this PR, the bigger aspect to get clarity on is #1859 (comment). Based on @felixarntz's feedback there we may not use a static class but rather a singleton class. You can expect a resolution to that discussion tomorrow.

hbhalodia · 2025-03-04T04:33:14Z

#1859 (comment)

Ah, alright, I missed that. No worries, we can update the PR based on the discussion and resolution.

Thank You,

plugins/optimization-detective/storage/class-od-rest-url-metrics-store-endpoint.php

…s and add it separate method

…than constant

westonruter · 2025-03-14T06:35:35Z

Sorry, after merging #1905 there are now some merge conflicts that need to be resolved. I think the new function which was introduced in that PR should made a method of the class introduced here

westonruter · 2025-03-14T17:21:58Z

I'm working on resolving the merge conflicts.

westonruter · 2025-03-14T19:14:20Z

That was more difficult than I was expecting.

…_update_actions()

…s#1574

plugins/optimization-detective/load.php

westonruter

@hbhalodia Thank you for the many iterations on this!

felixarntz

Thanks @hbhalodia and @westonruter!

This looks good, just one point of feedback, but not a blocker for merge, though it would be great to follow up on this separately.

felixarntz · 2025-03-14T22:01:36Z

plugins/optimization-detective/storage/class-od-rest-url-metrics-store-endpoint.php

+	 * @param WP_REST_Request $request Request used to generate the response.
+	 * @return mixed Passed through $result if successful, or otherwise a WP_Error.
+	 */
+	public function decompress_rest_request_body( $result, WP_REST_Server $server, WP_REST_Request $request ) {


Having this in here doesn't seem ideal, since the method is not scoped to this endpoint (even though it technically only acts if the endpoint is hit).

So I think ideally this would be either a standalone function or in a general REST utility class with static methods.

Since it's marked private, not a blocker for merging, but wanted to flag. It could be refactored later.

OK, I'll address in a follow-up I'm working on right now.

westonruter · 2025-03-16T20:05:00Z

I can see this is working now because in my Stream records on my site I see that the posts are updated:

This is happening before updates to URL Metrics:

hbhalodia · 2025-03-17T05:24:16Z

Thanks, @westonruter For updating the PR and apologies that I was not available last friday, hence was not able to pick the further feedbacks.

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

plugins/image-prioritizer/helper.php Outdated Show resolved Hide resolved

westonruter reviewed Feb 11, 2025

View reviewed changes

plugins/optimization-detective/storage/class-od-rest-api.php Outdated Show resolved Hide resolved

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 Outdated 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

Closed

hbhalodia requested a review from westonruter February 13, 2025 03:41

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Feb 13, 2025

westonruter added this to the optimization-detective 1.0.0-beta3 milestone Feb 13, 2025

Merge branch 'trunk' of https://github.com/WordPress/performance into…

4f9b1c9

… fix/issue-1859

Merge branch 'trunk' into fix/issue-1859

06d7462

westonruter reviewed Mar 12, 2025

View reviewed changes

plugins/optimization-detective/storage/class-od-rest-url-metrics-store-endpoint.php Outdated Show resolved Hide resolved

westonruter reviewed Mar 12, 2025

View reviewed changes

plugins/optimization-detective/storage/class-od-rest-url-metrics-store-endpoint.php Outdated Show resolved Hide resolved

hbhalodia added 5 commits March 13, 2025 11:02

Update the trigger page cache validation function to remove from clas…

c668aa2

…s and add it separate method

Merge branch 'trunk' into fix/issue-1859

8c6bfac

Update the class constants to use new names

cb42aff

Add the method for permission callback instead of closure

e4eabe5

Remove access private from class and add to individual methods other …

e3612f8

…than constant

hbhalodia requested a review from westonruter March 13, 2025 06:13

Fix merge conflicts with WordPress#1905

6bb5f84

westonruter added 6 commits March 14, 2025 12:18

Rename od_handle_trigger_page_cache_invalidation() to od_trigger_post…

ea7b687

…_update_actions()

Address minor static analysis complaints

fc2a70f

Improve permission check method name and fix test case name

daa3b66

Remove obsolete class aliases deprecated in October 2024 via WordPres…

d0cf874

…s#1574

Add codeCoverageIgnore

9e02a42

Double-check that od_trigger_post_update_actions() is getting called

2199ebb

westonruter reviewed Mar 14, 2025

View reviewed changes

plugins/optimization-detective/load.php Outdated Show resolved Hide resolved

Move class aliases to deprecated.php

1b96f73

westonruter approved these changes Mar 14, 2025

View reviewed changes

Revert moving class aliases to deprecated.php for sake of PHPStan

70002a5

westonruter added [Type] Bug An existing feature is broken and removed [Type] Enhancement A suggestion for improvement of an existing feature labels Mar 14, 2025

westonruter changed the title ~~Update: REST API logic in Optimization Detective to be encapsulated into a static class~~ Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into class Mar 14, 2025

felixarntz approved these changes Mar 14, 2025

View reviewed changes

westonruter merged commit ab0b99a into WordPress:trunk Mar 14, 2025
26 checks passed

westonruter mentioned this pull request Mar 14, 2025

Disable gzip compression of URL Metric by default #1924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into class #1865

Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into class #1865

hbhalodia commented Feb 11, 2025 •

edited by westonruter

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

hbhalodia commented Feb 12, 2025

westonruter commented Feb 12, 2025

hbhalodia commented Feb 13, 2025

westonruter commented Feb 13, 2025

hbhalodia commented Feb 13, 2025

westonruter commented Feb 28, 2025 •

edited

Loading

hbhalodia commented Mar 4, 2025

westonruter commented Mar 4, 2025

hbhalodia commented Mar 4, 2025

westonruter commented Mar 14, 2025

westonruter commented Mar 14, 2025

westonruter commented Mar 14, 2025

westonruter left a comment

felixarntz left a comment

felixarntz Mar 14, 2025

westonruter Mar 14, 2025

westonruter Mar 14, 2025

westonruter commented Mar 16, 2025

hbhalodia commented Mar 17, 2025

Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into class #1865

Fix triggering post update actions after storing a URL Metric and refactor REST API endpoint logic into class #1865

Conversation

hbhalodia commented Feb 11, 2025 • edited by westonruter Loading

Summary

Relevant technical choices

TODO

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

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

hbhalodia commented Feb 12, 2025

westonruter commented Feb 12, 2025

hbhalodia commented Feb 13, 2025

westonruter commented Feb 13, 2025

hbhalodia commented Feb 13, 2025

westonruter commented Feb 28, 2025 • edited Loading

hbhalodia commented Mar 4, 2025

westonruter commented Mar 4, 2025

hbhalodia commented Mar 4, 2025

westonruter commented Mar 14, 2025

westonruter commented Mar 14, 2025

westonruter commented Mar 14, 2025

westonruter left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Mar 14, 2025

Choose a reason for hiding this comment

westonruter Mar 14, 2025

Choose a reason for hiding this comment

westonruter Mar 14, 2025

Choose a reason for hiding this comment

westonruter commented Mar 16, 2025

hbhalodia commented Mar 17, 2025

hbhalodia commented Feb 11, 2025 •

edited by westonruter

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

westonruter commented Feb 28, 2025 •

edited

Loading