Size Change: +16 B (0%)

Total Size: 8.21 MB

_{compressed-size-action}

The change reminded me of our previous conversation - #49409 (comment). Do you think that's still doable? It should also resolve the related issue #15993.

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: alecgeatches <alecgeatches@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

The change reminded me of our previous conversation

Haha you remember everything 🙂 At the end of the comment I wrote:

In practice it's not so simple and there's some amount of complexity to untangle, but in principle it should work.

I don't remember what I meant by that, but today there is at least one caveat: the settings we send to the data store is not the raw object that was passed to initializeEditor, but it's enhanced inside EditorProvider with styles and callbacks like onNavigateToEntityRecord. So it's not so easy to move the updateEditorSettings call from the React component (EditorProvider) to the initializeEditor function.

I think it’s doable to untangle. Styles should already be available on server and passed as raw settings. Navigation callback will need more tinkering.

Mostly want to close old issue with a fix when possible or with comment why change can’t be made.

Anyway, it’s not a blocker here, just something I remembered.

Unfortunately, the tests seem to be failing on this branch as well.

Unfortunately, the tests seem to be failing on this branch as well.

Yes, that's expected, because I removed the stillHasRemoteAutosave check. Its purpose is to check whether we managed to set up the initial condition: there are both remote and local autosaves. Sometimes setting up the remote autosave fails, because of the modified race condition. Then the test silently gave up, acted as noop and succeeded. Without the check, it now fails.

If the test succeeds at setting up both autosaves, it checks the notices. And there was the updateEditorSettings bug that caused the notices to be displayed incorrectly.

If we don't manage to fix the setup problem, I'll add the check back, although it's a bad thing to have it there.

The meta-save request probably shouldn't update any timestamps at all.

@jsnajdr Your change in 5dfbf96 looks fine to me! The purpose of the persistCRDTDoc call is to fix the server-persisted CRDT doc when we load a post and see that the content has changed, e.g. a WP-CLI command has added content to the post or a taxonomy/meta value has changed on server but not in the CRDT doc. We want both to match, so when a user loads in and determines there's a mismatch, the saveEntityRecord call fires with the intended side-effect of just updating CRDT meta. I don't think there would be any problems patching the timestamp to keep it stable. Another more invasive solution could be to detect CRDT meta updates server-side and then not update the post_modified flag to avoid overwriting an actual change, but that might be too case-specific.

@chubes4 only recently fixed this pathway to save the CRDT doc alone instead of the whole post which was even more disruptive.

the saveEntityRecord call fires with the intended side-effect of just updating CRDT meta. I don't think there would be any problems patching the timestamp to keep it stable.

Patching the modified timestamp works for the moment, but I think an ideal solution will be a separate endpoint to save the CRTD document. Maybe in the wp-sync namespace, something like

POST /wp-json/wp-sync/save { "room": "postType/post:5", "doc": ... }

The reason is that saveEntityRecord, the REST endpoint, and the server-side PHP wp_update_post have the semantics that the user really saved a new version of the post, and modified it in some meaningful way. But saving the CRDT document is just a RTC bookkeeping, the user is not really saving anything. It's triggered even by purely read-only actions.

In wp-data, saveEntityRecord makes the UI think that the post is really saving, and it disables the "Publish" button for a moment, for example.

wp_update_post calls many popular filters and actions that plugins hook into, and the assumption is that we're saving a post.

With a separate endpoint we would avoid a lot of confusion. What do you think?

@chubes4 only recently fixed this pathway to save the CRDT doc alone instead of the whole post

It seems there is a case where the payload is too small and doesn't even include the CRDT doc. Sometimes when creating a new post I see a request to update a category, where the payload includes only the id:

POST /wp/v2/categories/1 { id: 1 }

This is triggered by a getEntityRecord( 'taxonomy', 'category' ) read.

Taxonomies have a syncConfig, but it's the defaultSyncConfig one that doesn't have a getPersistedCRDTDoc, and also taxonomies don't have the __unstablePrePersist hook that would add the CRDT document to the empty payload.

What is the solution for this? Should categories have a persisted CRDT doc at all?

Setting modified explicitly doesn't work, the REST API considers the modified field as read-only and that attempt to set it is ignored.

Patching the modified timestamp works for the moment, but I think an ideal solution will be a separate endpoint to save the CRTD document. Maybe in the wp-sync namespace, something like

POST /wp-json/wp-sync/save { "room": "postType/post:5", "doc": ... }

The reason is that saveEntityRecord, the REST endpoint, and the server-side PHP wp_update_post have the semantics that the user really saved a new version of the post, and modified it in some meaningful way. But saving the CRDT document is just a RTC bookkeeping, the user is not really saving anything. It's triggered even by purely read-only actions.

In wp-data, saveEntityRecord makes the UI think that the post is really saving, and it disables the "Publish" button for a moment, for example.

@jsnajdr This sounds right to me, especially considering all of the side effects of saveEntityRecord() you mentioned. I opened a new issue in #78867 to track this. Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

Yes, this is causing at least two flaky e2e tests, this one, and also the Cancel button failures in #77721. The mechanism is similar in each case: the POST request with CRDT meta is an unexpected side effect that is observable in one way or another: changes the modified date, signals that the post is already saving, etc. I think there will be more flaky tests like this, it's just that so far I know about two.

If we don't fix this, I think people will soon discover that their modified dates are off, it's a very visible bug. Maybe it's already reported somewhere.

I also wanted to clarify the empty { id } payload when saving a category. Should categories (and other taxonomies) also have a CRDT document? They don't have a content attribute to edit.

Does this seem necessary to fix the problems you're seeing in this PR, i.e. should we get this done pretty quickly?

Yes, this is causing at least two flaky e2e tests, this one, and also the Cancel button failures in #77721. The mechanism is similar in each case: the POST request with CRDT meta is an unexpected side effect that is observable in one way or another: changes the modified date, signals that the post is already saving, etc. I think there will be more flaky tests like this, it's just that so far I know about two.

If we don't fix this, I think people will soon discover that their modified dates are off, it's a very visible bug. Maybe it's already reported somewhere.

Okay, thanks for the clarification. I'll look into what's involved with changing our persistence endpoint.

I also wanted to clarify the empty { id } payload when saving a category. Should categories (and other taxonomies) also have a CRDT document? They don't have a content attribute to edit.

Categories/taxonomies are synced over Yjs, but they're never persisted as stored CRDT docs, so I think you've identified a bug. We're supposed to bail when an entity doesn't support meta, but it appears categories do have a meta property and are incorrectly getting the "persist CRDT" side-effect with no doc to save for no good reason. That should also be addressed in this work.

@jsnajdr I started on a PR in #78891. There are a couple of snags with other side effects we relied on from saving, but overall it seems to be working well. I'll ping you on that PR when it's ready for review.

I started on a PR in #78891. There are a couple of snags with other side effects we relied on from saving, but overall it seems to be working well.

Thanks, it works very well in my testing, and I posted a few review comments already.