WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 8 months ago

Last modified 5 months ago

#45113 closed task (blessed) (fixed)

Integrate compatibility related functions for the new editor

Reported by: desrosj Owned by: pento
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit fixed-5.0
Focuses: Cc:

Description

In gutenberg/lib/compat.php, there are several functions that ensure compatibility with the new editor. These should be merged into WordPress Core.

Attachments (2)

45113.heartbeatnonce.diff (17.4 KB) - added by dd32 9 months ago.
45113.heartbeatnonce.2.diff (10.1 KB) - added by dd32 9 months ago.
patch refresh

Download all attachments as: .zip

Change History (30)

#1 @pento
10 months ago

  • Owner set to pento
  • Status changed from new to assigned

#2 @pento
10 months ago

In 43758:

Classic Editor: Disable the wpautop TinyMCE plugin on block posts.

As the block editor adds its own <p> tags, disabling the wpautop stops the classic editor from removing them.

See #45113.

#3 @pento
10 months ago

  • Keywords needs-patch needs-unit-tests removed

Done/not needed:

  • _gutenberg_utf8_split() doesn't need to be ported, as it's only used by the spec parser, not the default parser.
  • gutenberg_disable_editor_settings_wpautop() was ported in [43758].
  • gutenberg_wpautop() was ported in [43752].
  • gutenberg_warn_classic_about_cloudflare() won't be ported.

I haven't ported gutenberg_add_rest_nonce_to_heartbeat_response_headers() yet, the current naming doesn't fit in with the structure of the other nonces in wp_refresh_post_nonces(), but we'll need to change the name in @wordpress/api-fetch to match.

I haven't ported the warning that shows up when trying to open a block post in the classic editor, I'm leaning towards putting that in the Classic Editor plugin. @karmatosed, do you have thoughts on this?

#4 @karmatosed
10 months ago

I haven't ported the warning that shows up when trying to open a block post in the classic editor, I'm leaning towards putting that in the Classic Editor plugin. @karmatosed, do you have thoughts on this?

If I understand correctly you are suggesting putting the warning in the classic editor? If that's the case then it makes sense to me assuming the plugin would have to be active to get that warning?

#5 @pento
10 months ago

Yah, you'd need to have the plugin active to get the warning. Not having the plugin would mean that no warning is shown when a block post is opened in the classic editor, which will likely cause unexpected behaviour.

#6 @pento
10 months ago

@adamsilverstein: Do you want the rest-api nonce to be put into core as is? I saw you were reviewing a bug in the nonce refreshing. I was originally unsure about it being in the top level of $response, but I'm not really fussed.

#7 @adamsilverstein
10 months ago

@pento yes, i think it makes sense to have this in core. I'll try to get back to the gutenberg ticket soon.

#8 @pento
9 months ago

@dd32: Since you wonderfully managed GB11347, would you mind having a look at getting the nonce refreshes included in core?

This ticket was mentioned in Slack in #core-editor by ocean90. View the logs.


9 months ago

#10 @ocean90
9 months ago

  • Keywords needs-patch added

#11 @dd32
9 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Replying to pento:

I haven't ported gutenberg_add_rest_nonce_to_heartbeat_response_headers() yet, the current naming doesn't fit in with the structure of the other nonces in wp_refresh_post_nonces(), but we'll need to change the name in @wordpress/api-fetch to match.

While working on this, I realised that wp_refresh_post_nonces() is no longer being used, as that code path is classic-editor only. As a result, the heartbeat nonce hasn't been being refreshed either (As that's been handled by wp-admin/includes/post.js in the past).

45113.heartbeatnonce.diff takes a slightly different approach than what was previously done in core:

  • The Heartbeat nonce updating is now handled by heartbeat.js itself, rather than relying upon the loaded editor
  • heartbeat.js also updates the wp-api package nonce global if loaded
  • heartbeat.js now includes a reliance upon wp-hooks to do both jQuery.trigger() and wp.hooks.doAction() on events, so api-fetch can hook in directly to retrieve the nonce.

