Remove transitional XPath #1820
Conversation
westonruter
added
[Type] Enhancement
This comment was marked as resolved.
This comment was marked as resolved.
westonruter
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1820 +/- ##
==========================================
+ Coverage 65.85% 65.98% +0.12%
==========================================
Files 88 88
Lines 6851 6826 -25
==========================================
- Hits 4512 4504 -8
+ Misses 2339 2322 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@westonruter This looks good to me, just one small note.
I wonder, do we already want to merge this for 1.0.0-beta2, or wait until the release after? The tentative 1.0.0-beta2 release is only 3 weeks away, so is this enough time to give users to have their data migrated?
| public function get( string $key ) { | ||
| if ( 'xpath' === $key ) { | ||
| return $this->get_xpath(); | ||
| } | ||
| return $this->data[ $key ] ?? null; | ||
| } |
There was a problem hiding this comment.
Just wondering whether we should actually remove this. Wouldn't it be a good idea to have the data always be in sync, no matter whether you use get_xpath() or get( 'xpath' )?
There was a problem hiding this comment.
I think we should remove it because the reason for it being added in the first place is no longer present. If we retain this then it would seem like we should to the same for all of the other getter methods as well, like is_lcp, get_intersection_ratio, and so on. But they're all just different ways of accessing the $this->data array.
There was a problem hiding this comment.
Yes, that's what I was getting at.
If we have dedicated methods but also this general getter, I think it's a slightly safer implementation to fall through to the relevant methods where they exist. We could have a method_exists( $this, "get_{$key}" ) check for example. That way it's ensured the return values will always be aligned, no matter what approach the caller chooses.
There was a problem hiding this comment.
Yeah, maybe. Although that adds overhead and complexity for what otherwise is a simple lookup to grab data out of the data array. So they will all be in sync. If we find we need to re-introduce mutations generally on properties (beyond the temporary one-off for xpath) then I think this would make sense.
… remove/transitional-xpath
@felixarntz The TTL for a URL Metric is 1 day. So for a site that gets consistent traffic the old URL Metrics should get refreshed after a day. However, for lower-traffic sites or sites that get infrequent visitors of different form factors, a stale URL Metric could stick around much longer. I suppose we could hold off on merging this for another cycle to give extra time. |
westonruter
modified the milestones:
optimization-detective 1.0.0-beta2,
optimization-detective 1.0.0-beta3
|
SG, thanks! |
Summary
This removes the transitional XPath logic from #1797. This PR should only be merged after #1797 has been live for a few weeks. The logic was put in place so that the new XPath format wouldn't immediately invalidate all existing optimizations applied with old URL Metrics.