Include UUID with each URL metric #1489
Conversation
westonruter
added
[Type] Enhancement
|
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. |
westonruter
force-pushed
the
add/od-debugging
branch
from
3dcefe3 to
9e30d5f
Compare
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@westonruter LGTM, just one nit-pick.
| * @throws OD_Data_Validation_Exception When the input is invalid. | ||
| */ | ||
| public function __construct( array $data ) { | ||
| if ( ! array_key_exists( 'uuid', $data ) ) { |
There was a problem hiding this comment.
isset() is faster than array_key_exists(), this has been subject of several core changes. I'd argue anyway that it would be safer to use the following here to ensure this is never empty:
| if ( ! array_key_exists( 'uuid', $data ) ) { | |
| if ( ! isset( $data['uuid'] ) || ! $data['uuid'] ) { |
There was a problem hiding this comment.
I guess we should reduce the scope of that issue to remove use of empty() only.
There was a problem hiding this comment.
Also, the ! $data['uuid'] part is flagged by PHPStan's strict rules:
Only booleans are allowed in a negated boolean, mixed given.
So I'll just commit the ! isset( $data['uuid'] ) part. The JSON Schema will handle validation of uuid. It should be absent for everyone right now, resulting in a new uuid being assigned to all existing URL metrics whenever a new URL metric is captured and added to an od_url_metrics post.
Done in 7a1b892
There was a problem hiding this comment.
+1, I don't think there's any problem with isset() when used with array keys or object properties. In other words, isset( $arr['something'] ) is perfectly fine, but isset( $something ) is not.
It's a good idea to remove empty() usages and, if reasonable to implement, also warn about using isset() when it's not with array keys or object properties.
There was a problem hiding this comment.
Only booleans are allowed in a negated boolean, mixed given.
On another note, this is the only thing that PHPStan complains about that I don't like. I don't like writing '' === $value just as little as I like writing false === $value, and I don't see any benefit of preventing ! on a string. What is the benefit? 🤔
There was a problem hiding this comment.
While I agree it can be annoying at times, I think the explicitness is good for type safety and making sure that you're always working with the data you expect. Otherwise, I had to ask Gemini for advice: https://g.co/gemini/share/270a3e76fcdd
@swissspidy may have more thoughts.
Co-authored-by: felixarntz <[email protected]>
When working with URL metrics, I found it difficult when debugging to not have a stable identifier as part of a given URL metric instance. This adds a
uuidfield to the URL metric to serve as this stable identifier.