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 2 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
1 change: 1 addition & 0 deletions .dev-lib
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ PROJECT_SLUG=amp
function after_wp_install {
echo "Installing REST API..."
svn export -q https://plugins.svn.wordpress.org/jetpack/trunk/ "$WP_CORE_DIR/src/wp-content/plugins/jetpack"
svn export -q https://plugins.svn.wordpress.org/gutenberg/trunk/ "$WP_CORE_DIR/src/wp-content/plugins/gutenberg"
}
2 changes: 1 addition & 1 deletion includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ public static function prepare_response( $response, $args = array() ) {
return $response;
}

$is_validation_debug_mode = ! empty( $_REQUEST[ AMP_Validation_Utils::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok.
$is_validation_debug_mode = isset( $_REQUEST[ AMP_Validation_Utils::DEBUG_QUERY_VAR ] ); // WPCS: csrf ok.

$args = array_merge(
array(
Expand Down
62 changes: 38 additions & 24 deletions includes/utils/class-amp-dom-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,6 @@ public static function get_dom( $document ) {
// @todo In the future consider an AMP_DOMDocument subclass that does this automatically. See <https://github.com/Automattic/amp-wp/pull/895/files#r163825513>.
$document = self::convert_amp_bind_attributes( $document );

/*
* Prevent amp-mustache syntax from getting URL-encoded in attributes when saveHTML is done.
* While this is applying to the entire document, it only really matters inside of <template>
* elements, since URL-encoding of curly braces in href attributes would not normally matter.
* But when this is done inside of a <template> then it breaks Mustache. Since Mustache
* is logic-less and curly braces are not unsafe for HTML, we can do a global replacement.
* The replacement is done on the entire HTML document instead of just inside of the <template>
* elements since it is faster and wouldn't change the outcome.
*/
$placeholders = self::get_mustache_tag_placeholders();
$document = str_replace(
array_keys( $placeholders ),
array_values( $placeholders ),
$document
);

// Force all self-closing tags to have closing tags since DOMDocument isn't fully aware.
$document = preg_replace(
'#<(' . implode( '|', self::$self_closing_tags ) . ')[^>]*>(?!</\1>)#',
Expand Down Expand Up @@ -365,13 +349,51 @@ public static function get_content_from_dom_node( $dom, $node ) {
$self_closing_tags_regex = "#</({$self_closing_tags})>#i";
}

/*
* Prevent amp-mustache syntax from getting URL-encoded in attributes when saveHTML is done.
* While this is applying to the entire document, it only really matters inside of <template>
* elements, since URL-encoding of curly braces in href attributes would not normally matter.
* But when this is done inside of a <template> then it breaks Mustache. Since Mustache
* is logic-less and curly braces are not unsafe for HTML, we can do a global replacement.
* The replacement is done on the entire HTML document instead of just inside of the <template>
* elements since it is faster and wouldn't change the outcome.
*/
$mustache_tag_placeholders = self::get_mustache_tag_placeholders();
$mustache_tags_replaced = false;
$xpath = new DOMXPath( $dom );
$templates = $dom->getElementsByTagName( 'template' );
foreach ( $templates as $template ) {

// These attributes are the only ones that saveHTML() will URL-encode.
foreach ( $xpath->query( './/*/@src|.//*/@href|.//*/@action', $template ) as $attribute ) {
$attribute->nodeValue = str_replace(
array_keys( $mustache_tag_placeholders ),
array_values( $mustache_tag_placeholders ),
$attribute->nodeValue,
$count
);
if ( $count ) {
$mustache_tags_replaced = true;
}
}
}

$html = $dom->saveHTML( $node );

// Whitespace just causes unit tests to fail... so whitespace begone.
if ( '' === trim( $html ) ) {
return '';
}

// Restore amp-mustache placeholders which were replaced to prevent URL-encoded corruption by saveHTML.
if ( $mustache_tags_replaced ) {
$html = str_replace(
array_values( $mustache_tag_placeholders ),
array_keys( $mustache_tag_placeholders ),
$html
);
}

// Restore noscript elements which were temporarily removed to prevent libxml<2.8 parsing problems.
if ( version_compare( LIBXML_DOTTED_VERSION, '2.8', '<' ) ) {
$html = str_replace(
Expand All @@ -383,14 +405,6 @@ public static function get_content_from_dom_node( $dom, $node ) {

$html = self::restore_amp_bind_attributes( $html );

// Restore amp-mustache placeholders which were replaced to prevent URL-encoded corruption by saveHTML.
$placeholders = self::get_mustache_tag_placeholders();
$html = str_replace(
array_values( $placeholders ),
array_keys( $placeholders ),
$html
);

/*
* Travis w/PHP 7.1 generates <br></br> and <hr></hr> vs. <br/> and <hr/>, respectively.
* Travis w/PHP 7.x generates <source ...></source> vs. <source ... />. Etc.
Expand Down
91 changes: 91 additions & 0 deletions includes/utils/class-amp-validation-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,97 @@ public static function add_validation_hooks() {

add_filter( 'do_shortcode_tag', array( __CLASS__, 'decorate_shortcode_source' ), -1, 2 );
add_filter( 'amp_content_sanitizers', array( __CLASS__, 'add_validation_callback' ) );

$is_gutenberg_active = (
function_exists( 'do_blocks' )
&&
function_exists( 'gutenberg_parse_blocks' )
&&
function_exists( 'gutenberg_render_block' )
&&
class_exists( 'WP_Block_Type_Registry' )
);
if ( $is_gutenberg_active ) {
$do_blocks_priority = has_filter( 'the_content', 'do_blocks' );
if ( false !== $do_blocks_priority ) {
remove_filter( 'the_content', 'do_blocks', $do_blocks_priority );
add_filter( 'the_content', array( __CLASS__, 'do_blocks' ), $do_blocks_priority );
}
}
}

/**
* Parse blocks out of content and render with AMP source stack comments added to demarcate blocks.
*
* @see do_blocks() prior to https://github.com/WordPress/gutenberg/commit/49f05e53a4e9ceff0c08fe646f3b0bb724f58b53
*
* @param string $content Content.
* @return string Content with rendered blocks.
*/
public static function do_blocks( $content ) {
$blocks = gutenberg_parse_blocks( $content );
return self::render_blocks( $blocks );
}

/**
* Render parsed blocks.
*
* @param array $blocks Blocks.
* @return string Rendered blocks.
*/
protected static function render_blocks( $blocks ) {
$rendered_blocks = '';

foreach ( $blocks as $block ) {
$rendered_block = gutenberg_render_block( $block );

// Obtain source information for block.
$source = null;
if ( ! empty( $block['blockName'] ) ) {
$source = array(
'block_name' => $block['blockName'],
);
if ( ! empty( $block['attrs'] ) ) {
$source['block_attrs'] = $block['attrs'];
}
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
if ( $block_type && $block_type->is_dynamic() ) {
$callback_source = self::get_source( $block_type->render_callback );
if ( $callback_source ) {
$source = array_merge(
$source,
$callback_source
);
}
}
}

// Prepend rendered block with start source comment.
if ( $source ) {
$rendered_block = self::get_source_comment( $source, true ) . $rendered_block;
}

// Render nested blocks.
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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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

image

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

$rendered_block = preg_replace(
'#(?=(</[a-zA-Z0-9_-]+>\s*)$)#s',
$rendered_inner_blocks,
$rendered_block,
1
);
}

// Append rendered block with end source comment.
if ( $source ) {
$rendered_block .= self::get_source_comment( $source, false );
}

$rendered_blocks .= $rendered_block;
}
return $rendered_blocks;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
convertWarningsToExceptions="true"
>
<php>
<const name="WP_TEST_ACTIVATED_PLUGINS" value="jetpack/jetpack.php" />
<const name="WP_TEST_ACTIVATED_PLUGINS" value="jetpack/jetpack.php,gutenberg/gutenberg.php" />
</php>
<testsuites>
<testsuite>
Expand Down
65 changes: 65 additions & 0 deletions tests/test-class-amp-dom-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ public function test_amp_bind_conversion() {
* Test handling of empty elements.
*
* @covers \AMP_DOM_Utils::get_dom()
* @covers \AMP_DOM_Utils::get_content_from_dom_node()
*/
public function test_html5_empty_elements() {
$original = '<amp-video width="432" height="987">';
Expand All @@ -146,6 +147,70 @@ public function test_html5_empty_elements() {
$this->assertEquals( 'span', $video->childNodes->item( 5 )->nodeName );
}

/**
* Test parsing DOM with Mustache or Mustache-like templates.
*
* @covers \AMP_DOM_Utils::get_dom()
* @covers \AMP_DOM_Utils::get_content_from_dom_node()
*/
public function test_mustache_replacements() {

$data = array(
'foo' => array(
'bar' => array(
'baz' => array(),
),
),
);

$html = implode( "\n", array(
'<!--amp-source-stack {"block_name":"core\/columns"}-->',
'<div class="wp-block-columns has-2-columns">',
'<!--amp-source-stack {"block_name":"core\/quote","block_attrs":{"layout":"column-1"}}-->',
'<blockquote class="wp-block-quote layout-column-1"><p>Quote</p><cite>Famous</cite></blockquote>',
'<!--/amp-source-stack {"block_name":"core\/quote","block_attrs":{"layout":"column-1"}}-->',
'<!-- wp:paragraph -->',
'<p><a href="https://example.com/"><img src="https://example.com/img.jpg"></a></p>',
'<!-- /wp:paragraph -->',
'</div>',
'<!--/amp-source-stack {"block_name":"core\/columns"}-->',
'<!-- wp:html {} -->',
'<script type="application/json">' . wp_json_encode( $data ) . '</script>',
'<template type="amp-mustache">Hello {{world}}! <a href="{{href}}" title="Hello {{name}}"><img src="{{src}}"></a><blockquote cite="{{cite}}">{{quote}}</blockquote></template>',
'<!-- /wp:html -->',
) );

$dom = AMP_DOM_Utils::get_dom_from_content( $html );
$xpath = new DOMXPath( $dom );

// Ensure that JSON in scripts are left intact.
$script = $xpath->query( '//script' )->item( 0 );
$this->assertEquals(
$data,
json_decode( $script->nodeValue, true )
);

// Ensure that mustache var in a[href] attribute is intact.
$template_link = $xpath->query( '//template/a' )->item( 0 );
$this->assertSame( '{{href}}', $template_link->getAttribute( 'href' ) );
$this->assertEquals( 'Hello {{name}}', $template_link->getAttribute( 'title' ) );

// Ensure that mustache var in img[src] attribute is intact.
$template_img = $xpath->query( '//template/a/img' )->item( 0 );
$this->assertEquals( '{{src}}', $template_img->getAttribute( 'src' ) );

// Ensure that mustache var in blockquote[cite] is not changed.
$template_blockquote = $xpath->query( '//template/blockquote' )->item( 0 );
$this->assertEquals( '{{cite}}', $template_blockquote->getAttribute( 'cite' ) );

$serialized_html = AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement );

$this->assertContains( '<a href="{{href}}" title="Hello {{name}}">', $serialized_html );
$this->assertContains( '<img src="{{src}}">', $serialized_html );
$this->assertContains( '<blockquote cite="{{cite}}">', $serialized_html );
$this->assertContains( '"block_attrs":{"layout":"column-1"}}', $serialized_html );
}

/**
* Get Table Row Iterations
*
Expand Down
85 changes: 84 additions & 1 deletion tests/test-class-amp-validation-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public function test_init() {
}

/**
* Test init.
* Test add_validation_hooks.
*
* @covers AMP_Validation_Utils::add_validation_hooks()
*/
Expand All @@ -131,6 +131,89 @@ public function test_add_validation_hooks() {
$this->assertEquals( -1, has_action( 'do_shortcode_tag', array( self::TESTED_CLASS, 'decorate_shortcode_source' ) ) );
}

/**
* Test add_validation_hooks with Gutenberg active.
*
* @covers AMP_Validation_Utils::add_validation_hooks()
*/
public function test_add_validation_hooks_gutenberg() {
if ( ! function_exists( 'do_blocks' ) ) {
$this->markTestSkipped( 'Gutenberg not active.' );
}

$this->assertEquals( 9, has_filter( 'the_content', 'do_blocks' ) );
AMP_Validation_Utils::add_validation_hooks();
$this->assertFalse( has_filter( 'the_content', 'do_blocks' ) );
}

/**
* Get block data.
*
* @ses Test_AMP_Validation_Utils::test_do_blocks()
* @return array
*/
public function get_block_data() {
return array(
'paragraph' => array(
"<!-- wp:paragraph -->\n<p>Latest posts:</p>\n<!-- /wp:paragraph -->",
"<!--amp-source-stack {\"block_name\":\"core\/paragraph\"}-->\n<p>Latest posts:</p>\n<!--/amp-source-stack {\"block_name\":\"core\/paragraph\"}-->",
array(
'element' => 'p',
'blocks' => array( 'core/paragraph' ),
),
),
'latest_posts' => array(
'<!-- wp:latest-posts /-->',
'<!--amp-source-stack {"block_name":"core\/latest-posts","type":"plugin","name":"gutenberg","function":"render_block_core_latest_posts"}--><ul class="wp-block-latest-posts aligncenter"></ul><!--/amp-source-stack {"block_name":"core\/latest-posts","type":"plugin","name":"gutenberg","function":"render_block_core_latest_posts"}-->',
array(
'element' => 'ul',
'blocks' => array( 'core/latest-posts' ),
),
),
'columns' => array(
"<!-- wp:columns -->\n<div class=\"wp-block-columns has-2-columns\">\n <!-- wp:quote {\"layout\":\"column-1\",\"foo\":{\"bar\":1}} -->\n <blockquote class=\"wp-block-quote layout-column-1\">\n <p>A quotation!</p><cite>Famous</cite></blockquote>\n <!-- /wp:quote -->\n\n <!-- wp:html {\"layout\":\"column-2\"} -->\n <div class=\"layout-column-2\">\n <script>\n document.write('Not allowed!');\n </script>\n </div>\n <!-- /wp:html -->\n</div>\n<!-- /wp:columns -->",
"<!--amp-source-stack {\"block_name\":\"core\/columns\"}-->\n<div class=\"wp-block-columns has-2-columns\">\n\n\n\n<!--amp-source-stack {\"block_name\":\"core\/quote\",\"block_attrs\":{\"layout\":\"column-1\",\"foo\":{\"bar\":1}}}-->\n <blockquote class=\"wp-block-quote layout-column-1\">\n <p>A quotation!</p><cite>Famous</cite></blockquote>\n <!--/amp-source-stack {\"block_name\":\"core\/quote\",\"block_attrs\":{\"layout\":\"column-1\",\"foo\":{\"bar\":1}}}--><!--amp-source-stack {\"block_name\":\"core\/html\",\"block_attrs\":{\"layout\":\"column-2\"}}-->\n <div class=\"layout-column-2\">\n <script>\n document.write('Not allowed!');\n </script>\n </div>\n <!--/amp-source-stack {\"block_name\":\"core\/html\",\"block_attrs\":{\"layout\":\"column-2\"}}--></div>\n<!--/amp-source-stack {\"block_name\":\"core\/columns\"}-->",
array(
'element' => 'blockquote',
'blocks' => array(
'core/columns',
'core/quote',
),
),
),
);
}

/**
* Test do_blocks.
*
* @param string $content Content.
* @param string $expected Expected content.
* @param array $query Query.
* @dataProvider get_block_data
* @covers AMP_Validation_Utils::do_blocks()
* @covers AMP_Validation_Utils::render_blocks()
*/
public function test_do_blocks( $content, $expected, $query ) {
if ( ! function_exists( 'do_blocks' ) ) {
$this->markTestSkipped( 'Gutenberg not active.' );
}

$rendered_block = AMP_Validation_Utils::do_blocks( $content );
$this->assertEquals(
preg_replace( '/\s+/', ' ', $expected ),
preg_replace( '/\s+/', ' ', $rendered_block )
);

$dom = AMP_DOM_Utils::get_dom_from_content( $rendered_block );
$el = $dom->getElementsByTagName( $query['element'] )->item( 0 );

$this->assertEquals(
$query['blocks'],
wp_list_pluck( AMP_Validation_Utils::locate_sources( $el ), 'block_name' )
);
}

/**
* Test add_validation_error.
*
Expand Down