Improve display of validation errors for scripts #3722

westonruter · 2019-11-12T21:07:40Z

Summary

At present the list of validation errors for scripts is not helpful at all because it just says “Invalid Element: <script>” over and over again. You have to expand it to figure out what it is for. We can improve that by having it instead say “Invalid script” and then provide the basename of the script being included.

Validated URL Screen

Before	After

Validation Error Index Screen

Before	After

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

westonruter · 2019-11-12T21:33:36Z

includes/validation/class-amp-validation-error-taxonomy.php

+ $is_js_script = self::is_validation_error_for_js_script_element( $validation_error );
+ if ( self::INVALID_ELEMENT_CODE === $validation_error['code'] && $is_js_script && isset( $validation_error['node_attributes']['src'] ) ) {
+ $content .= sprintf( ': <code>%s</code>', esc_html( basename( wp_parse_url( $validation_error['node_attributes']['src'], PHP_URL_PATH ) ) ) );
+ } elseif ( ( self::INVALID_ELEMENT_CODE === $validation_error['code'] || 'duplicate_element' === $validation_error['code'] ) && ! $is_js_script ) {


Eventually this whole conditional block should be moved to \AMP_Validation_Error_Taxonomy::get_error_title_from_code(), as opposed to checking conditions twice.

Fixed in 96267a8

pierlon

Also,

diff --git a/includes/validation/class-amp-validated-url-post-type.php b/includes/validation/class-amp-validated-url-post-type.php
index bb3ed433..e9e50fd6 100644
--- a/includes/validation/class-amp-validated-url-post-type.php
+++ b/includes/validation/class-amp-validated-url-post-type.php
@@ -1433,7 +1433,7 @@ class AMP_Validated_URL_Post_Type {
                        $heading = sprintf(
                                '%s: <code>%s</code>%s',
                                esc_html( $error_title ),
-                               esc_html( $description['node_name'] ),
+                               esc_html( $validation_error['node_name'] ),
                                wp_kses_post( $status_text )
                        );
                        ?>

includes/validation/class-amp-validated-url-post-type.php

pierlon · 2019-11-12T22:04:55Z

Also, the tag_row_actions filter has been deprecated in WP 5.3 (or was supposed to be from WP 3.0.0). Only one instance of it is being used in the plugin, as found here:

amp-wp/includes/validation/class-amp-validation-error-taxonomy.php

Line 761 in 480a667

 add_filter( 'tag_row_actions', [ __CLASS__, 'filter_tag_row_actions' ], 10, 2 ); 

Changing that to amp_validation_error_row_actions should resolve that issue, along with removing the condition for verifying the taxonomy in AMP_Validation_Error_Taxonomy::filter_tag_row_actions:

amp-wp/includes/validation/class-amp-validation-error-taxonomy.php

Line 1625 in 480a667

if ( self::TAXONOMY_SLUG === $tag->taxonomy ) {

pierlon · 2019-11-12T22:11:25Z

Per #3722 (review), this will show:

I suppose it should be showing the basename of the script here as well, and nothing if its an inline script?

westonruter · 2019-11-12T23:00:25Z

I suppose it should be showing the basename of the script here as well, and nothing if its an inline script?

Yes, good point! I noticed that as well. I just pushed 96267a8 to fix that.

westonruter · 2019-11-13T00:03:00Z

Also, the tag_row_actions filter has been deprecated in WP 5.3 (or was supposed to be from WP 3.0.0).

Good eye. I'm impressed you noticed that. Fixed in e64154d.

pierlon

No issues to report 👍

* Improve display of validation errors for scripts * Fix phpcs issues * Improve validation error summary on index detail page * Eliminate soft-deprecated use of tag_row_actions filter * Eliminate unecessary use of sprintf()

…ve-duplicate-amp-scripts * 'develop' of github.com:ampproject/amp-wp: (66 commits) Improve display of validation errors for scripts (#3722) Conditionally run E2E tests (#3723) Tidy up validation error details (#3721) Add missing space after sentence (#3720) Default to the homepage instead of fetching the first AMP compatible post to customize (#3715) Include text content of style element in validation error (#3717) Fix summarizing error sources both parent theme and child theme (#3709) Exclude WordPress.PHP.DisallowShortTernary phpcs sniff Fix phpcs issues with date() and current_time() Exclude Generic.Arrays.DisallowShortArraySyntax from WordPress-Core Update dependency wp-coding-standards/wpcs to v2.2.0 Improve specificity of JS doc Fix identifying sources for validation errors coming child themes (#3708) Fix failing E2E tests (#3707) Remove amp_validate query var from Validated URL 'View' row action Re-factor get_html_attribute_pattern as match_element_attributes Quote variables added to regex pattern Replace incorrect usage of esc_url() with esc_url_raw() Remove empty alt attributes Add object-fit=contain to amp-youtube placeholder image ...

* tag '1.4.1': (26 commits) Bump 1.4.1 Update screenshots for 1.4.1 Fix expected image name after upstream change (#3749) Use length property instead of count() method on DOMNodeList (#3727) Improve display of validation errors for scripts (#3722) Conditionally run E2E tests (#3723) Tidy up validation error details (#3721) Bump 1.4.1-RC1 Default to the homepage instead of fetching the first AMP compatible post to customize (#3715) Add missing space after sentence (#3720) Include text content of style element in validation error (#3717) Use bitwise operator. Check if element is not in top toolbar. Fix user select for meta date and author Allow right click for meta blocks Fix summarizing error sources both parent theme and child theme (#3709) Fix identifying sources for validation errors coming child themes (#3708) Fix failing E2E tests (#3707) Remove amp_validate query var from Validated URL 'View' row action (#3706) Escape instances of unescapeed output in AMP settings screen code (#3703) ...

westonruter added this to the v1.4.1 milestone Nov 12, 2019

westonruter requested review from kienstra and pierlon November 12, 2019 21:07

googlebot added the cla: yes Signed the Google CLA label Nov 12, 2019

westonruter commented Nov 12, 2019

View reviewed changes

pierlon suggested changes Nov 12, 2019

View reviewed changes

westonruter force-pushed the improve/invalid-script-errors branch from 480a667 to 96267a8 Compare November 12, 2019 22:59

westonruter requested a review from pierlon November 13, 2019 00:03

pierlon approved these changes Nov 13, 2019

View reviewed changes

westonruter added 5 commits November 12, 2019 22:07

Improve display of validation errors for scripts

c1cc8db

Fix phpcs issues

e6f28d2

Improve validation error summary on index detail page

459c64a

Eliminate soft-deprecated use of tag_row_actions filter

be0f90e

Eliminate unecessary use of sprintf()

17d3a0a

westonruter force-pushed the improve/invalid-script-errors branch from 373ff01 to 17d3a0a Compare November 13, 2019 06:07

westonruter merged commit 90c21b1 into develop Nov 13, 2019

westonruter deleted the improve/invalid-script-errors branch November 13, 2019 06:31

csossi added the QA passed Has passed QA and is done label Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve display of validation errors for scripts #3722

Improve display of validation errors for scripts #3722

westonruter commented Nov 12, 2019

westonruter Nov 12, 2019

westonruter Nov 12, 2019

pierlon left a comment

pierlon commented Nov 12, 2019 •

edited

Loading

pierlon commented Nov 12, 2019

westonruter commented Nov 12, 2019 •

edited

Loading

westonruter commented Nov 13, 2019

pierlon left a comment

Improve display of validation errors for scripts #3722

Improve display of validation errors for scripts #3722

Conversation

westonruter commented Nov 12, 2019

Summary

Validated URL Screen

Validation Error Index Screen

Checklist

westonruter Nov 12, 2019

Choose a reason for hiding this comment

westonruter Nov 12, 2019

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

pierlon commented Nov 12, 2019 • edited Loading

pierlon commented Nov 12, 2019

westonruter commented Nov 12, 2019 • edited Loading

westonruter commented Nov 13, 2019

pierlon left a comment

Choose a reason for hiding this comment

pierlon commented Nov 12, 2019 •

edited

Loading

westonruter commented Nov 12, 2019 •

edited

Loading