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

Add AMP validation checking for Gutenberg blocks #1019

Merged
merged 49 commits into from Apr 16, 2018

Conversation

4 participants
@westonruter
Copy link
Member

commented Mar 13, 2018

  • Add source comments around each Gutenberg block to track validation errors.
  • Pass validation errors back in REST API response when Gutenberg saves a post. (Discovery needed.)
  • Show validation status in editor after saving post.

An invalid script inside of an HTML block which is inside of a Column block has sources that look like this:

image

Fixes #1009.

@westonruter westonruter requested a review from kienstra Mar 13, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2018

@kienstra feel free to pick this up from me.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Mar 13, 2018

Thanks, @westonruter! It'll be great to work on this PR. If it's alright, tomorrow is probably the earliest I could get to it. I'm working on #1018 now.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2018

There is a bug with how Mustache replacements are being done which is corrupting the JSON in the block attributes. I'm working on a fix for that. Not a blocker for work being done on this.

westonruter added some commits Mar 13, 2018

Add source comments around each Gutenberg block to track validation i…
…ssues

Remove requirement that amp_debug query var have a value

@westonruter westonruter force-pushed the add/block-validation branch from 4a10ee4 to 07aeee2 Mar 14, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

Rebased to include test page generated from #1018. Former head was 4a10ee4.

