RTC: Ensure a single canonical update log for collaborative edits.#11660
RTC: Ensure a single canonical update log for collaborative edits.#11660dmsnell wants to merge 1 commit intoWordPress:trunkfrom
Conversation
Trac ticket: Core-65138 Backport of WordPress/gutenberg#77675 A race condition on opening an editor session allows for Core to create duplicate post meta for sync storage. This creates two copies of the document which the editors operate on independently, leading to a mismatch between the sessions. In this patch, when such a duplicate storage row is detected, a canonical version of the post meta is chosen (the one with the lowest id viz. the oldest one) and the duplicate is merged into it. This should ensure that all edit sessions for a given post use the same synchronized backing store. Co-authored-by: Dennis Snell <dmsnell@git.wordpress.org>
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
apermo
left a comment
apermo
left a comment
There was a problem hiding this comment.
LGTM, I would consider stricter conditions, and checking for int over not falsy.
| array( '%d', '%s', '%s' ) | ||
| ); | ||
|
|
||
| if ( $result ) { |
There was a problem hiding this comment.
While technically fine, how would you feel about a strict comparison? insert() return int|false.
What about
| if ( $result ) { | |
| if ( is_int( $result ) ) { |
| if ( false !== $result ) { | ||
| self::$storage_post_ids[ $room_hash ] = $this->merge_duplicate_storage_posts( $room_hash, $post_id ); | ||
| } |
There was a problem hiding this comment.
Here you actually did what i suggested above, but why the negative check instead of the positive test?
| if ( $result ) { | ||
| self::$storage_post_ids[ $room_hash ] = $this->merge_duplicate_storage_posts( $room_hash, $post_id ); | ||
| } |
| $canonical_post_id = $this->promote_storage_post_to_canonical_slug( $room_hash, $inserted_post_id ); | ||
| } | ||
|
|
||
| if ( null === $canonical_post_id ) { |
There was a problem hiding this comment.
I would still consider checking for intval() instead for null, but in these cases the case is not as strong as above.
|
thanks @apermo — for now this is still a draft and mainly here to backport an upstream change in Gutenberg. I didn’t want to change the patch yet; just preserve what’s on the other side so tests can run and all. I think you have good feedback; the original code was created by LLMs and that’s why it seems confused and split-brained. |
2 participants
Trac ticket: Core-65138
Backport of WordPress/gutenberg#77675
A race condition on opening an editor session allows for Core to create duplicate post meta for sync storage. This creates two copies of the document which the editors operate on independently, leading to a mismatch between the sessions.
In this patch, when such a duplicate storage row is detected, a canonical version of the post meta is chosen (the one with the lowest id viz. the oldest one) and the duplicate is merged into it.
This should ensure that all edit sessions for a given post use the same synchronized backing store.
Trac ticket:
Use of AI Tools
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.