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

Refactor storage of validation errors to use taxonomy terms #1093

Merged
merged 58 commits into from
May 29, 2018
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
2d5a341
Refactor storage of validation errors to use taxonomy terms
westonruter Apr 21, 2018
1105fe3
Add acknowledged/ignored statuses for validation errors
westonruter Apr 21, 2018
36cf381
Add filtering of validation errors by status
westonruter Apr 22, 2018
044d3c2
Add views for filtering invalid URLs by which contain specific error …
westonruter Apr 23, 2018
95487df
Update count with Invalid Pages admin menu to use count with new errors
westonruter Apr 23, 2018
212171f
Let non-new validation errors be collapsed by default when viewing in…
westonruter Apr 23, 2018
65999ea
Allow validation errors to be deleted which no longer have any occura…
westonruter Apr 23, 2018
c482b37
Ensure checkboxes for validation error terms are always shown; block …
westonruter Apr 23, 2018
a6d2f70
Add sorting of validation errors by creation date
westonruter Apr 24, 2018
546c163
Hide irrelevant published status from invalid URL post list
westonruter Apr 24, 2018
52f4cdd
Add amp_invalid_url post list table column for error status
westonruter Apr 24, 2018
8f31472
Add inline acknowledge/ignore actions to validation errors listed in …
westonruter Apr 24, 2018
1e1516d
Remove scheme/host from invalid AMP URLs in list table; show URL as h…
westonruter Apr 24, 2018
1cb4a69
Add error status to invalid AMP URL publish metabox; add view link
westonruter Apr 24, 2018
aadadaf
Only show count of new validation errors that have associated URLs in…
westonruter Apr 24, 2018
f885262
Remove script dependency object from enqueued_script validation error
westonruter Apr 24, 2018
7af2db5
Prevent showing AMP validation notice on edit post screen when all va…
westonruter Apr 25, 2018
03bf406
Explicitly only use (short) strings for description
westonruter Apr 6, 2018
c5e4202
Allow the validation_error_callback to return false to prevent saniti…
westonruter Apr 6, 2018
3916d2e
Improve handling of validation errors in style sanitizer
westonruter Apr 7, 2018
fd22331
Move validation error sources to term meta
westonruter Apr 25, 2018
5341f2b
Preserve validation error sources when coming from multiple origins
westonruter Apr 25, 2018
efc6661
Prevent Kses from corrupting JSON in term_description
westonruter Apr 26, 2018
8348190
Improve handling of validation errors for enqueued scripts
westonruter Apr 26, 2018
85989eb
Prevent serving AMP when there are non-sanitized validation errors
westonruter May 2, 2018
159d808
Ensure unique keys for React elements in notice
westonruter May 2, 2018
7048994
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter May 21, 2018
3c0d724
Include script/style handle in validation error source data
westonruter May 21, 2018
99759b0
Split up AMP_Validation_Utils into AMP_Validation_Manager, AMP_Invali…
westonruter May 22, 2018
de5c323
Prevent redirecting to canonical when doing validation of paired AMP
westonruter May 23, 2018
c9a33b1
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter May 25, 2018
d30ab72
Promote additional closures to methods
westonruter May 25, 2018
13c14a9
Rename "Acknowledge" with "Reject", and "Ignore" with "Accept"
westonruter May 25, 2018
71d36e7
Add iconography and success/warning messages for rejected/accepted st…
westonruter May 25, 2018
4cf66ba
Exclude webpack.config.js from build
westonruter May 25, 2018
9dde322
Fix excluding accepted validatione errors when showing notice in editor
westonruter May 25, 2018
9a79f05
Remove debug links since not helpful; let view links include param to…
westonruter May 25, 2018
932ec66
Vary parsed stylesheet cache by the sanitization status for its valid…
westonruter May 26, 2018
8a92355
Ensure node is made available to amp_validation_error filter from sty…
westonruter May 26, 2018
c58f897
Let parsed stylesheet caching be done invariably
westonruter May 26, 2018
e00e16d
Eliminate debug param in favor of explicitly requesting to preserve s…
westonruter May 26, 2018
6cd5e7e
Include sanitization status with each error when getting validation r…
westonruter May 26, 2018
9d159d5
Add prepare_validation_error_taxonomy_term helper method
westonruter May 26, 2018
7869b63
Prevent cached stylesheet validation errors from being reported twice…
westonruter May 27, 2018
1f315a8
Fix static analysis complaints in AMP_Style_Sanitizer
westonruter May 27, 2018
85a0585
Disable document.write() at DOM ready when serving dirty AMP
westonruter May 27, 2018
7c0a02d
Remove process_markup method from validation manager
westonruter May 27, 2018
90eff4f
Eliminate cookie authentication requirement to do frontend validation
westonruter May 27, 2018
69055c0
Make clear what it means to accept a validation error
westonruter May 28, 2018
f17df99
Add previewability to validation error status changes for invalid URL
westonruter May 28, 2018
f960f62
Vary parsed stylesheet cache by validation error status overrides
westonruter May 29, 2018
439bca5
De-duplicate code with helper methods
westonruter May 29, 2018
f29eaa2
Move invalid URL post action buttons to publish metabox
westonruter May 29, 2018
c3db57e
Vary CSS parse cache only when there are CSS-specific validation erro…
westonruter May 29, 2018
2b1985a
Show full invalid AMP URL on edit post screen
westonruter May 29, 2018
a4312b4
Always hide validation errors which lack any URLs; eliminate delete a…
westonruter May 29, 2018
cfa8bf5
Merge branch 'develop' of https://github.com/Automattic/amp-wp into u…
westonruter May 29, 2018
543b935
Combine isset() checks
westonruter May 29, 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
2 changes: 1 addition & 1 deletion Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ module.exports = function( grunt ) {
versionAppend = commitHash + '-' + new Date().toISOString().replace( /\.\d+/, '' ).replace( /-|:/g, '' );

paths = lsOutput.trim().split( /\n/ ).filter( function( file ) {
return ! /^(blocks|\.|bin|([^/]+)+\.(md|json|xml)|Gruntfile\.js|tests|wp-assets|dev-lib|readme\.md|composer\..*)/.test( file );
return ! /^(blocks|\.|bin|([^/]+)+\.(md|json|xml)|Gruntfile\.js|tests|wp-assets|dev-lib|readme\.md|composer\..*|webpack.*)/.test( file );
} );
paths.push( 'vendor/autoload.php' );
paths.push( 'assets/js/*-compiled.js' );
Expand Down
26 changes: 20 additions & 6 deletions assets/js/amp-block-validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,14 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars

currentPost = wp.data.select( 'core/editor' ).getCurrentPost();
ampValidity = currentPost[ module.data.ampValidityRestField ] || {};
validationErrors = ampValidity.errors;
validationErrors = _.map(
_.filter( ampValidity.results, function( result ) {
return ! result.sanitized;
} ),
function( result ) {
return result.error;
}
);

// Short-circuit if there was no change to the validation errors.
if ( ! validationErrors || _.isEqual( module.lastValidationErrors, validationErrors ) ) {
Expand Down Expand Up @@ -172,13 +179,13 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
);
}

