Replace synchronous URL validation in editor with async validation#5741
Replace synchronous URL validation in editor with async validation#5741westonruter merged 84 commits intodevelopfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5741 +/- ##
=============================================
+ Coverage 74.98% 75.17% +0.19%
- Complexity 5679 5682 +3
=============================================
Files 213 214 +1
Lines 17059 17195 +136
=============================================
+ Hits 12791 12927 +136
Misses 4268 4268
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…T endpoint on post save
…debar' into feature/2069-async-url-validation
|
Plugin builds for 79751f5 are ready 🛎️!
|
…9-async-url-validation
…debar' into feature/2069-async-url-validation
| @@ -0,0 +1,51 @@ | |||
| /** | |||
There was a problem hiding this comment.
Added these unrelated tests to make up for a reduction in JS test coverage percentage, which was causing a ❌
| 'id' => [ | ||
| 'description' => __( 'Unique identifier for the object.', 'amp' ), | ||
| 'required' => true, | ||
| 'type' => 'integer', | ||
| ], | ||
| 'preview_id' => [ | ||
| 'description' => __( 'Unique identifier for the preview.', 'amp' ), | ||
| 'required' => false, | ||
| 'type' => 'integer', | ||
| ], | ||
| 'preview_nonce' => [ | ||
| 'description' => __( 'Preview nonce string.', 'amp' ), | ||
| 'required' => false, | ||
| 'type' => 'string', | ||
| ], |
There was a problem hiding this comment.
I realized we would do well to validate these parameters.
- Ensure the
idandpreview_idare positive integers which refer to an actual post. - Ensure the
preview_nonceis sanitized.
There was a problem hiding this comment.
This has been addressed in 669fdce.
Note that we don't have to manually check if preview_id is a positive integer since – as far as I understand and confirmed in tests – it's something done by the REST controller itself. This is why the only thing I've added is a validate_callback that checks if a post exists. With this, it seems the endpoint returns errors as expected:
String:
Negative number:
Inexistent post:
Also, the preview_nonce is now verified in a similar way as Core does in _show_post_preview()
There was a problem hiding this comment.
While working on this I realized there's no error handling in our UI (editor sidebar).
I'd say it's not mission-critical, but something we should definitely look into shortly. How about adding error reporting in the follow-up PR #5929 I've already started?
@jwold Having your input on this would be much appreciated.
There was a problem hiding this comment.
Yes, showing the any error resulting from validation can be incorporated into #5929.
|
@westonruter I think all your points have been addressed. Recheck and let me know if you find anything else. |
…dated in non-Standard mode
|
Fantastic! 🎉 |
Summary
Fixes #2069
Before this change, when a user is editing a post, the post's URL is re-validated synchronously on the
save_posthook.After this change, URL revalidation occurs in a separate process after a post has been saved (and on the initial load of the editor).
How it works
amp_validityREST field, which this PR removes. I suggest considering whether some of the fields might be removed entirely (rather than just via REST context), but I think this is a separate issue.isSavingPostto go from true to false, and at that time refetches validation results and passes them to the validation data store.Unfinished
It would be nice to do this: Eliminate synchronous loopback requests in favor of asynchronously re-checking AMP validity #2069 (comment)Done in c6a4f6fChecklist