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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
8413457
Add source comments around each Gutenberg block to track validation i…
westonruter Mar 13, 2018
07aeee2
Defer mustache tag replacements to right before serialization and onl…
westonruter Mar 14, 2018
01691e5
Eliminate needless use of PEG parser for adding block source comments
westonruter Mar 17, 2018
27e8bec
Revert commit that removed REST API logic.
Mar 18, 2018
590cd76
Prototype asynchronous notices for blocks.
Mar 18, 2018
8218919
Rever commit 27e8b, which added REST API endpoint.
Mar 19, 2018
9c3568d
Add amp_validation field to REST API response.
Mar 19, 2018
ab82e8d
Output validation errors in REST API response.
Mar 19, 2018
ea1c37a
Remove jQuery dependency and ES6 class.
Mar 20, 2018
96297fc
Change which post types have the added field.
Mar 20, 2018
ed4b03b
In REST API response, validate front-end if no errors exist.
Mar 21, 2018
561af32
Skip Gutenberg-based tests for WP version < 4.9.
Mar 21, 2018
f64ab2e
Begin to add notices to blocks based on errors.
Mar 21, 2018
b9bd206
Address Travis error by aligning array values vertically.
Mar 21, 2018
bcdff5f
Get the block types with errors from the REST API response.
Mar 22, 2018
049e482
Update test to reflect change in text.
Mar 22, 2018
fe34e8e
Store the block validation errors, avoiding lookup in every edit().
Mar 22, 2018
49b66ae
Get validation errors for specific blocks, not only for block names.
Mar 22, 2018
743cca4
Address Travis errors by raising variable declaration.
Mar 22, 2018
4676777
Add 'block_attrs' to blocksWithErrors.
Mar 22, 2018
b9aa16c
Correct the variable for block_attrs.
Mar 22, 2018
e2fc9a7
Use the new blockAttrs to find a match with errors.
Mar 23, 2018
6cd996a
Move the notice from below to above the block.
Mar 23, 2018
339f69f
Add a 'More details' link to the notice.
Mar 23, 2018
a02d029
Enable showing multiple validation errors.
Mar 23, 2018
15f0abe
Enable outputting several error codes, and their counts.
Mar 23, 2018
2b03c6a
Address a Travis error regarding complexity.
Mar 23, 2018
d366da3
Remove the counts from after the error codes.
Mar 23, 2018
80d32de
Add keys to the components and edit block.
Mar 23, 2018
16388c2
Make the notice expandable.
Mar 26, 2018
f9d8575
Merge in develop, resolve conflicts.
kienstra Apr 6, 2018
9aa6704
Address Travis error by removing extra comma.
kienstra Apr 6, 2018
dcb1e77
Force re-validation of post on frontend for amp_validation_errors fie…
westonruter Apr 8, 2018
eb00bcf
Prevent re-validating posts that have just been validated
westonruter Apr 8, 2018
9ecc24c
Fix: Each child in an array or iterator should have a unique "key" prop
westonruter Apr 8, 2018
354fdcd
Remove block error summary while waiting for design to formulate
westonruter Apr 8, 2018
0bada73
Fix delay between save and update of validation error notice
westonruter Apr 9, 2018
34024cc
Use eslint-config-wordpress and fix eslint issues
westonruter Apr 9, 2018
8e78904
Prevent reporting validation errors for blocks that are not in the cu…
westonruter Apr 9, 2018
277da63
Use eslint config adapted from Gutenberg
westonruter Apr 10, 2018
ca3ff60
Use block content index to match blocks with corresponding validation…
westonruter Apr 10, 2018
01a21a8
Only update block validation errors when editor state is clean
westonruter Apr 10, 2018
3e032d4
Handle showing validation errors for nested blocks
westonruter Apr 11, 2018
6a4cbcc
Add initial overall warning notice when there are validation errors
westonruter Apr 11, 2018
4d7dc19
Improve organization of Gutenberg extension code; improve warning not…
westonruter Apr 11, 2018
21e60f0
Show details with each block's validation errors; improve styling
westonruter Apr 11, 2018
ca6c96c
Add link to validation error details in Gutenberg notice
westonruter Apr 12, 2018
7a60509
Use wp.element.Fragment instead of wrangling arrays with key props
westonruter Apr 12, 2018
5bf12da
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Apr 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions assets/js/amp-block-validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
/*jshint esversion: 6 */
/*global _, wp:true */
/**
* AMP Gutenberg integration.
*
* On editing a block, this checks that the content is AMP-compatible.
* And it displays a notice if it's not.
*/