I've included a line that's intended to be temporary, returning rest-nonce in addition to rest_nonce for compatibility with the current api-fetch while acknowledging we probably should rename that to be rest_nonce.

Although untested at time of this comment, these changes should work transparently with the current Gutenberg stable/master.


After I wrote and tested the above, I realised that the nonces mentioned in wp_refresh_post_nonces() are included within the HTML for the Block editor. However, I don't think they're actually used, and are just a vestige so maybe not needed?

  • getpermalinknonce - Not included
  • samplepermalinknonce - Included in HTML, seems unused.
  • closedpostboxesnonce - Included in HTML, seems unused.
  • _ajax_linking_nonce - Included in HTML, seems like the internal linking dialogue uses wp-json instead of utilising this with admin-ajax.php though.
  • _wpnonce - Included in HTML (actually multiple fields named that, with different source nonces) and seems like it's not used either as all actions go through the rest api.

With the above in mind, the approach taken in 45113.heartbeatnonce.diff might be OK, or might need stepping back and having the existing JS for updating nonces extracted from post.js and moved into heartbeat.js or it's own file.

#12 follow-up: @adamsilverstein
9 months ago

The changes in 45113.heartbeatnonce.diff look good, I can give that some testing later in my local with nonce expiration set very low to verify refresh is actually working.

I agree we can likely remove the unused nonces included in the html as long as we are sure they aren't used. I did a little research to see how/if we use these nonces:

I think we need these nonces (and keep them refreshed) for actions taken within the classic editor block:

_ajax_linking_nonce is for ajax requests made when using the link selection modal which is still available.

_wpnonce is used in wp.media, think we need to retain that, I'll try to confirm by looking for the nonce in requests

These I think can be dropped for GB:
samplepermalinknonce is used in the classic editor for slug operations, we probably don't need it for Gutenberg.

closedpostboxesnonce is used when persisting metabox open/closed states - we probably don't need for GB

I couldn't find any use of getpermalinknonce

Last edited 9 months ago by adamsilverstein (previous) (diff)

#13 @adamsilverstein
9 months ago

@dd32 is this against the 5.0 branch or trunk? having trouble applying cleanly.

@dd32
9 months ago

patch refresh

#14 @dd32
9 months ago

@adamsilverstein I've uploaded a refreshed patch, a few conflicts came in with some recent commits.
The patch is generated from https://core.svn.wordpress.org/branches/5.0 but should apply cleanly after the refresh.

#15 in reply to: ↑ 12 @dd32
9 months ago

Replying to adamsilverstein:

The changes in 45113.heartbeatnonce.diff look good, I can give that some testing later in my local with nonce expiration set very low to verify refresh is actually working.

I agree we can likely remove the unused nonces included in the html as long as we are sure they aren't used. I did a little research to see how/if we use these nonces:

I think we need these nonces (and keep them refreshed) for actions taken within the classic editor block:

_ajax_linking_nonce is for ajax requests made when using the link selection modal which is still available.

Yep, confirmed that's still in use. When links are inserted with the classic editor block they go via admin-ajax.php, rather than via wp-json in the rest of the blocks.

_wpnonce is used in wp.media, think we need to retain that, I'll try to confirm by looking for the nonce in requests

The _wpnonce in question was a edit_post_{$id} nonce, so I don't think it was being refreshed for the purposes of media, but I'll take another look into that.

Based on the above, I'm going to go back to the drawing board a little, and restore the existing nonce refreshing, but I'm not sure if i'll get to that today.

#16 @adamsilverstein
9 months ago

Confirming that in my testing using 5.0 with 45113.heartbeatnonce.2.diff nonces are refreshed correctly. I set nonce expiration to 25 seconds and heartbeat to 15 seconds and observed that saves continued to work and used each new nonce returned via the heartbeat.

re _wpnonce I only saw that when scanning the codebase, i'll do a little more digging to see if I can determine where/how it is used and if we still need it.