if ( ! empty( $block['innerBlocks'] ) ) {
$rendered_inner_blocks = self::render_blocks( $block['innerBlocks'] );
// Inject rendered inner blocks before closing tag of outer block (or at end of string if no closing). @todo Confirm approach here.

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 14, 2018

Author Member

I've raised this in WordPress #core-editor:

image

https://wordpress.slack.com/archives/C02QB2JS7/p1521043684000458

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

Let's not merge this as part of 0.7 since nested blocks are still in flux.

@kienstra kienstra self-assigned this Mar 18, 2018

kienstra added some commits Mar 18, 2018

Revert commit that removed REST API logic.
Commit ba9365 removed this,
as it wasn't needed for 0.7.
This restores the logic for the REST API,
but not the wp-cron logic.

See PR #971.
Prototype asynchronous notices for blocks.
The POST to the REST API in the JS file doesn't work yet.
And this doesn't apply a requirement in PR #1019:
'Pass validation errors back in REST API response when Gutenberg saves a post.'
But this is a prototype of a way to display notices for each block, async.
The UX would be like that in the previous PR #902.
As Weston mentioned earlier, edit() is synchronous.
And REST requests to validate content could delay it.
So add a component for 'edit.'
Display a notice, based on the state of isValidAMP.
The REST API request will update the state.
@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

@kienstra Here's an idea: what if we piggy-backed on the autosave functionality to incorporate the validation results? This could avoid the need to trigger an update when changing each block. We would be piggy-backing on the autosave interval.

Upon saving draft we'll want to do the loopback request to get the full validation results anyway and display them in a notification. (Also, a block may enqueue scripts which would need to be caught but process_markup does not currently do that since it does not do wp_head or wp_footer. ) The results after this save we'll want to add to the blocks. So in addition to there being a validation message at the top, there should also be the specific validation errors added to their respective individual blocks inside so that users know which ones specifically are problematic.

In short, I think perhaps this REST API endpoint should not be used, but rather a new REST field should be added to the existing post/page endpoints. When a POST/PUT request is made to create/update a post, this can trigger the same loopback request we're doing during save_post now, and then the array of validation errors could be returned as the contents of the field for the client to then display in the notification error and with each block. This could be done both during autosave and regular save requests.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2018

Validation On Saving Post

Hi @westonruter,
Thanks, that's a good idea to make a request to get the validation results on autosaving the post. The block edit() functions can fire a few times a second, so it was going to be hard to hook into those or componentDidMount().

I'll look at how we might display individual block notices when the page autosaves. So far, it looks setting the state of the AMPNotice component from outside the block might be hard. But there's probably another approach.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2018

So far, it looks setting the state of the AMPNotice component from outside the block might be hard. But there's probably another approach.

I think that it shouldn't actually be part of the state then in this case. Instead the AMP validation results should be stored in the store that then sends the data down as props. So then you just update the store with the validation results, and then the edit function for a block can determine (somehow) which validation errors are relevant to itself and then display them.

I'm guessing this is what you'll want to use: https://github.com/WordPress/gutenberg/tree/master/data

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2018

Thanks

Hi @westonruter,
Thanks, that looks like a better approach.

kienstra added some commits Mar 19, 2018

Rever commit 27e8b, which added REST API endpoint.
The approach changed,
and I'm going to add a response field to
existing endpoints.
Add amp_validation field to REST API response.
@todo: implement the output callback.
Thanks to Weston's suggestion,
This will enable displaying notices in blocks.
Output validation errors in REST API response.
In new field 'amp_validation_errors'.
@todo: we might not always want to output this.
It's mainly useful in the Gutenberg editor.
* @return array|null $validation_data Validation data if it's available, or null.
*/
public static function rest_field_amp_validation( $post_data, $field_name ) {
$validation_post = self::get_validation_status_post( $post_data['link'] );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 20, 2018

Author Member

Excellent re-use of this function.

One key thing is that instead of $post_data['link'] it should do amp_get_permalink( $post_data['id'] ) so that it can work in paired mode.

Idea: If this post is empty or the post type is not viewable, should we take the post content and try to validate it? In other words, this:

https://github.com/Automattic/amp-wp/blob/0bd1d58451d7a78b125e8eac2f1b7d02ebfb124a/includes/utils/class-amp-validation-utils.php#L549-L560

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 21, 2018

Collaborator

Good idea, @westonruter. This commit abstracts that snippet above into get_existing_validation_errors(), and reuses it here. If there are no existing errors, it validates the front-end.

The only issue is that this delays the response if it validates several posts.

*/
public static function add_rest_api_fields() {
register_rest_field(
array( 'post', 'page' ),

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 20, 2018

Author Member

When ! amp_is_canonical() then this should be the REST object types for all post types that have amp post type support. Otherwise, it should be added for all post types which have the editor post type support.

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 21, 2018

Collaborator

Thanks, @westonruter. This commit implements that, and updates the tests.

let block = wp.blocks.unregisterBlockType( blockType );
let OriginalBlockEdit = block.edit;

block.edit = class AMPNotice extends wp.element.Component {

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 20, 2018

Author Member

ES6 classes aren't going to work in IE11.

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 21, 2018

Collaborator

Thanks, @westonruter. This commit removes the ES6 class.


return module;

} )( window.jQuery );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 20, 2018

Author Member

We should eliminate jQuery as being a dependency for this script. Extending Gutenberg should be jQuery-free.

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 21, 2018

Collaborator

Thanks, @westonruter. This commit removes the jQuery dependency.

kienstra added some commits Mar 20, 2018

Remove jQuery dependency and ES6 class.
As Weston mentioned,
we can't use ES6 classes in IE11.
The script is probably going to change drastically,
in how it displays notices for validation errors.
So remove almost all of the existing logic.
Change which post types have the added field.
Per Weston's suggestion,
if this is Native AMP (canonical),
it will be all posts with 'editor' support.
Otherwise, all REST objects that have 'amp' support.

@westonruter westonruter force-pushed the add/block-validation branch from ca6c96c to 8372dbd Apr 12, 2018

@westonruter westonruter force-pushed the add/block-validation branch from 8372dbd to 7a60509 Apr 12, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

Autosaves just got improved as well, so this needs to be accounted for: WordPress/gutenberg#4156

@westonruter westonruter added this to the v1.0 milestone Apr 12, 2018

@westonruter westonruter changed the title [WIP] Add AMP validation checking for Gutenberg blocks Add AMP validation checking for Gutenberg blocks Apr 13, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2018

@kienstra this is now unblocked and ready for review/merge. The latest state of master in Gutenberg works with this PR (so first do git pull; npm install; npm run build for that plugin).

There will be more work to do on it of course, but the initial Gutenberg block validation seems important to get merged to start testing and using.

The easiest way to test this is to add a Custom HTML block and add a script tag to it. Upon saving you should see the notification at the top and with the block. Upon removal of the script and re-saving you should see the notice with the block disappear. There may be non-block validation errors (e.g. regarding enqueued CSS) which persist at the top and this is expected. You can ignore this warning in the console because it is regarding a problem with the Custom HTML block:

Warning: Failed prop type: You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly.

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2018

Approved

Hi @westonruter,
Great job with the Gutenberg validation here. It displays errors where expected:

amp-gutenberg-validation-1019

It's nice how the notice at the top of the editor says "But none are directly due to content here."

And as expected, there was no block validation error on the page created by the WP-CLI script create-gutenberg-test-post.php.


module.store = module.registerStore();

wp.data.subscribe( module.handleValidationErrorsStateChange );

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 16, 2018

Collaborator

Good idea to use wp.data.subscribe().

// Clear out block validation errors in case the block sand errors cannot be aligned.
wp.data.dispatch( module.storeName ).updateBlocksValidationErrors( {} );

noticeMessage += ' ' + wp.i18n._n(

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 16, 2018

Collaborator

Nice use of _n() to make the message clearer.

class_exists( 'WP_Block_Type_Registry' )
);
if ( $is_gutenberg_active ) {
add_filter( 'the_content', array( __CLASS__, 'add_block_source_comments' ), $do_blocks_priority - 1 );

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 16, 2018

Collaborator

Good idea to run this at the priority of $do_blocks_priority - 1.

* @return array|null $validation_data Validation data if it's available, or null.
*/
public static function get_amp_validity_rest_field( $post_data, $field_name, $request ) {
unset( $field_name );

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 16, 2018

Collaborator

This doesn't look to do any harm, but is there a reason to unset() this? Maybe to clarify that it's not used in the function.

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 16, 2018

Author Member

It's to prevent PhpStorm from flagging an unused function argument.

This comment has been minimized.

Copy link
@kienstra

kienstra Apr 16, 2018

Collaborator

Thanks, that makes sense.

@kienstra kienstra assigned westonruter and unassigned kienstra Apr 16, 2018

@westonruter westonruter merged commit c15a368 into develop Apr 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/block-validation branch Apr 16, 2018

@westonruter westonruter moved this from In progress to Ready for QA in v1.0 Apr 17, 2018

juanchaur1 added a commit to juanchaur1/amp-wp that referenced this pull request May 9, 2018

Prototype asynchronous notices for blocks.
The POST to the REST API in the JS file doesn't work yet.
And this doesn't apply a requirement in PR ampproject#1019:
'Pass validation errors back in REST API response when Gutenberg saves a post.'
But this is a prototype of a way to display notices for each block, async.
The UX would be like that in the previous PR ampproject#902.
As Weston mentioned earlier, edit() is synchronous.
And REST requests to validate content could delay it.
So add a component for 'edit.'
Display a notice, based on the state of isValidAMP.
The REST API request will update the state.
@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

Request For Testing

Hi @csossi,
Could you please test this?

  1. Go to https://native-ampconfdemo.pantheonsite.io/
  2. Create a new post
  3. Add a Custom HTML block
  4. Put this in the block:
<script>doThis();</script>
<button onclick="doSomething();">
	Click Me
</button>
  1. Publish the post
  2. Expected: there's a notice above the block, and at the top of the editor:

post-validation

7. Expected: clicking "Review issues" in the top notice leads to a page "Invalid AMP Page (URL)"
@csossi

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2018

verified in QA
image

@csossi csossi moved this from Ready for QA to Ready for Merging in v1.0 Jun 8, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2018

Request For Regression Testing

Hi @csossi,
Thanks for testing this. Could you please retest this to ensure there wasn't a regression, using the test steps above?

Also, could you please test for edge cases by creating several blocks, moving and duplicating blocks, and other combinations?

Here's the staging site:
https://paired-ampconfdemo.pantheonsite.io/

@kienstra kienstra moved this from Ready for Merging to Ready for QA in v1.0 Sep 5, 2018

@csossi

This comment has been minimized.

Copy link
Collaborator

commented Sep 30, 2018

verified in QA

@csossi csossi moved this from Ready for QA to Ready for Merging in v1.0 Sep 30, 2018

@csossi csossi removed their assignment Sep 30, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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.