feat: let wp_set_presence() accept an explicit GMT timestamp - #456
feat: let wp_set_presence() accept an explicit GMT timestamp#456theaminulai wants to merge 2 commits into
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
🛝 WordPress PlaygroundsBuilt from 4077810
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
=========================================
Coverage 95.97% 95.97%
Complexity 255 255
=========================================
Files 21 21
Lines 3056 3057 +1
=========================================
+ Hits 2933 2934 +1
Misses 123 123
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
i-am-chitti
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
#450 has been merged, so the scope note is stale. Once, this PR merges the bug is in main.
wp_presence_write_is_redundant() never sees $date_gmt, so it skips the write. A relay backdating a collaborator who has left gets ignored, and they stay in the room. The parameter does nothing for the case it was added for.
Fix looks like one line at includes/functions.php:311, plus a test for that combination:
if ( null === $date_gmt && wp_presence_write_is_redundant( $room, $client_id, $data_json ) ) {
return true;
}| $data_json = wp_json_encode( $state ); | ||
| $now = gmdate( 'Y-m-d H:i:s' ); | ||
| $current = gmdate( 'Y-m-d H:i:s' ); | ||
| $now = null === $date_gmt ? $current : min( $date_gmt, $current ); |
There was a problem hiding this comment.
$date_gmt needs validating as a real date before it gets here. Invalid input string can be passed and processed incorrectly. A format check up front would be better -
if ( null !== $date_gmt && ! DateTimeImmutable::createFromFormat( 'Y-m-d H:i:s', $date_gmt ) ) {
return false;
}There was a problem hiding this comment.
Thanks for the feedback. I agree that $date_gmt should be validated before it is processed.
One question regarding the validation approach: as far as I can see, DateTimeImmutable::createFromFormat() is not currently used in the WordPress codebase. Would you prefer us to use this native PHP API here, or should we follow an existing WordPress date/time validation pattern instead?
I’d prefer to keep this consistent with existing WordPress Core conventions if there is an appropriate utility/pattern available.
There was a problem hiding this comment.
Validate it up front with a preg_match on the full Y-m-d H:i:s plus wp_checkdate(), which is what wp_resolve_post_date() does.
cf8ca86 to
97a7afd
Compare
josephfusco
left a comment
There was a problem hiding this comment.
This is nearly there and just needs the guard bypass so an explicit timestamp is not dropped, plus the format check from the thread above.
Extend `wp_set_presence()` with an optional `$date_gmt` argument so relayed presence updates can keep the original client timestamp instead of always using the relay server clock. Future timestamps are clamped to current GMT time to avoid rows being pinned beyond TTL. README API docs and function tests were updated to cover default behavior, explicit past timestamps, and future clamping.
b905367 to
4077810
Compare
Why
#452
wp_set_presence()hardcodes$now = gmdate( 'Y-m-d H:i:s' )as the row'sdate_gmt, so a caller relaying awareness on behalf of other clients (like sync-storage bridging Gutenberg collaborators) can't preserve their real timestamps. Every relayed entry gets stamped with the relay's own clock, so whoever happens to be polling holds everyone else in the room alive. sync-storage hit exactly this before sync-storage#91: each client rewrote every collaborator's row on every poll, keeping departed collaborators in the room.What
wp_set_presence()gains an optional 5th parameter,$date_gmt('Y-m-d H:i:s', same shapewp_get_presence()already returns), defaulting tonull(current behavior: now).min( $date_gmt, $current ), so a caller can't pin a row past the TTL indefinitely.@since 0.3.0added on the parameter; README's PHP API snippet forwp_set_presence()updated to document it.How
$nowis computed asnull === $date_gmt ? $current : min( $date_gmt, $current )string comparison works because'Y-m-d H:i:s'sorts lexicographically the same as chronologically. Everything downstream (theINSERT ... ON DUPLICATE KEY UPDATE) is unchanged;$nowjust has a new source.Scope note: #452 also lists "the skip guard from #450 does not swallow a write that carries an explicit timestamp" as done-when criteria. #450 (a guard that skips a write when state is unchanged and the row is inside the cutoff) hasn't landed in this repo yet; there's currently no such guard in
wp_set_presence()to interact with. This PR implements #452 standalone; whoever lands #450 will need to make sure its "skip" branch checks fornull !== $date_gmt(or equivalent) before discarding an explicitly-timestamped write.Test instructions
composer phpcs/composer phpstanboth clean.npm test(needswp-env/Docker); new tests intests/test-functions.php:test_set_presence_defaults_date_gmt_to_nowno$date_gmtarg still stamps now.test_set_presence_accepts_an_explicit_past_timestampa past timestamp is stored verbatim.test_set_presence_clamps_a_future_timestamp_to_nowa future timestamp is clamped, not trusted.wp_set_presence( $room, 'client-1', [], 0, gmdate('Y-m-d H:i:s', time() - 300) )viawp evalor a custom mu-plugin, thenwp presence listand confirm the row's age reflects the past timestamp, not "just now."