De-duplicate logic between REST API and URL Metrics post type #1867
Conversation
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
ShyamGadde
added
[Type] Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1867 +/- ##
==========================================
- Coverage 66.64% 66.63% -0.01%
==========================================
Files 88 88
Lines 7015 7011 -4
==========================================
- Hits 4675 4672 -3
+ Misses 2340 2339 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Shyamsundar Gadde <[email protected]>
plugins/optimization-detective/storage/class-od-url-metrics-post-type.php
Outdated
Show resolved
Hide resolved
…etric Signed-off-by: Shyamsundar Gadde <[email protected]>
Signed-off-by: Shyamsundar Gadde <[email protected]>
ShyamGadde
requested review from
felixarntz and
adamsilverstein
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| public static function store_url_metric( string $slug, OD_URL_Metric $new_url_metric ) { | ||
| $post_data = array( | ||
| public static function update_post( string $slug, OD_URL_Metric_Group_Collection $url_metric_group_collection ) { | ||
| $url_metrics = $url_metric_group_collection->get_flattened_url_metrics(); |
There was a problem hiding this comment.
What if there are no URL Metrics in the collection? Shouldn't this method short-circuit with a WP_Error?
Alternatively, should we allow updating a post with zero URL Metrics (i.e. to empty it out)? We don't have a use case for that in the plugin and this method is not intended to be public, so probably not.
Ensuring there is at least one URL Metric will prevent a possible error below when accessing $url_metrics[0].
| $post_data = array( | ||
| 'post_title' => $new_url_metric->get_url(), | ||
| ); | ||
|
|
||
| $post = OD_URL_Metrics_Post_Type::get_post( $slug ); | ||
| if ( $post instanceof WP_Post ) { | ||
| $post_data['ID'] = $post->ID; | ||
| $post_data['post_name'] = $post->post_name; | ||
| $url_metrics = OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ); | ||
| } else { | ||
| $post_data['post_name'] = $slug; | ||
| $url_metrics = array(); | ||
| } | ||
|
|
||
| $etag = $new_url_metric->get_etag(); | ||
|
|
||
| $group_collection = new OD_URL_Metric_Group_Collection( | ||
| $url_metrics, | ||
| $etag, | ||
| od_get_breakpoint_max_widths(), | ||
| od_get_url_metrics_breakpoint_sample_size(), | ||
| od_get_url_metric_freshness_ttl() | ||
| ); | ||
|
|
||
| try { | ||
| $group = $group_collection->get_group_for_viewport_width( $new_url_metric->get_viewport_width() ); | ||
| $group->add_url_metric( $new_url_metric ); | ||
| } catch ( InvalidArgumentException $e ) { | ||
| return new WP_Error( 'invalid_url_metric', $e->getMessage() ); | ||
| } | ||
|
|
||
| $post_data['post_content'] = wp_json_encode( | ||
| $group_collection->get_flattened_url_metrics(), | ||
| JSON_UNESCAPED_SLASHES // No need for escaping slashes since this JSON is not embedded in HTML. | ||
| ); | ||
| if ( ! is_string( $post_data['post_content'] ) ) { | ||
| return new WP_Error( 'json_encode_error', json_last_error_msg() ); | ||
| } | ||
|
|
||
| $has_kses = false !== has_filter( 'content_save_pre', 'wp_filter_post_kses' ); | ||
| if ( $has_kses ) { | ||
| // Prevent KSES from corrupting JSON in post_content. | ||
| kses_remove_filters(); | ||
| } | ||
|
|
||
| $post_data['post_type'] = OD_URL_Metrics_Post_Type::SLUG; | ||
| $post_data['post_status'] = 'publish'; | ||
| $slashed_post_data = wp_slash( $post_data ); | ||
| if ( isset( $post_data['ID'] ) ) { | ||
| $result = wp_update_post( $slashed_post_data, true ); | ||
| } else { | ||
| $result = wp_insert_post( $slashed_post_data, true ); | ||
| } | ||
|
|
||
| if ( $has_kses ) { | ||
| kses_init_filters(); | ||
| } | ||
|
|
||
| return $result; |
There was a problem hiding this comment.
Couldn't this be simplified now to just construct the OD_URL_Metric_Group_Collection, add the URL Metric to it, and then pass it to OD_URL_Metrics_Post_Type::update_post()?
Summary
Fixes #1858
Relevant technical choices
OD_URL_Metrics_Post_Type::update_post( string $slug, string $post_title, OD_URL_Metric_Group_Collection $url_metric_group_collection )to replacestore_url_metric(), eliminating redundant logic.update_post()instead ofstore_url_metric(), ensuring the existingOD_URL_Metric_Group_Collectionis passed directly.store_url_metric()toOptimization_Detective_Test_Helpersfor test compatibility.