Add post ID for the od_url_metrics post to the tag visitor context
#1847
Conversation
westonruter
added
[Type] Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1847 +/- ##
==========================================
+ Coverage 65.97% 66.21% +0.23%
==========================================
Files 88 88
Lines 6895 6943 +48
==========================================
+ Hits 4549 4597 +48
Misses 2346 2346
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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. |
| /** | ||
| * Post ID for the od_url_metrics post which provided the URL Metrics in the collection. | ||
| * | ||
| * May be null if no post has been created yet. | ||
| * | ||
| * @since n.e.x.t | ||
| * @var positive-int|null | ||
| * @readonly | ||
| */ | ||
| public $url_metrics_post_id; |
There was a problem hiding this comment.
I think we should omit the post from the name, as it can lead to confusion. Calling it url_metrics_id makes it clear it's the ID of the URL metrics, whereas if it contains "post", one could think this is referring to the post that lives at that URL.
It's okay to mention "post" when referring to it in combination with the od_url_metrics post type, because then it's obvious. But otherwise I'd refrain from it to avoid confusion.
| /** | |
| * Post ID for the od_url_metrics post which provided the URL Metrics in the collection. | |
| * | |
| * May be null if no post has been created yet. | |
| * | |
| * @since n.e.x.t | |
| * @var positive-int|null | |
| * @readonly | |
| */ | |
| public $url_metrics_post_id; | |
| /** | |
| * ID for the `od_url_metrics` post which provided the URL Metrics in the collection. | |
| * | |
| * May be null if no post has been created yet. | |
| * | |
| * @since n.e.x.t | |
| * @var positive-int|null | |
| * @readonly | |
| */ | |
| public $url_metrics_id; |
Aside: Should it be url_metrics_id or url_metric_id?
There was a problem hiding this comment.
It should definitely not be url_metric_id since if you're talking about a single URL Metric then you would be using its UUID. An od_url_metrics post always contains an array of URL Metrics, so this should be plural.
There was a problem hiding this comment.
I think we should omit the
postfrom the name, as it can lead to confusion. Calling iturl_metrics_idmakes it clear it's the ID of the URL metrics, whereas if it contains "post", one could think this is referring to the post that lives at that URL.
I'm not sure. The od_url_metric_stored action includes a context object that references post_id. In that case the url_metrics_ prefix is not present, but that is more clear from context. For a tag visitor context, adding the prefix is needed because it wouldn't be clear from the tag visitor what the ID is referring to. But in both cases I thought it made sense to mention post_id. Probably the former should be updated to have the same as the latter for consistently. But back to the main point about whether to mention "post". I think it is helpful to mention "post" so that the developer understands what kind of ID is being referred to and what you can do with it. Namely, you can use it to access the post meta. If it's just a bare url_metrics_id then the developer would need to look deeper to figure out what it is exactly.
There was a problem hiding this comment.
I think that it's a very common pattern in plugins to refer to this data as just "ID" (instead of "post ID") in combination with the plugin's specific purpose, so I'm not concerned that people wouldn't understand it's a post ID. Consider for example WooCommerce referring to a "product ID" - it's not saying "product post ID". It's so common that I think plugin developers would actually assume it's a post ID anyway. And by omitting the word "post" it's clear that it's the ID of the URL metrics themselves, not the post ID of the content in that URL.
Of course in theory it wouldn't hurt to include the word "post", but since URL metrics can also refer to a URL of a specific post, in this case that could be confusing. So IMO the downsides of using "URL metrics post ID" would be greater than the benefits.
plugins/optimization-detective/tests/class-optimization-detective-test-helpers.php
Outdated
Show resolved
Hide resolved
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: felixarntz <[email protected]>
westonruter
force-pushed
the
add/context-post-id
branch
from
9a091e8 to
145f89e
Compare
There was a problem hiding this comment.
@westonruter LGTM!
One unrelated question, that we can discuss here and either discard or handle elsewhere.
| * @property-read OD_URL_Metric_Group_Collection $url_metric_group_collection URL Metric group collection. | ||
| * @property-read OD_Link_Collection $link_collection Link collection. | ||
| * @property-read positive-int|null $url_metrics_id ID for the od_url_metrics post which provided the URL Metrics in the collection. | ||
| * @property-read OD_URL_Metric_Group_Collection $url_metrics_group_collection Deprecated alias for the $url_metric_group_collection property. |
There was a problem hiding this comment.
Just seeing this now and I know it's not related to the change here, but why is url_metric_group better than url_metrics_group? In your previous comment, you mentioned that we should definitely use $url_metrics_id instead of $url_metric_id.
I get that it's fuzzy when referring to a group of that, but for better understanding, I would still find it more intuitive to use url_metrics_group, since each group also covers multiple URL metrics, and it's aligned with the url_metrics_id terminology.
There was a problem hiding this comment.
It was originally OD_URL_Metrics_Group and OD_URL_Metrics_Group_Collection. This was undone in 942b397 as part of #1574.
In that PR description I had noted:
Aside: Perhaps
OD_URL_Metrics_Group_Collectionshould be renamed toOD_URL_Metric_Group_Collectionas well. The "metrics" plural here is redundant with the group in the name.
And you concurred:
+1 for that. Since it seems nobody other than us is adopting the plugin yet, this is probably fine to just rename. Once we document this further and encourage external usage, we'll need to be more careful about this kind of change.
For $url_metrics_id it should specifically be plural because it's referring to an instance of the od_url_metrics post type. If we went with plural then it would seem for consistency that it would have to be OD_URL_Metrics_Groups_Collection (with Groups also being plural). But the plural is not needed because the "group" and "collection" here both indicate that it is a plurality.
| * | ||
| * @property-read WP_REST_Request<array<string, mixed>> $request Request. | ||
| * @property-read positive-int $url_metrics_id ID for the od_url_metrics post. | ||
| * @property-read OD_URL_Metric_Group_Collection $url_metric_group_collection URL Metric group collection. |
This adds a
$url_metrics_post_idproperty to theOD_Tag_Visitor_Contextclass for the sake of extensions which may be attaching postmeta to theod_url_metricspost during theod_url_metric_storedaction. For example, the Optimization Detective Content Visibility plugin does this. TheOD_URL_Metric_Store_Request_Contextinstance passed to theod_url_metric_storedaction includes such apost_id, but the context passed to the tag visitors did not. This necessitated manually calculating the slug to obtain the post ID. By making the post ID available on theOD_Tag_Visitor_Contextthen this is greatly simplified, as seen in westonruter/od-content-visibility#1.This PR also introduces a
OD_URL_Metric_Group::get_collection()method to provide a way to access the private$collectionvariable. This parallels howOD_URL_Metric::get_group()provides access to the private$groupvariable a level lower.This PR facilitates the ability for extensions to persist data outside of the URL Metric itself and in the postmeta of the
od_url_metricspost type. This is required when data being collected in URL Metrics is not always available. For example, with Embed Optimizer, not every collected URL Metric will include the resized height for an embed since a user may not scroll down to see it in the viewport to cause it to be lazily-loaded. And in the case of Content Visibility, once an element hascontent-visibilityapplied, it is not longer able to be measured during detection unless, again, the user happens to scroll down to view the element that has CV applied. So we need to capture the initial height of the element in addition to the height of the element when users happen to scroll to it. Persisting this information in postmeta outside of URL Metrics is a way to do this. We should explore further if the core URL Metric construct should have a way to persist some data as new URL Metrics push out old ones, but in the meantime using postmeta as an ad hoc storage area is a way for us to experiment.