/* exported ampBlockValidation */
var ampBlockValidation = ( function( $ ) {
var module = {

/**
* Holds data.
*/
data: {},

/**
* Boot module.
*
* @param {Object} data Object data.
* @return {void}
*/
boot: function( data ) {
module.data = data;
$( document ).ready( function() {
if ( 'undefined' !== typeof wp.blocks ) {
module.processBlocks();
}
} );
},

/**
* Gets all of the registered blocks, and overwrites their edit() functions.
*
* The new edit() functions will check if the content is AMP-compliant.
* If not, it will display a notice.
*
* @returns {void}
*/
processBlocks: function() {
var blocks = wp.blocks.getBlockTypes();
blocks.forEach( function( block ) {
if ( block.hasOwnProperty( 'name' ) ) {
module.overwriteEdit( block.name );
}
} );
},

/**
* Overwrites the edit() function of a block.
*
* Outputs the original edit function, stored in OriginalBlockEdit.
* This also appends a notice to the block.
* It only displays if the block's content isn't valid AMP,
*
* @see https://riad.blog/2017/10/16/one-thousand-and-one-way-to-extend-gutenberg-today/
* @param {string} blockType the type of the block, like 'core/paragraph'.
* @returns {void}
*/
overwriteEdit: function( blockType ) {
let block = wp.blocks.unregisterBlockType( blockType );
let OriginalBlockEdit = block.edit;

block.edit = class AMPNotice extends wp.element.Component {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 classes aren't going to work in IE11.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* The AMPNotice constructor.
*
* @param {object} props The component properties.
* @returns {void}
*/
constructor( props ) {
props.attributes.pendingValidation = false;
super( props );
this.validateAMP = _.throttle( this.validateAMP, 5000 );
this.state = { isInvalidAMP: false };
}

/**
* Outputs the existing block, with a Notice element below.
*
* The Notice only appears if the state of isInvalidAMP is false.
* It also displays the name of the block.
*
* @returns {array} The elements.
*/
render() {
let originalEdit;
let result;

result = [];
originalEdit = wp.element.createElement( OriginalBlockEdit, this.props );
if ( originalEdit ) {
result.push( originalEdit );
}
if ( this.state.isInvalidAMP && wp.components.hasOwnProperty( 'Notice' ) ) {
result.push( wp.element.createElement(
wp.components.Notice,
{
status: 'warning',
content: module.data.i18n.notice.replace( '%s', this.props.name ),
isDismissible: false
}
) );
}

this.props.attributes.pendingValidation = false;
return result;
}

/**
* Handler for after the component mounting.
*
* If validateAMP() changes the isInvalidAMP state, it will result in this method being called again.
* There's no need to check if the state is valid again.
* So this skips the check if pendingValidation is true.
*
* @returns {void}
*/
componentDidMount() {
if ( ! this.props.attributes.pendingValidation ) {
let content = this.props.attributes.content;
if ( 'string' !== typeof content ) {
content = wp.element.renderToString( content );
}
if ( content.length > 0 ) {
this.validateAMP( this.props.attributes.content );
}
}
this.props.attributes.pendingValidation = false;
}

/**
* Validates the content for AMP compliance, and sets the state of the Notice.
*
* Depending on the results of the validation,
* it sets the Notice component's isInvalidAMP state.
* This will cause the notice to either display or be hidden.
*
* @param {string} content The block content, from calling save().
* @returns {void}
*/
validateAMP( content ) {
this.setState( function() {

// Changing the state can cause componentDidMount() to be called, so prevent it from calling validateAMP() again.
component.props.attributes.pendingValidation = true;
return { isInvalidAMP: ( Math.random() > 0.5 ) };
} );

let component = this;
$.post(
module.data.endpoint,
{
markup: content
}
).done( function( data ) {
if ( data.hasOwnProperty( 'removed_elements' ) && ( 0 === data.removed_elements.length ) && ( 0 === data.removed_attributes.length ) ) {
component.setState( function() {

// Changing the state can cause componentDidMount() to be called, so prevent it from calling validateAMP() again.
component.props.attributes.pendingValidation = true;
return { isInvalidAMP: false };
} );
} else {
component.setState( function() {
component.props.attributes.pendingValidation = true;
return { isInvalidAMP: true };
} );
}
} );
}

};

wp.blocks.registerBlockType( blockType, block );
}

};

return module;

} )( window.jQuery );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