#17 @adamsilverstein
9 months ago

Note: to test that nonce refreshes work i’m setting heartbeat frequency https://gist.github.com/adamsilverstein/4bfc76454656046b24f18b292da895f7 and nonce expiration https://gist.github.com/adamsilverstein/cf39db696d242e6c1f2f7695442d7fc0 to testing intervals…

eg:

i set nonce expiration to 25 and heartbeat to 15. with every other heartbeat i can check the wp-ajax response in console and verify a new wp-nonce (and wp_nonce :() are returned
then in GB i click save draft or update and observe the reset request, verifying that the nonce included in the header is the same one that was returned from the heartbeat

then, i tried with the nonce expiration set to 25 and heartbeat set to 60
after 30 seconds, i hit save and it failed (as expected) because the nonce was not valid any more and had not been refreshed
the api responds rest_cookie_invalid_nonce

to complete testing, i'm investigating how nonces are used in wp.media to ensure those are being refreshed as well.

#19 @adamsilverstein
9 months ago

testing the media modal in GB, i see nonces used in media for the following actions.

Once my nonces expired these actions all failed. Worth testing if these failures also occur in the classic editor, i'm not sure if they were ever being refreshed.

Although we should ensure all nonces are refreshed I don't think this is a blocker for merging - only a very small percentage of users will leave their browser window open on the editor for long enough for normal nonces to expire, and these extra nonces already are not refreshed, so the classic editor similarly will fail to perform media actions, eg this isn't a regression.

Next: testing again with refreshing in place to see if any of these are currently being refreshed.

#20 @adamsilverstein
9 months ago

verified nonces for wp.media are not refreshed in gb or in the classic editor. once my nonce expires, the media actions noted above all fail. Since this isn't a regression, I think we can address this in a separate ticket.

#21 @matveb
9 months ago

Thanks for following up on this one. Is this ready to be committed?

#22 @adamsilverstein
9 months ago

  • Keywords commit added; needs-testing removed

Yes - think it is, I was going to await @dd32 to see if he wanted to add/revise the latest patch. As is though, I verified nonce refreshing works and if merging this unblocks the RC i'd say go for it!

#23 @youknowriad
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43939:

Block Editor: Refresh nonces used by wp.apiFetch.

Adds heartbeat nonces refreshing support to wp.apiFetch requests.

Props pento, adamsilverstein, dd32, desrosj.
Fixes #45113.

#24 @youknowriad
9 months ago

  • Keywords fixes-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#25 @youknowriad
9 months ago

  • Keywords fixed-5.0 added; fixes-5.0 removed

#26 @desrosj
8 months ago

In 44120:

Classic Editor: Disable the wpautop TinyMCE plugin on block posts.

As the block editor adds its own <p> tags, disabling the wpautop stops the classic editor from removing them.

Props pento.

Merges [43758] to trunk.

See #45113.

#27 @desrosj
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44275:

Block Editor: Refresh nonces used by wp.apiFetch.

Adds heartbeat nonces refreshing support to wp.apiFetch requests.

Props pento, adamsilverstein, dd32, desrosj, youknowriad.

Merges [43939] into trunk.

Fixes #45113.

#28 @aduth
5 months ago

In 44949:

Scripts: Assign api-fetch nonce with corrected rest_nonce.

As of @wordpress/api-fetch@3.0.0 (introduced in 44812), the apiFetch nonce middleware must have its nonce value assigned explicitly, and will no longer listen for heartbeat ticks automatically. This changeset adds an inline script for the default registration of the api-fetch script handle to assign the nonce value in response to the heartbeat action. In doing so, it removes the now-unused, misnamed rest-nonce property from the heartbeat response, whose original introduction served as temporary compatibility with earlier versions of @wordpress/api-fetch.

See https://github.com/WordPress/gutenberg/pull/13451
See #45113

Props adamsilverstein, nerrad .
Fixes #46107 .

Note: See TracTickets for help on using tickets.