noticeMessage += ' ' + wp.i18n.__( 'Invalid code is stripped when displaying AMP.', 'amp' );
noticeMessage += ' ' + wp.i18n.__( 'Non-accepted validation errors prevent AMP from being served.', 'amp' );
noticeElement = wp.element.createElement( 'p', {}, [
noticeMessage + ' ',
ampValidity.link && wp.element.createElement(
ampValidity.review_link && wp.element.createElement(
'a',
{ key: 'details', href: ampValidity.link, target: '_blank' },
wp.i18n.__( 'Details', 'amp' )
{ key: 'review_link', href: ampValidity.review_link, target: '_blank' },
wp.i18n.__( 'Review issues', 'amp' )
)
] );

Expand Down Expand Up @@ -211,7 +218,14 @@ var ampBlockValidation = ( function() { // eslint-disable-line no-unused-vars
var blockValidationErrorsByUid, editorSelect, currentPost, blockOrder, validationErrors, otherValidationErrors;
editorSelect = wp.data.select( 'core/editor' );
currentPost = editorSelect.getCurrentPost();
validationErrors = currentPost[ module.data.ampValidityRestField ].errors;
validationErrors = _.map(
_.filter( currentPost[ module.data.ampValidityRestField ].results, function( result ) {
return ! result.sanitized;
} ),
function( result ) {
return result.error;
}
);
blockOrder = module.getFlattenedBlockOrder( editorSelect.getBlocks() );

otherValidationErrors = [];
Expand Down
8 changes: 4 additions & 4 deletions composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ class AMP_Autoloader {
'AMP_DOM_Utils' => 'includes/utils/class-amp-dom-utils',
'AMP_HTML_Utils' => 'includes/utils/class-amp-html-utils',
'AMP_Image_Dimension_Extractor' => 'includes/utils/class-amp-image-dimension-extractor',
'AMP_Validation_Utils' => 'includes/utils/class-amp-validation-utils',
'AMP_Validation_Manager' => 'includes/validation/class-amp-validation-manager',
Copy link
Contributor

@kienstra kienstra May 29, 2018

Choose a reason for hiding this comment

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

It's nice how this is now organized in the includes/validation/ directory.

'AMP_Invalid_URL_Post_Type' => 'includes/validation/class-amp-invalid-url-post-type',
'AMP_Validation_Error_Taxonomy' => 'includes/validation/class-amp-validation-error-taxonomy',
'AMP_String_Utils' => 'includes/utils/class-amp-string-utils',
'AMP_WP_Utils' => 'includes/utils/class-amp-wp-utils',
'AMP_Widget_Archives' => 'includes/widgets/class-amp-widget-archives',
Expand Down
5 changes: 1 addition & 4 deletions includes/class-amp-response-headers.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@ public static function send_header( $name, $value, $args = array() ) {
* Send Server-Timing header.
*
* @since 1.0
* @todo What is the ordering in Chrome dev tools? What are the colors about?
* @todo Is there a better name standardization?
* @todo Is there a way to indicate nested server timings, so an outer method's own time can be seen separately from the inner method's time?
*
* @param string $name Name.
* @param float $duration Duration. If negative, will be added to microtime( true ). Optional.
Expand All @@ -75,7 +72,7 @@ public static function send_header( $name, $value, $args = array() ) {
public static function send_server_timing( $name, $duration = null, $description = null ) {
$value = $name;
if ( isset( $description ) ) {
$value .= sprintf( ';desc=%s', wp_json_encode( $description ) );
$value .= sprintf( ';desc="%s"', str_replace( array( '\\', '"' ), '', substr( $description, 0, 100 ) ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replacing _ with space would have a more elegant result for class names.

Copy link
Member Author

Choose a reason for hiding this comment

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

This logic here is specifically for ensuring the well-formedness of the Server-Timing header. If we want to use spaces instead of underscores then we should do that at:

https://github.com/Automattic/amp-wp/blob/9dde3226a5998cab2ba12983d63f0fa8111fb338/includes/templates/class-amp-content-sanitizer.php#L96

}
if ( isset( $duration ) ) {
if ( $duration < 0 ) {
Expand Down
94 changes: 65 additions & 29 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,11 @@ public static function init() {
return;
}

AMP_Validation_Manager::init( array(
'should_locate_sources' => AMP_Validation_Manager::should_validate_response(),
) );

self::$init_start_time = microtime( true );
AMP_Validation_Utils::init();

self::purge_amp_query_vars();
self::handle_xhr_request();
Expand Down Expand Up @@ -154,15 +157,20 @@ public static function finish_init() {

self::add_hooks();
self::$sanitizer_classes = amp_get_content_sanitizers();
self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
self::$embed_handlers = self::register_content_embed_handlers();
}

/**
* Redirect to canonical URL if the AMP URL was loaded, since canonical is now AMP.
*
* @since 0.7
* @since 1.0 Added $exit param.
* @todo Rename to redirect_non_amp().
*
* @param bool $exit Whether to exit after redirecting.
*/
public static function redirect_canonical_amp() {
public static function redirect_canonical_amp( $exit = true ) {
if ( false !== get_query_var( amp_get_slug(), false ) ) { // Because is_amp_endpoint() now returns true if amp_is_canonical().
$url = preg_replace( '#^(https?://.+?)(/.*)$#', '$1', home_url( '/' ) );
if ( isset( $_SERVER['REQUEST_URI'] ) ) {
Expand All @@ -171,8 +179,14 @@ public static function redirect_canonical_amp() {

$url = amp_remove_endpoint( $url );

wp_safe_redirect( $url, 302 ); // Temporary redirect because canonical may change in future.
exit;
/*
* Temporary redirect because AMP URL may return when blocking validation errors
* occur or when a non-canonical AMP theme is used.
*/
wp_safe_redirect( $url, 302 );
if ( $exit ) {
exit;
}
}
}

Expand All @@ -191,7 +205,13 @@ public static function is_paired_available() {
return false;
}

if ( is_singular() && ! post_supports_amp( get_queried_object() ) ) {
/**
* Queried object.
*
* @var WP_Post $queried_object
*/
$queried_object = get_queried_object();
if ( is_singular() && ! post_supports_amp( $queried_object ) ) {
return false;
}

Expand Down Expand Up @@ -275,11 +295,6 @@ public static function add_hooks() {
$priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.PHP.NewConstants.php_int_minFound
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ), $priority );

// Add validation hooks *after* output buffering has started for the response.
if ( AMP_Validation_Utils::should_validate_response() ) {
AMP_Validation_Utils::add_validation_hooks();
}

// Commenting hooks.
add_filter( 'wp_list_comments_args', array( __CLASS__, 'set_comments_walker' ), PHP_INT_MAX );
add_filter( 'comment_form_defaults', array( __CLASS__, 'filter_comment_form_defaults' ) );
Expand Down Expand Up @@ -408,7 +423,7 @@ protected static function wp_kses_amp_mustache( $text ) {
*
* @param string $url Comment permalink to redirect to.
* @param WP_Comment $comment Posted comment.
* @return string URL.
* @return string|null URL if redirect to be done; otherwise function will exist.
*/
public static function filter_comment_post_redirect( $url, $comment ) {
$theme_support = get_theme_support( 'amp' );
Expand Down Expand Up @@ -443,6 +458,7 @@ public static function filter_comment_post_redirect( $url, $comment ) {
wp_send_json( array(
'message' => self::wp_kses_amp_mustache( $message ),
) );
return null;
}

/**
Expand Down Expand Up @@ -631,7 +647,7 @@ public static function amend_comment_form() {
* @see get_query_template()
*
* @param array $templates Template hierarchy.
* @returns array Templates.
* @return array Templates.
*/
public static function filter_paired_template_hierarchy( $templates ) {
$support = get_theme_support( 'amp' );
Expand Down Expand Up @@ -848,6 +864,13 @@ public static function print_amp_styles() {
* @param DOMDocument $dom Doc.
*/
public static function ensure_required_markup( DOMDocument $dom ) {
/**
* Elements.
*
* @var DOMElement $meta
* @var DOMElement $script
* @var DOMElement $link
*/
$head = $dom->getElementsByTagName( 'head' )->item( 0 );
if ( ! $head ) {
$head = $dom->createElement( 'head' );
Expand All @@ -856,11 +879,6 @@ public static function ensure_required_markup( DOMDocument $dom ) {
$meta_charset = null;
$meta_viewport = null;
foreach ( $head->getElementsByTagName( 'meta' ) as $meta ) {
/**
* Meta.
*
* @var DOMElement $meta
*/
if ( $meta->hasAttribute( 'charset' ) && 'utf-8' === strtolower( $meta->getAttribute( 'charset' ) ) ) { // @todo Also look for meta[http-equiv="Content-Type"]?
$meta_charset = $meta;
} elseif ( 'viewport' === $meta->getAttribute( 'name' ) ) {
Expand Down Expand Up @@ -1012,17 +1030,18 @@ public static function filter_customize_partial_render( $partial ) {
* @since 0.7
*
* @param string $response HTML document response. By default it expects a complete document.
* @param array $args {
* Args to send to the preprocessor/sanitizer.
*
* @type callable $remove_invalid_callback Function to call whenever a node is removed due to being invalid.
* }
* @param array $args Args to send to the preprocessor/sanitizer.
* @return string AMP document response.
* @global int $content_width
*/
public static function prepare_response( $response, $args = array() ) {
global $content_width;

if ( isset( $args['validation_error_callback'] ) ) {
_doing_it_wrong( __METHOD__, 'Do not supply validation_error_callback arg.', '1.0' );
unset( $args['validation_error_callback'] );
}

/*
* Check if the response starts with HTML markup.
* Without this check, JSON responses will be erroneously corrupted,
Expand All @@ -1032,25 +1051,23 @@ public static function prepare_response( $response, $args = array() ) {
return $response;
}

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

$args = array_merge(
array(
'content_max_width' => ! empty( $content_width ) ? $content_width : AMP_Post_Template::CONTENT_MAX_WIDTH, // Back-compat.
'use_document_element' => true,
'allow_dirty_styles' => self::is_customize_preview_iframe(), // Dirty styles only needed when editing (e.g. for edit shortcodes).
'allow_dirty_scripts' => is_customize_preview(), // Scripts are always needed to inject changeset UUID.
'disable_invalid_removal' => $is_validation_debug_mode,
'enable_response_caching' => (
( ! defined( 'WP_DEBUG' ) || true !== WP_DEBUG )
&&
! AMP_Validation_Utils::should_validate_response()
! AMP_Validation_Manager::should_validate_response()
),
),
$args
);

// Return cache if enabled and found.
$response_cache_key = null;
if ( true === $args['enable_response_caching'] ) {
// Set response cache hash, the data values dictates whether a new hash key should be generated or not.
$response_cache_key = md5( wp_json_encode( array(
Expand Down Expand Up @@ -1110,15 +1127,34 @@ public static function prepare_response( $response, $args = array() ) {
$dom_serialize_start = microtime( true );
self::ensure_required_markup( $dom );

if ( ! AMP_Validation_Manager::should_validate_response() && AMP_Validation_Manager::has_blocking_validation_errors() ) {
if ( amp_is_canonical() ) {
$dom->documentElement->removeAttribute( 'amp' );

/*
* Make sure that document.write() is disabled to prevent dynamically-added content (such as added
* via amp-live-list) from wiping out the page by introducing any scripts that call this function.
*/
if ( $head ) {
$script = $dom->createElement( 'script' );
$script->appendChild( $dom->createTextNode( 'document.addEventListener( "DOMContentLoaded", function() { document.write = function( text ) { throw new Error( "[AMP-WP] Prevented document.write() call with: " + text ); }; } );' ) );
$head->appendChild( $script );
}
} else {
self::redirect_canonical_amp( false );
return esc_html__( 'Redirecting to non-AMP version.', 'amp' );
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This conditional is the critical piece to the whole thing.


// @todo If 'utf-8' is not the blog charset, then we'll need to do some character encoding conversation or "entityification".
if ( 'utf-8' !== strtolower( get_bloginfo( 'charset' ) ) ) {
/* translators: %s is the charset of the current site */
trigger_error( esc_html( sprintf( __( 'The database has the %s encoding when it needs to be utf-8 to work with AMP.', 'amp' ), get_bloginfo( 'charset' ) ) ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
}

if ( AMP_Validation_Utils::should_validate_response() ) {
AMP_Validation_Utils::finalize_validation( $dom, array(
'remove_source_comments' => ! $is_validation_debug_mode,
if ( AMP_Validation_Manager::should_validate_response() ) {
AMP_Validation_Manager::finalize_validation( $dom, array(
'remove_source_comments' => ! isset( $_GET['amp_preserve_source_comments'] ), // WPCS: CSRF.
) );
}

Expand Down
17 changes: 17 additions & 0 deletions includes/options/class-amp-options-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static function register_settings() {
);

add_action( 'update_option_' . self::OPTION_NAME, array( __CLASS__, 'maybe_flush_rewrite_rules' ), 10, 2 );
add_action( 'admin_notices', array( __CLASS__, 'persistent_object_caching_notice' ) );
}

/**
Expand Down Expand Up @@ -259,4 +260,20 @@ public static function update_analytics_options( $data ) {
_deprecated_function( __METHOD__, '0.6', __CLASS__ . '::update_option' );
return self::update_option( 'analytics', wp_unslash( $data ) );
}

/**
* Outputs an admin notice if persistent object cache is not present.
*
* @return void
*/
public static function persistent_object_caching_notice() {
if ( ! wp_using_ext_object_cache() && 'toplevel_page_' . self::OPTION_NAME === get_current_screen()->id ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

'toplevel_page_' . self::OPTION_NAME can be replaced with get_plugin_page_hookname( self::OPTION_NAME, null ) to avoid hard coded prefix.

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 tried swapping that out and it worked but it broke the unit test. Feel free to amend the PR with the change.

printf(
'<div class="notice notice-warning"><p>%s <a href="%s">%s</a></p></div>',
esc_html__( 'The AMP plugin performs at its best when persistent object cache is enabled.', 'amp' ),
esc_url( 'https://codex.wordpress.org/Class_Reference/WP_Object_Cache#Persistent_Caching' ),
esc_html__( 'More details', 'amp' )
);
}
}
}
Loading