Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid saving metaboxes before a preview #14877

Merged
merged 3 commits into from Apr 9, 2019

Conversation

@talldan
Copy link
Contributor

commented Apr 9, 2019

Description

Fixes #12617

This PR proposes to remove the saving of metaboxes when previewing a post, which resolves the issue described at #12617. A number of other solutions have been proposed for the bug, but generally all have a shortcoming of some kind—it seems like we're stuck with a process of choosing the best from a bad bunch.

In discussion with @pento, @draganescu and @azaozz it was mentioned that the classic editor didn't (doesn't) save metaboxes during a preview, so while this PR is removing a feature from Gutenberg, it is also reintroducing behaviour that users are used to, and at the same time resolving a bug affecting many.

How has this been tested?

An end-to-end test has been added to catch regressions.

  1. Create a new post.
  2. Enable 'custom fields' from the options menu.
  3. Add a custom field.
  4. Add some post content.
  5. Save the post.
  6. Add more post content.
  7. Click preview.
  8. Observe that changes added in step 4 are not present in the preview

Expected:
The preview displays the content added in step 4.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan self-assigned this Apr 9, 2019

@talldan talldan added this to the 5.5 (Gutenberg) milestone Apr 9, 2019

@talldan talldan changed the title Remove metabox save on preview Avoid saving metaboxes during preview Apr 9, 2019

@talldan talldan changed the title Avoid saving metaboxes during preview Avoid saving metaboxes before a preview Apr 9, 2019

@youknowriad
Copy link
Contributor

left a comment

So, it seems that the classic editor does save the meta-boxes on preview to ensure they are shown properly (as these don't support revisions).

Gutenberg at the moment tries to mimic this behavior by triggering the save (previous to this PR) but it seems like the two saves are causing previews to break (not show the last changes) regardless.

I feel like reverting the metabox save is preview is a good tradeoff that:

  • Ensures the modified content that is not in a metabox is shown properly on preview
  • Don't publish changes that are not ready yet

The downside is that some changes (the ones coming from the metaboxes) will not be shown in the preview if unsaved.

I think it's the best trade-off we can have for now.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Thanks for the quick review and 👍, @youknowriad. Will create an issue where we can track reintroducing this in a way where the preview still works.

@talldan talldan merged commit b89869c into master Apr 9, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@talldan talldan deleted the remove/metabox-save-on-preview branch Apr 9, 2019

@davidAIS

This comment has been minimized.

Copy link

commented Apr 9, 2019

As the person that raised #12617 - can I ask - has this been tested with edits to existing pages created prior to WP5 and the appearance of Gutenberg which seemed to more regularly trigger this issue compared to content created since then using the steps on your end to end test?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@davidAIS The issue happens when you have metaboxes and is not related to the content itself. If you don't have meta-boxes in your editor, no issue will happen.

The e2e test added here covers this.

@earnjam

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@davidAIS The reason you saw it more with pre WP5.0 content is because it would only occur on published pages. Drafts would do a regular save when clicking preview, published posts would do an autosave. The issue was that the content changes were saved in an autosave, which was then getting deleted when the meta boxes would save.

There were a few people who suggested previews were not working even when they did not have meta boxes, so we have #13232 to try to validate that, but I haven't been able to recreate that issue. I think we'll know more there with this PR merged, as that will eliminate the meta box problem.

@youknowriad I think that e2e test should be testing this against published content. Was the test run with and without this change?

@azaozz

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Will create an issue where we can track reintroducing this in a way where the preview still works.

As discussed yesterday the underlying problem here is that two separate requests are fired at about the same time: one to do the autosave necessary for the preview, the other to save metaboxes data. This only happens when previewing changes to published posts that have metaboxes.

Ideally all data that needs to be saved would be combined in one request. If that's not possible, one request has to be "dependent" on the other, i.e. we will need to wait for metabox save to finish before firing the autosave (that means it would be slower, two trips to the server, etc.).

However, another problem is that previewing a published post should not save/update the content for the post, including the post meta. In that case saving of metabox data for published posts should only happen when the post is updated, not on autosaves. This will change when the (long-standing) enhancement for saving revisions of post meta gets done, see https://core.trac.wordpress.org/ticket/20564. Then autosaves for published posts will be able to include post meta data that will be saved in a revision.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@earnjam Well spotted on the e2e test, I'll work on a PR to make sure it has a publish step before previewing.

edit: PR here - #14894

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Here's the issue to track reintroducing the metabox save when previewing:
#14900

@Armarsh

This comment has been minimized.

Copy link

commented Apr 10, 2019

There were a few people who suggested previews were not working even when they did not have meta boxes, so we have #13232 to try to validate that, but I haven't been able to recreate that issue.

@eanjam -- Actually, I'm the person that opened #13232 and made that suggestion, but as I have worked on things since I have discovered that at least one of the sites that was giving me problems had hidden custom fields-- put in by a plugin that had since been removed. So I now think that the metabox problem is the cause. Part of the problem is that Gutenberg defaults to hiding custom fields on the edit screen, so it was a while before I figured out the extra steps I needed to take in order to display them.

I did test my sites with a function to disable the metabox autosave, and it fully resolved the preview issue on the sites that I tested with. So I'd anticipate that this should resolve the problem. (Although of course that will be inconvenient for sites that do rely on metabox information -- but I actually think that premature saving of custom fields ahead of publication can be causing its own set of problems, so not such a good idea in any event).

aduth added a commit that referenced this pull request Apr 15, 2019

Avoid saving metaboxes before a preview (#14877)
* Remove logic that causes a metabox save before a preview

* Add e2e test for preview with custom fields active.

* Improve test robustness

aduth added a commit that referenced this pull request Apr 16, 2019

Avoid saving metaboxes before a preview (#14877)
* Remove logic that causes a metabox save before a preview

* Add e2e test for preview with custom fields active.

* Improve test robustness

aduth added a commit that referenced this pull request Apr 16, 2019

Avoid saving metaboxes before a preview (#14877)
* Remove logic that causes a metabox save before a preview

* Add e2e test for preview with custom fields active.

* Improve test robustness

This was referenced Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.