65 changes: 65 additions & 0 deletions includes/utils/class-amp-validation-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,13 @@ class AMP_Validation_Utils {
*/
const VALIDATION_ERRORS_META_BOX = 'amp_validation_errors';

/**
* The name of the REST API field with the AMP validation results.
*
* @var string
*/
const REST_FIELD_NAME = 'amp_validation_errors';

/**
* The errors encountered when validating.
*
Expand Down Expand Up @@ -204,6 +211,8 @@ public static function init() {
add_filter( 'dashboard_glance_items', array( __CLASS__, 'filter_dashboard_glance_items' ) );
add_action( 'rightnow_end', array( __CLASS__, 'print_dashboard_glance_styles' ) );
add_action( 'save_post', array( __CLASS__, 'handle_save_post_prompting_validation' ), 10, 2 );
add_action( 'enqueue_block_editor_assets', array( __CLASS__, 'enqueue_block_validation' ) );
add_action( 'rest_api_init', array( __CLASS__, 'add_rest_api_fields' ) );
}

add_action( 'edit_form_top', array( __CLASS__, 'print_edit_form_validation_status' ), 10, 2 );
Expand Down Expand Up @@ -1869,4 +1878,60 @@ public static function get_recheck_link( $post, $redirect_url, $recheck_url = nu
);
}

/**
* Enqueues the block validation script.
*
* @return void
*/
public static function enqueue_block_validation() {
$slug = 'amp-block-validation';

wp_enqueue_script(
$slug,
amp_get_asset_url( "js/{$slug}.js" ),
array( 'jquery' ),
AMP__VERSION,
true
);

$data = wp_json_encode( array(
'i18n' => array(
/* translators: %s: the name of the block */
'notice' => __( 'The %s block above has invalid AMP', 'amp' ),
),
) );
wp_add_inline_script( $slug, sprintf( 'ampBlockValidation.boot( %s );', $data ) );
}

/**
* Adds fields to the REST API responses, in order to display validation errors.
*
* @return void
*/
public static function add_rest_api_fields() {
register_rest_field(
array( 'post', 'page' ),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self::REST_FIELD_NAME,
array(
'get_callback' => array( __CLASS__, 'rest_field_amp_validation' ),
'schema' => array(
'description' => __( 'AMP validation results', 'amp' ),
'type' => 'object',
),
)
);
}

/**
* Adds a field to the REST API responses to display the validation status.
*
* @param array $post_data Data for the post.
* @param string $field_name The name of the field to add.
* @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'] );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return isset( $validation_post ) ? json_decode( $validation_post->post_content, true ) : null;
}

}
57 changes: 57 additions & 0 deletions tests/test-class-amp-validation-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public function test_init() {
$this->assertEquals( 10, has_action( 'admin_notices', self::TESTED_CLASS . '::remaining_error_notice' ) );
$this->assertEquals( 10, has_action( 'admin_menu', self::TESTED_CLASS . '::remove_publish_meta_box' ) );
$this->assertEquals( 10, has_action( 'add_meta_boxes', self::TESTED_CLASS . '::add_meta_boxes' ) );
$this->assertEquals( 10, has_action( 'rest_api_init', self::TESTED_CLASS . '::add_rest_api_fields' ) );
}

/**
Expand Down Expand Up @@ -1379,6 +1380,62 @@ public function test_get_recheck_link() {
$this->assertContains( 'Recheck the URL for AMP validity', $link );
}

/**
* Test enqueue_block_validation.
*
* @covers AMP_Validation_Utils::enqueue_block_validation()
*/
public function test_enqueue_block_validation() {
global $post;
$post = $this->factory()->post->create(); // WPCS: global override ok.
$slug = 'amp-block-validation';
AMP_Validation_Utils::enqueue_block_validation();

$script = wp_scripts()->registered[ $slug ];
$this->assertContains( 'js/amp-block-validation.js', $script->src );
$this->assertEquals( array( 'jquery' ), $script->deps );
$this->assertEquals( AMP__VERSION, $script->ver );
$this->assertTrue( in_array( $slug, wp_scripts()->queue, true ) );
$this->assertContains( 'ampBlockValidation.boot', $script->extra['after'][1] );
$this->assertContains( 'The %s block above has invalid AMP', $script->extra['after'][1] );
}

/**
* Test add_rest_api_fields.
*
* @covers AMP_Validation_Utils::add_rest_api_fields()
*/
public function test_add_rest_api_fields() {
global $wp_rest_additional_fields;
AMP_Validation_Utils::add_rest_api_fields();
$field = $wp_rest_additional_fields['post'][ AMP_Validation_Utils::REST_FIELD_NAME ];
$this->assertEquals(
$field['schema'],
array(
'description' => 'AMP validation results',
'type' => 'object',
)
);
$this->assertEquals( $field['get_callback'], array( self::TESTED_CLASS, 'rest_field_amp_validation' ) );
$this->assertEquals( $wp_rest_additional_fields['page'][ AMP_Validation_Utils::REST_FIELD_NAME ], array( self::TESTED_CLASS, 'rest_field_amp_validation' ) );
}

/**
* Test rest_field_amp_validation.
*
* @covers AMP_Validation_Utils::rest_field_amp_validation()
*/
public function test_rest_field_amp_validation() {
// @todo: implement the tested function.
$post_data = array(
'link' => home_url( '/' ),
);
$this->assertEquals( null, AMP_Validation_Utils::rest_field_amp_validation( $post_data, '' ) );

$this->create_custom_post();
assertEquals( $this->get_mock_errors(), AMP_Validation_Utils::rest_field_amp_validation( $post_data, '' ) );
}

/**
* Creates and inserts a custom post.
*
Expand Down