-
Notifications
You must be signed in to change notification settings - Fork 384
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
Replace synchronous URL validation in editor with async validation #5741
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these unrelated tests to make up for a reduction in JS test coverage percentage, which was causing a ❌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so close!
'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized we would do well to validate these parameters.
- Ensure the
id
andpreview_id
are positive integers which refer to an actual post. - Ensure the
preview_nonce
is sanitized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, showing the any error resulting from validation can be incorporated into #5929.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Will followup on #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_post
hook.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_validity
REST 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.isSavingPost
to 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