Skip to content

Eliminate AMP validation from Classic Editor #5996

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 19, 2021
Merged

Eliminate AMP validation from Classic Editor #5996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f7f447
Eliminate AMP validation from Classic Editor
westonruter Mar 19, 2021
4d8d3fe
Use __METHOD__ instead of __FUNCTION__ in _deprecated_function() calls
westonruter Mar 19, 2021
fec0b8d
Add note for why member variable is not yet removed
westonruter Mar 19, 2021
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
186 changes: 14 additions & 172 deletions includes/validation/class-amp-validation-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class AMP_Validation_Manager {
*
* Keys are post IDs and values are whether the post has been re-validated.
*
* @deprecated In 2.1 the classic editor block validation was removed. This is not removed yet since there is a mini plugin that uses it: https://gist.github.com/westonruter/31ac0e056b8b1278c98f8a9f548fcc1a.
* @var bool[]
*/
public static $posts_pending_frontend_validation = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable can now be removed, no? Unless it was being referenced directly by themes/plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that two plugins are making use of this variable, but in those cases they rely on the AMP plugin as a library so removing this variable shouldn't affect them.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be removed because it's being used by this mini plugin: https://gist.github.com/westonruter/31ac0e056b8b1278c98f8a9f548fcc1a

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Maybe we could add a note about that also so it isn't accidentally removed in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Expand Down Expand Up @@ -195,12 +196,10 @@ public static function init() {
AMP_Validated_URL_Post_Type::register();
AMP_Validation_Error_Taxonomy::register();

add_action( 'save_post', [ __CLASS__, 'handle_save_post_prompting_validation' ] );
add_action( 'enqueue_block_editor_assets', [ __CLASS__, 'enqueue_block_validation' ] );
add_action( 'edit_form_top', [ __CLASS__, 'print_edit_form_validation_status' ], 10, 2 );

// Add actions for checking theme support is present to determine plugin compatibility and show validation links in the admin bar.
// Actions and filters involved in validation.
// @todo Eliminate this in favor of async validation. See <https://github.com/ampproject/amp-wp/issues/5101>.
add_action(
'activate_plugin',
static function() {
Expand Down Expand Up @@ -529,92 +528,23 @@ public static function add_validation_error_sourcing() {
*
* This is intended to only apply to post edits made in the classic editor.
*
* @see AMP_Validation_Manager::validate_queued_posts_on_frontend()
*
* @param int $post_id Post ID.
* @deprecated In 2.1 the classic editor block validation was removed.
* @codeCoverageIgnore
*/
public static function handle_save_post_prompting_validation( $post_id ) {
global $pagenow;

if ( ! self::get_dev_tools_user_access()->is_user_enabled() ) {
return;
}

$post = get_post( $post_id );

$is_classic_editor_post_save = (
isset( $_SERVER['REQUEST_METHOD'] )
&&
'POST' === $_SERVER['REQUEST_METHOD']
&&
'post.php' === $pagenow
&&
isset( $_POST['post_ID'] ) // phpcs:ignore WordPress.Security.NonceVerification.Missing
&&
(int) $_POST['post_ID'] === (int) $post_id // phpcs:ignore WordPress.Security.NonceVerification.Missing
);

$should_validate_post = (
$is_classic_editor_post_save
&&
self::post_supports_validation( $post )
&&
! isset( self::$posts_pending_frontend_validation[ $post_id ] )
);
if ( $should_validate_post ) {
self::$posts_pending_frontend_validation[ $post_id ] = true;

// The reason for shutdown is to ensure that all postmeta changes have been saved, including whether AMP is enabled.
if ( ! has_action( 'shutdown', [ __CLASS__, 'validate_queued_posts_on_frontend' ] ) ) {
add_action( 'shutdown', [ __CLASS__, 'validate_queued_posts_on_frontend' ] );
}
}
public static function handle_save_post_prompting_validation() {
_deprecated_function( __METHOD__, '2.1' );
}

/**
* Validate the posts pending frontend validation.
*
* @see AMP_Validation_Manager::handle_save_post_prompting_validation()
*
* @return array Mapping of post ID to the result of validating or storing the validation result.
* @deprecated In 2.1 the classic editor block validation was removed.
* @codeCoverageIgnore
*/
public static function validate_queued_posts_on_frontend() {
$posts = array_filter(
array_map( 'get_post', array_keys( array_filter( self::$posts_pending_frontend_validation ) ) ),
function( $post ) {
return self::post_supports_validation( $post );
}
);

$validation_posts = [];

/*
* It is unlikely that there will be more than one post in the array.
* For the bulk recheck action, see AMP_Validated_URL_Post_Type::handle_bulk_action().
*/
foreach ( $posts as $post ) {
$url = amp_get_permalink( $post->ID );
if ( ! $url ) {
$validation_posts[ $post->ID ] = new WP_Error( 'no_amp_permalink' );
continue;
}

// Prevent re-validating.
self::$posts_pending_frontend_validation[ $post->ID ] = false;

$invalid_url_post_id = (int) get_post_meta( $post->ID, '_amp_validated_url_post_id', true );

$validity = self::validate_url_and_store( $url, $invalid_url_post_id );

// Remember the amp_validated_url post so that when the slug changes the old amp_validated_url post can be updated.
if ( ! is_wp_error( $validity ) && $invalid_url_post_id !== $validity['post_id'] ) {
update_post_meta( $post->ID, '_amp_validated_url_post_id', $validity['post_id'] );
}

$validation_posts[ $post->ID ] = $validity instanceof WP_Error ? $validity : $validity['post_id'];
}

return $validation_posts;
_deprecated_function( __METHOD__, '2.1' );
}

/**
Expand Down Expand Up @@ -754,101 +684,12 @@ public static function reset_validation_results() {
*
* This is essentially a PHP implementation of ampBlockValidation.handleValidationErrorsStateChange() in JS.
*
* @param WP_Post $post The updated post.
* @deprecated In 2.1 the classic editor block validation was removed.
* @codeCoverageIgnore
* @return void
*/
public static function print_edit_form_validation_status( $post ) {
if ( ! self::post_supports_validation( $post ) || ! self::get_dev_tools_user_access()->is_user_enabled() ) {
return;
}

$invalid_url_post = AMP_Validated_URL_Post_Type::get_invalid_url_post( get_permalink( $post->ID ) );
if ( ! $invalid_url_post ) {
return;
}

// Show all validation errors which have not been explicitly acknowledged as accepted.
$validation_errors = [];
$has_rejected_error = false;
foreach ( AMP_Validated_URL_Post_Type::get_invalid_url_validation_errors( $invalid_url_post ) as $error ) {
$needs_moderation = (
AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS === $error['status'] || // @todo Show differently since moderated?
AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS === $error['status'] ||
AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS === $error['status']
);
if ( $needs_moderation ) {
$validation_errors[] = $error['data'];
}

if (
AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_REJECTED_STATUS === $error['status']
||
AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACK_REJECTED_STATUS === $error['status']
) {
$has_rejected_error = true;
}
}

// No validation errors so abort.
if ( empty( $validation_errors ) ) {
return;
}

echo '<div class="notice notice-warning">';
echo '<p>';
esc_html_e( 'There is content which fails AMP validation.', 'amp' );
echo ' ';

// Auto-acceptance is enabled by default but can be overridden by the the `amp_validation_error_default_sanitized` filter.
if ( ! $has_rejected_error ) {
esc_html_e( 'The invalid markup has been automatically removed.', 'amp' );
} else {
/*
* Even if invalid markup is removed by default, if there are non-accepted errors in non-Standard mode, it will redirect to a non-AMP page.
* For example, the errors could have been stored as 'New Kept' when auto-accept was false, and now auto-accept is true.
* In that case, this will block serving AMP.
* This could also apply if this is in 'Standard' mode and the user has rejected a validation error.
*/
esc_html_e( 'In order for AMP to be served you will have to remove the invalid markup or allow the plugin to remove it.', 'amp' );
}

echo sprintf(
' <a href="%s" target="_blank">%s</a>',
esc_url( get_edit_post_link( $invalid_url_post ) ),
esc_html__( 'Review issues', 'amp' )
);
echo '</p>';

$results = AMP_Validation_Error_Taxonomy::summarize_validation_errors( array_unique( $validation_errors, SORT_REGULAR ) );
$removed_sets = [];
if ( ! empty( $results[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] ) && is_array( $results[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] ) ) {
$removed_sets[] = [
'label' => __( 'Invalid elements:', 'amp' ),
'names' => array_map( 'sanitize_key', $results[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] ),
];
}
if ( ! empty( $results[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ) && is_array( $results[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ) ) {
$removed_sets[] = [
'label' => __( 'Invalid attributes:', 'amp' ),
'names' => array_map( 'sanitize_key', $results[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ),
];
}
// @todo There are other kinds of errors other than REMOVED_ELEMENTS and REMOVED_ATTRIBUTES.
foreach ( $removed_sets as $removed_set ) {
printf( '<p>%s ', esc_html( $removed_set['label'] ) );
$items = [];
foreach ( $removed_set['names'] as $name => $count ) {
if ( 1 === (int) $count ) {
$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
} else {
$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
}
}
echo implode( ', ', $items ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
echo '</p>';
}

echo '</div>';
public static function print_edit_form_validation_status() {
_deprecated_function( __METHOD__, '2.1' );
}

/**
Expand Down Expand Up @@ -2235,6 +2076,7 @@ public static function get_validate_url_error_message( $error_code, $error_messa
/**
* On activating a plugin, display a notice if a plugin causes an AMP validation error.
*
* @todo Eliminate this in favor of async validation. See <https://github.com/ampproject/amp-wp/issues/5101>.
* @return void
*/
public static function print_plugin_notice() {
Expand Down
138 changes: 0 additions & 138 deletions tests/php/validation/test-class-amp-validation-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,8 @@ public function test_init() {
$this->assertTrue( taxonomy_exists( AMP_Validation_Error_Taxonomy::TAXONOMY_SLUG ) );

$this->assertEquals( 100, has_filter( 'map_meta_cap', self::TESTED_CLASS . '::map_meta_cap' ) );
$this->assertEquals( 10, has_action( 'save_post', self::TESTED_CLASS . '::handle_save_post_prompting_validation' ) );
$this->assertEquals( 10, has_action( 'enqueue_block_editor_assets', self::TESTED_CLASS . '::enqueue_block_validation' ) );

$this->assertEquals( 10, has_action( 'edit_form_top', self::TESTED_CLASS . '::print_edit_form_validation_status' ) );
$this->assertEquals( 10, has_action( 'all_admin_notices', self::TESTED_CLASS . '::print_plugin_notice' ) );

$this->assertEquals( 101, has_action( 'admin_bar_menu', [ self::TESTED_CLASS, 'add_admin_bar_menu_items' ] ) );
Expand Down Expand Up @@ -475,76 +473,6 @@ public function test_add_validation_error_sourcing_gutenberg() {
$this->assertEquals( $priority - 1, has_filter( 'the_content', [ self::TESTED_CLASS, 'add_block_source_comments' ] ) );
}

/**
* Tests handle_save_post_prompting_validation.
*
* @covers AMP_Validation_Manager::handle_save_post_prompting_validation()
* @covers AMP_Validation_Manager::validate_queued_posts_on_frontend()
*/
public function test_handle_save_post_prompting_validation_and_validate_queued_posts_on_frontend() {
$admin_user_id = self::factory()->user->create( [ 'role' => 'administrator' ] );
$editor_user_id = self::factory()->user->create( [ 'role' => 'editor' ] );

wp_set_current_user( $admin_user_id );
$service = $this->injector->make( UserAccess::class );

AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
$_SERVER['REQUEST_METHOD'] = 'POST';
$GLOBALS['pagenow'] = 'post.php';

register_post_type( 'secret', [ 'public' => false ] );
$secret = self::factory()->post->create_and_get( [ 'post_type' => 'secret' ] );
$_POST['post_ID'] = $secret->ID;
AMP_Validation_Manager::handle_save_post_prompting_validation( $secret->ID );
$this->assertFalse( has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );
$this->assertEmpty( AMP_Validation_Manager::validate_queued_posts_on_frontend() );

$auto_draft = self::factory()->post->create_and_get( [ 'post_status' => 'auto-draft' ] );
$_POST['post_ID'] = $auto_draft->ID;
AMP_Validation_Manager::handle_save_post_prompting_validation( $auto_draft->ID );
$this->assertFalse( has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );
$this->assertEmpty( AMP_Validation_Manager::validate_queued_posts_on_frontend() );

// Testing without $_POST context.
$post = self::factory()->post->create_and_get( [ 'post_type' => 'post' ] );
AMP_Validation_Manager::handle_save_post_prompting_validation( $post->ID );
$this->assertFalse( has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );

// Test when user doesn't have the capability.
wp_set_current_user( $editor_user_id );
$post = self::factory()->post->create_and_get( [ 'post_type' => 'post' ] );
AMP_Validation_Manager::handle_save_post_prompting_validation( $post->ID );
$this->assertFalse( has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );

// Test when user has dev tools turned off.
wp_set_current_user( $admin_user_id );
$service->set_user_enabled( $admin_user_id, false );
$post = self::factory()->post->create_and_get( [ 'post_type' => 'post' ] );
$_POST['post_ID'] = $post->ID;
AMP_Validation_Manager::handle_save_post_prompting_validation( $post->ID );
$this->assertFalse( has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );

// Test success.
$service->set_user_enabled( $admin_user_id, true );
wp_set_current_user( $admin_user_id );
$post = self::factory()->post->create_and_get( [ 'post_type' => 'post' ] );
$_POST['post_ID'] = $post->ID;
AMP_Validation_Manager::handle_save_post_prompting_validation( $post->ID );
$this->assertEquals( 10, has_action( 'shutdown', [ 'AMP_Validation_Manager', 'validate_queued_posts_on_frontend' ] ) );

add_filter(
'pre_http_request',
static function() {
return new WP_Error( 'http_request_made' );
}
);
$results = AMP_Validation_Manager::validate_queued_posts_on_frontend();
$this->assertArrayHasKey( $post->ID, $results );
$this->assertInstanceOf( 'WP_Error', $results[ $post->ID ] );

unset( $GLOBALS['pagenow'] );
}

/**
* Test map_meta_cap.
*
Expand Down Expand Up @@ -688,72 +616,6 @@ public function test_reset_validation_results() {
$this->assertEquals( [], AMP_Validation_Manager::$validation_results );
}

/**
* Test print_edit_form_validation_status
*
* @covers AMP_Validation_Manager::print_edit_form_validation_status()
*/
public function test_print_edit_form_validation_status() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
$this->accept_sanitization_by_default( false );

AMP_Validated_URL_Post_Type::register();
AMP_Validation_Error_Taxonomy::register();
$this->set_capability();
$post = self::factory()->post->create_and_get();
$output = get_echo( [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ], [ $post ] );

$this->assertStringNotContains( 'notice notice-warning', $output );

$validation_errors = [
[
'code' => AMP_Tag_And_Attribute_Sanitizer::DISALLOWED_TAG,
'node_name' => $this->disallowed_tag_name,
'parent_name' => 'div',
'node_attributes' => [],
'sources' => [
[
'type' => 'plugin',
'name' => $this->plugin_name,
],
],
],
];

AMP_Validated_URL_Post_Type::store_validation_errors( $validation_errors, get_permalink( $post->ID ) );
$output = get_echo( [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ], [ $post ] );

// When sanitization is accepted by default.
$this->accept_sanitization_by_default( true );
$expected_notice_non_accepted_errors = 'There is content which fails AMP validation. In order for AMP to be served you will have to remove the invalid markup or allow the plugin to remove it.';
$this->assertStringContains( 'notice notice-warning', $output );
$this->assertStringContains( '<code>script</code>', $output );
$this->assertStringContains( $expected_notice_non_accepted_errors, $output );

// When auto-accepting validation errors, if there are unaccepted validation errors, there should be a notice because this will block serving an AMP document.
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
$output = get_echo( [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ], [ $post ] );
$this->assertStringContains( 'There is content which fails AMP validation. In order for AMP to be served you will have to remove the invalid markup or allow the plugin to remove it.', $output );

/*
* When there are 'Rejected' or 'New Rejected' errors, there should be a message that explains that this will serve a non-AMP URL.
* This simulates sanitization being accepted by default, but it having been false when the validation errors were stored,
* as there are errors with 'New Rejected' status.
*/
$this->accept_sanitization_by_default( true );
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
AMP_Validated_URL_Post_Type::store_validation_errors( $validation_errors, get_permalink( $post->ID ) );
$this->accept_sanitization_by_default( false );
$output = get_echo( [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ], [ $post ] );
$this->assertStringContains( $expected_notice_non_accepted_errors, $output );

// Ensure not displayed when dev tools is disabled.
$service = $this->injector->make( UserAccess::class );
$service->set_user_enabled( wp_get_current_user()->ID, false );
$output = get_echo( [ 'AMP_Validation_Manager', 'print_edit_form_validation_status' ], [ $post ] );
$this->assertStringNotContains( 'notice notice-warning', $output );
}

/**
* Test source comments.
*
Expand Down