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.47% 66.09% +0.62%
==========================================
Files 85 88 +3
Lines 6748 6851 +103
==========================================
+ Hits 4418 4528 +110
+ Misses 2330 2323 -7
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.
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.