Amp-wp: Eliminate synchronous loopback requests in favor of asynchronously re-checking AMP validity
Feature description
Every time a post is saved in the editor, the AMP plugin performs a loopback request to that post's permalink on the frontend to then check it for validation errors. In the block editor, this loopback request is doing _during_ the handling of the PUT request; this is done so that the PUT response can include all of the validation error data, but it also means that the PUT request takes _much_ longer then it would normally.
In the classic editor this loopback request is performed when the the whole page is reloading after saving, and it is also done when activating a new plugin and switching between template modes. The loopback requests here also slow down the page load time, but with the benefit being the validation results can be easily displayed immediately in the page response.
Nevertheless, it would be much better to switch to doing asynchronous loopback requests. When updating a post in the block editor, a successful PUT request should then trigger another request to a new REST API endpoint dedicated to URL validation. So clicking the Save button would then kick off a new loading message and spinner that could say, “Checking AMP validity”. When this returns, any AMP validation messages would be added/removed in the block interface.
Similarly, when activating a new plugin, this loopback request should be performed via Ajax on the plugin screen; an admin notice can be added which contains, “Re-checking AMP validity…” with the notice then updated to he success or error based on the result.
_Do not alter or remove anything below. The following sections will be managed by moderators only._
Acceptance criteria
- Switch to asynchronous loopback requests
- When updating a post in the block editor, a successful PUT request should trigger another request to a new REST API endpoint dedicated to URL validation.
- Clicking the Save button should kick off a new loading message and spinner that could say, “Checking AMP validity”.
- When this returns, any AMP validation messages should be added/removed in the block interface.
Implementation brief
- Consider adding a new
amp_uuidblock attribute for all blocks which persists across page loads. This must be a UUID that remains unique even after duplicating a block. If such a UUID were in place, then it would be possible to store theamp_uuidwith the block attributes in the validation error, and this could then be used to connect a validation error to the corresponding block in the editor.
QA testing instructions
Demo
Changelog entry
westonruter
All 31 comments
Can this solution also support async validation for the classic editor? We have specifically disabled the block editor on our site using https://wordpress.org/plugins/classic-editor/, and automatic validation essentially doubles the already slow Update/Publish action.
archon810
on 10 Sep 2019
I just did some benchmarking on our dev server, out of a 12 second run after hitting Update, 9s is a sync call to AMP validate.
So to our writers, launching AMP means a 4x increase in wait time, which is excruciating.
Please let's change it to async ASAP.
archon810
on 12 Sep 2019
Try this extension to the AMP plugin: https://gist.github.com/westonruter/31ac0e056b8b1278c98f8a9f548fcc1a
This is a workaround for sites encountering slowness when updating posts due to the synchronous loopback requests which happen at post save to report back validation errors to the editor; instead of running at the save_post action, the post is queued for validation in an immediate event via WP Cron. The AMP plugin intends to make this process asynchronous, but until that lands this plugin can be used. A side effect of turning off synchronous validation is that validation error warnings are no longer displayed in the editor. To see the errors, you have to go to the AMP Validated URLs screen.
westonruter
on 12 Sep 2019
Are you sure they won't be displayed? Because for me right now, they display every time I reload the edit page, not just after pressing update. They are persistent.
archon810
on 12 Sep 2019
I do not see any warning, no. Not initially, not after update, not after reload.
The plugin prevents the admin notice from being added via this code:
remove_action( 'edit_form_top', [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ] );
If I comment that line out, then I see the admin notice again.
westonruter
on 12 Sep 2019
Oh, I didn't know you added that into this particular plugin, as it's a related feature but not necessarily in the same scope.
We discussed earlier hiding these from non-admin users. I think the plugin should just do that as a compromise. It's still valuable to see those warnings for admins.
archon810
on 12 Sep 2019
See the plugin source for the amp_async_validation filter. There is some example code you can use to disable the async validation conditionally, such as when the user is an admin.
Hiding of the notices is really bound up with the async validation, because as soon as async validation is performed then the notices are no longer accurate.
westonruter
on 12 Sep 2019
I'd be OK with the notices showing and being delayed if the Recheck button continued to work and be synchronous.
archon810
on 12 Sep 2019
Notices with stale information will be frustrating. If there is a script in the content, and a validation error notice appears because of it, removing the script should cause the notice to go away immediately after saving. This is only possible with synchronous validation. So when using async validation, the notice needs to be suppressed, at least in this little workaround plugin.
Figuring out how to do async validation _and_ show a notice at the same time... that is what this issue is all about. Synchronous validation makes it easy to show the notice, because the currently saved state is the same state that was validated. But if async, then the user could make changes while validation is being performed, and this can cause the validated state and the editor state to be out of sync. This is particularly a challenge for the block editor, since we try to show validation warning notices contextually with each block.
westonruter
on 12 Sep 2019
Perhaps the solution is a metabox that has the live validation status and can refresh it via Ajax? Or a custom notice at the top that can do the same?
If you don't want to change much, what about only showing the notice after the cron job ran? If it's pending, suppress?
We're using the classic editor, not the block editor, so I'd love the solution to work for classic as well.
archon810
on 12 Sep 2019
Mentioning as per Slack chat with @westonruter: currently, AMP_Validation_Manager::post_supports_validation() doesn't account for autosaves, which means the validator unnecessarily runs in the background for every autosave, which, if you leave a post open for a while, can amount to hundreds of autosaves and hundreds of wasted server resources.
Validation should only run on explicit post saves, not background ones. AMP_Validation_Manager::post_supports_validation() already filters out most of them in this code:
@westonruter's suggestion is to add a check for defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE.
archon810
on 12 Sep 2019
This is something we'll need to look into further. The ideal is that the user is provided feedback as soon as possible after they have added something that is not AMP compatible. The idea behind autosave is that, especially in the context of Gutenberg, warnings can be added shortly after a block has been added which does something not valid in AMP.
In any case, I've updated the AMP async validation plugin to prevent validation from being queued during autosave requests, while also preventing validation for non-published posts (since WP Cron is not authenticated as a user and thus can't access non-published posts on the frontend): https://gist.github.com/westonruter/31ac0e056b8b1278c98f8a9f548fcc1a
westonruter
on 12 Sep 2019
For doing async validation of draft posts, we'll need to find a secure way for WP Cron to make requests as the authenticated user who last modified the post.
westonruter
on 12 Sep 2019
Perhaps wp-ajax is the right way to go here?
archon810
on 12 Sep 2019
The request passed to WP Cron could include a token that can only be validated using a private key information the user has.
schlessera
on 26 Sep 2019
Continuing a train of thought from an in-person conversation with @adamsilverstein…
The issue with limiting validation results to autosave is that we need to handle the case of getting validation results when someone hits save draft or publish and no autosave has happened. We need to get the validation results and show them with the blocks they relate to. At the same time we shouldn't necessarily slow down the REST API call to save a post, especially for non-publish actions. Maybe that means triggering an autosave when the pre-publish flow is engaged, and prevent publishing until that save has completed and we have obtained the validation results. In other words, validation should be asynchronous and unblocking for saving posts in the editor UI, with the only exception being a soft-block during the presentation of the pre-publish panel. The pre-publish panel should display validation results once they have been asynchronously-obtained and block publishing until they have been addressed (marked as removed or kept). Certain limited user roles (e.g. author) would not necessarily see such validation UI, but that is to be tackled as part of #2673.
A closely related issue here is ensuring validation results can be applied to a document even after changes have been made since the time of getting the validation results. This is the idea above of adding a UUID to every block which is persistent, though which is ensured to be unique when duplicating blocks and when adding the same reusable block to the page. By keying block validation errors to the UUID we will no longer need to rely on the block index, which is brittle.
westonruter
on 5 Feb 2020
And continued further…
The synchronous validation being performed here is the loopback request that is performed to check the saved post on the frontend.
First of all, the AMP plugin registers a new amp-validity REST field for all supported posts:
When a PUT request is being done, it will initiate the a loopback request to obtain the validation results of the just-saved post on the frontend and return them in the PUT response body:
This slows down the request a lot. The whole thing needs to be refactored/revisited.
Again, if each block has a UUID this will help guard against a async delay in getting the validation results from causing validation errors to be displayed with the wrong blocks, since the block indices would no longer be used to match up the blocks with the errors.
Validation should no longer be performed in the REST API field callback. In fact, the amp-validity field should be removed entirely. Instead, there should be a REST API endpoint for validating any URL. This validation endpoint should be queried immediately after an autosave revision is saved, and the results can then update the UI with any changes for validation. When doing a Publish action, an autosave should be triggered followed by obtaining the validation results in a subsequent request… once these both complete, the pre-publish panel can display the validation results. (In the future, for some user roles, the presence of validation errors should perhaps block the post from being published. Again #2673.) This would be applicable to any template mode where validation results are provided. (Reader mode should get validation results as well: https://github.com/ampproject/amp-wp/issues/2044#issuecomment-582683838).
westonruter
on 10 Feb 2020
When doing a Publish action, an autosave should be triggered followed by obtaining the validation results in a subsequent request… once these both complete, the pre-publish panel can display the validation results.
One concern I have about this is it potentially means clicking Publish will require 3 HTTP requests (1 autosave, 1 validation, 1 publish) whereas at present only 1 HTTP request is required (the one to both update and publish at once). I suppose the latency is fine when the pre-publish checks are present, but if the user has turned them off then publishing should _not_ be blocked by any other requests. In such case, validation results would appear in the UI _after_ the user successfully published the post.
westonruter
on 10 Feb 2020
How about hooking into autosaves directly and adding the validation data to their response?
How/where is the validation actually performed against a URL, can you explain how this runs and I can try hooking up to autosaves and the autosave URL. Then we can show validation status on every autosave, and potentially lock publishing until validation passes.We can also trigger a recheck with a button that triggers an autosave.
Adding a UUID for each block makes sense if we need that, I'm still unclear why core GB doesn't have this in place already. We should be able to add a custom property via filters and set a UUID that way, I can try that.
adamsilverstein
on 21 Feb 2020
@westonruter responses to some of your specific points below as well as thoughts for moving forward...
Again, if each block has a UUID this will help guard against a async delay in getting the validation results from causing validation errors to be displayed with the wrong blocks, since the block indices would no longer be used to match up the blocks with the errors.
I have a uuid property working in my revisions plugin and can add it, currently looks something like this:
<!-- wp:paragraph {"uuid":"28e2488a-73b1-470f-9473-cb54fb67f21a"} -->
<p>b</p>
<!-- /wp:paragraph -->
I assume for the purposes of the validator, we would want to add this data to the preview HTML? what is the best form for this, a data attribute?
One concern I have about this is it potentially means clicking Publish will require 3 HTTP requests (1 autosave, 1 validation, 1 publish)
Validation can happen as _part of the autosave_ and amp-validity returned as part of the autosave response.
I suppose the latency is fine when the pre-publish checks are present, but if the user has turned them off then publishing should not be blocked by any other requests. In such case, validation results would appear in the UI after the user successfully published the post
There are two separate issues: where/when we show validation results and when we disable the publish or save button:
Triggering checks and displaying issues
- If an autosave returns a validation error at any point we can display that using the current approach (a Gutenberg error 'alert'). with uuid's in place these can link to the correct block and blocks with errors can be highlighted
- when the user visits the pre-publish panel, a new section will show 'AMP Validity'. if the post is dirty, an autosave is immediately triggered and this section shows a spinner until validity is checked.
- note: If the user has disabled the pre-publish panel, this section never shows.
- After a post is published or updated, if its amp-validity was not already determined for some reason (eg no autosave fired), we would check validity asynchronously.
Locking publishing or updating
- We can work on locking as a separate issue.
- When AMP is in standard mode (could be filterable) I propose requiring AMP validity to publish a post, or update a published post. Because checking validity requires an autosave. This works best with the pre-publish panel where we intelligently trigger an autosave. If the panel is disabled we might want to consider some other refresh mechanism or ui.
adamsilverstein
on 24 Feb 2020
How about hooking into autosaves directly and adding the validation data to their response?
Actually, I believe we're doing this currently. The autosave condition is what the above amp-async-plugin disables.
I assume for the purposes of the validator, we would want to add this data to the preview HTML? what is the best form for this, a data attribute?
The AMP plugin currently grabs any blog attributes and exposes them to the validation process via source stack comments, so this should be already available. See:
Validation can happen as _part of the autosave_ and
amp-validityreturned as part of the autosave response.
I think this is what we want to avoid because it causes extra long spinner for autosave requests. It also seems somewhat hacky to overload a PUT request in this way. If we can decouple it from the PUT request to update the post (whether autosave or publish) then we can consistently perform the validation asynchronously.
when the user visits the pre-publish panel, a new section will show 'AMP Validity'. if the post is dirty, an autosave is immediately triggered and this section shows a spinner until validity is checked.
Yes, I think what should happen is whenever Gutenberg is doing an autosave request, immediately when it finishes a second validation request (via a new endpoint) can be performed to obtain the validation results. This validation request after an autosave could be performed invisibly or indicated via some secondary AMP validation spinner. When clicking Publish, however, the autosave request should be triggered and the Publish button should be disabled until the autosave request and the subsequent validation request are completed. If the validation request returned no errors, then the Publish button would be enabled. Otherwise, if errors then it would remain disabled but then a list of errors could be shown in the panel below. The errors could perhaps be dismissed to allow the Publish button to be re-enabled.
- After a post is published or updated, if its amp-validity was not already determined for some reason (eg no autosave fired), we would check validity asynchronously.
In other words, I think the validation should always be performed asynchronously in terms of the HTTP requests. The UI may be synchronous/sequential in terms of the workflow.
- note: If the user has disabled the pre-publish panel, this section never shows.
If someone has disabled this then I think we should not block the publishing. Without the pre-publish checks enabled, the user has indicated they are fine to cowboy-publish. AMP validation errors can be displayed to them asynchronously in the same way as after an autosave happens.
westonruter
on 4 Mar 2020
Two bug reports that have come up since launching 2.0 add more urgency to implementing asynchronous validation.
On one hand there is less urgency because admins can turn off Developer Tools and non-admins don't have Developer Tools to begin with, and when there is no Developer Tools there is no synchronous validation being performed.
Nevertheless, all sites that have Reader mode now have Developer Tools turned on by default, and this is why we've seen a couple new issues arise. The problem is that with a synchronous validation request, we will create an amp_validated_url post during the same request to save a non-amp_validated_url post type. When a plugin implements a save_post callback that looks for some $_REQUEST variable as an indicator for whether to perform some action, this can end up getting executed both on the post being edited _and_ on the amp_validated_url post.
Two examples of this:
- OneSignal sends notifications for
amp_validated_urlposts being “published” (even though they are private): https://github.com/ampproject/amp-wp/issues/5297, https://github.com/OneSignal/OneSignal-WordPress-Plugin/pull/251 - Custom Permalinks plugin adds the same
custom_permalinkpost meta to the edited post and theamp_validated_urlpost: support topic, https://github.com/samiahmedsiddiqui/custom-permalinks/pull/40
Both of these would be avoided (especially the second) if we spawned a separate request to validate a post permalink _after_ the REST API request to update the post has completed.
westonruter
on 30 Aug 2020
@westonruter After #5515 is merged, synchronous loopback requests will be eliminated for users with dev tools turned off. They'll still exist for users with dev tools turned on. Just wanted to confirm with you that we should still go forward with this, correct?
I think it will be fairly straightforward to create the REST endpoint and send a JS fetch request after post save in the editor. I think we'd also want to merge #5589 first, though, because we'd want to show the "Re-checking" state in the new sidebar, and we'd probably use the sidebar's data store as well. #5589 is already pretty huge, so this would be better as a followup.
johnwatkins0
on 4 Dec 2020
After #5515 is merged, synchronous loopback requests will be eliminated for users with dev tools turned off. They'll still exist for users with dev tools turned on. Just wanted to confirm with you that we should still go forward with this, correct?
Synchronous loopback requests are already turned off for users who have DevTools turned off, no? No validation happens at all when a user has DevTools turned off.
And yes, for the moment users with DevTools enabled will continue to have synchronous validation being performed. To make validation asynchronous, I think we need to first address the concern of relying on the block index position in the content to identify which block goes with which errors. We discussed maybe using a UUID, but that can be problematic in that it would pollute the post_content.
westonruter
on 4 Dec 2020
I'm going to test out a UUID idea on the editor sidebar branch and will follow up. I have had some bugginess on that branch with posts with a lot of blocks. The index is sometimes off.
johnwatkins0
on 4 Dec 2020
Once this is implemented along with the plugin sidebar (#5589), then we can default to enabling DevTools for administrators regardless of template mode. This will revert part of #5333, specifically $enabled will by true by default here:
We disable DevTools by default in Reader mode because the UI is intrusive and it slows down saving of posts. But once these issues are solved, we can enable it again.
westonruter
on 9 Dec 2020
I'm going to test out a UUID idea on the editor sidebar branch and will follow up. I have had some bugginess on that branch with posts with a lot of blocks. The index is sometimes off.
Promised followup: I have persistent block UUIDs working on the sidebar branch, and still testing it out. We obviously want to avoid causing Gutenberg block validation errors if the AMP plugin is turned off, so I'm double checking everything.
johnwatkins0
on 9 Dec 2020
What about the issue of “polluting” block attributes with UUIDs? While most users won't notice this, anyone who uses the code editor would see a whole bunch of UUIDs all over the place, right?
westonruter
on 9 Dec 2020
@westonruter That is exactly right.
It does feel somewhat heavy handed to apply an override to every single block. Even though block order is currently unreliable, in working on the editor sidebar over several weeks I've only found it to be inaccurate once or twice. Maybe, on second thought, the amp_uuid is too heavy a solution for the problem.
When it comes to this issue, one thing we do know is that the core client IDs stay consistent within an editing session. Maybe instead of having a UUID that persists between editing sessions, we can do something like:
- On save, send a validation request to our new custom endpoint, and in the request include a list of the client IDs in the request POST data.
- In the REST response, return the errors with the client IDs.
- If any of the received errors includes a client ID that is no longer present in the editor, ignore it.
There's probably some holes to this, but I think a solution along these lines can be found. Let me know what you think -- removing the UUID update I made on the sidebar branch this morning (46a204eea89e25b07f6118b192680b05daa43b14) would be no problem.
johnwatkins0
on 9 Dec 2020
I think the client IDs are indeed the key here, but I think we don't even have to send them. What we need to know is what client IDs map to which block indices at the time of saving, right? That being the case, at the moment that a post is _initiating_ a save request (or for the initial editor load), we can capture an ordered list of the client IDs for the blocks in the content. After the save request finishes, the request to validate the saved post can proceed. It then will return with validation errors with each including the block_content_index in the response. Then when deciding which block goes which which error, we can refer to that stored mapping of client ID to block_content_index. If a block no longer exists with a given client ID, then it would be skipped from being associated with the block. I believe this will solve our stability problem while also unblock us from performing asynchronous validation. What do you think?
westonruter
on 9 Dec 2020
@westonruter Yes, I think we'll definitely be able to make something like that work when we have the validation REST endpoint. So for now I'm going to remove this UUID experiment from the sidebar branch and will switch to this issue immediately following the sidebar, so that hopefully both can be in 2.1.
johnwatkins0
on 10 Dec 2020
Related issues
alexhaller
·
5Comments
GitaStreet
·
4Comments
fumikito
·
5Comments
schlessera
·
5Comments
luizeof
·
4Comments