-
Notifications
You must be signed in to change notification settings - Fork 381
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 missing error titles for error codes #4405
Add missing error titles for error codes #4405
Conversation
@pierlon Thanks for opening this draft PR. However, I am not sure this is the right approach for the immediate term (v1.5). Namely, the error codes that we are using are not 1:1 mappings with the error codes used by the AMP validator. There are many error codes that we don't use at all (e.g. I think the initial target to solve #1420 should be less ambitious. And that is to extend what we have today in So I think the patch should largely be limited to making changes to this method: amp-wp/includes/validation/class-amp-validation-error-taxonomy.php Lines 2978 to 3136 in 15fb873
|
This reverts commit 4294c39
Ah OK, I'm changing the direction of this PR from automating the extraction of the error codes to identifying the missing error titles for error codes and adding them. |
@@ -993,14 +993,16 @@ private function validate_tag_spec_for_node( DOMElement $node, $tag_spec ) { | |||
|
|||
if ( ! empty( $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ] ) && ! $this->has_parent( $node, $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ] ) ) { | |||
return [ | |||
'code' => self::WRONG_PARENT_TAG, | |||
'code' => self::WRONG_PARENT_TAG, | |||
'required_parent_name' => $tag_spec[ AMP_Rule_Spec::MANDATORY_PARENT ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that at some point we will not need to capture the required_parent_name
in the error. We could instead capture the spec_name
alone. Then the spec_name
could be looked up to obtain the required_parent_name
(or other properties depending on the error). This will be made possible by #3817. However, that is not yet available, so what you are doing here is the perfect approach in the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I initially was retrieving it by looking up the tag spec in AMP_Allowed_Tags_Generated::class
, but I realized it was available in the sanitizer so I took it from there.
This will be made possible by #3817.
Awesome, made a note of that.
|
||
case AMP_Tag_And_Attribute_Sanitizer::INVALID_URL_PROTOCOL: | ||
$parsed_url = wp_parse_url( $validation_error['element_attributes'][ $validation_error['node_name'] ] ); | ||
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to do here if the scheme could not be parsed, defaulting to an empty string for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe null
would be more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, it's used in a string. Yeah, an empty string seems fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since code
is now being used as opposed to quotes, then I think something else is needed:
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] : ''; | |
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] . ':' : '(failure)'; |
@@ -27,7 +27,6 @@ | |||
class AMP_Tag_And_Attribute_Sanitizer extends AMP_Base_Sanitizer { | |||
|
|||
const DISALLOWED_TAG = 'DISALLOWED_TAG'; | |||
const DISALLOWED_TAG_MULTIPLE_CHOICES = 'DISALLOWED_TAG_MULTIPLE_CHOICES'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've went ahead and removed the DISALLOWED_TAG_MULTIPLE_CHOICES
error code as I don't see a meaningful way of displaying the multiple validation errors on the page. Instead, each error will be sanitized separately which allows for each to be viewed.
unset( $validation_error['spec_name'] ); | ||
unset( | ||
$validation_error['spec_name'], | ||
// Remove other keys that may not not make the error unique. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing any other keys that could potentially make the validation error unique (but it should not). Take for example this HTML element:
<stop offset="5%" stop-color="gold" />
This would raise two separate MANDATORY_TAG_ANCESTOR
validation errors, but each would have its own required_ancestor_name
(lineargradient
and radialgradient
), making each validation error unique.
|
||
case AMP_Tag_And_Attribute_Sanitizer::INVALID_URL_PROTOCOL: | ||
$parsed_url = wp_parse_url( $validation_error['element_attributes'][ $validation_error['node_name'] ] ); | ||
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since code
is now being used as opposed to quotes, then I think something else is needed:
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] : ''; | |
$invalid_protocol = isset( $parsed_url['scheme'] ) ? $parsed_url['scheme'] . ':' : '(failure)'; |
Co-authored-by: Weston Ruter <westonruter@google.com>
…node This prevents a validation error from having no source stack
$this->remove_invalid_child( | ||
$node, | ||
[ | ||
'code' => self::DISALLOWED_TAG_MULTIPLE_CHOICES, | ||
'errors' => $validation_errors, | ||
] | ||
); | ||
foreach ( $validation_errors as $validation_error ) { | ||
if ( true === $this->remove_invalid_child( $node, $validation_error ) ) { | ||
break; // Once removed, ignore remaining errors. | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given a Custom HTML block with the following markup:
<amp-accordion id="my-accordion" disable-session-states="">
<section>
<h2>Section 1</h2>
<p>Content in section 12.</p>
</section>
<section>
<h2>Section 2</h2>
<div>Content in section 2.</div>
</section>
<section expanded="">
<!--<h2>Section 3</h2>-->
<amp-img src="/static/inline-examples/images/squirrel.jpg" width="320" height="256"></amp-img>
</section>
</amp-accordion>
This is invalid because of the h2
being commented out. The amp-accordion
component requires an h2
followed by a div
or a p
. The validation errors for this are not expected, however:
Notice the amp-img
lacks the expected Custom HTML source.
I've fixed this here in feababc by breaking the loop of validation error reporting once a markup causing validation error is removed.
This will probably make this code no longer necessary:
amp-wp/includes/sanitizers/class-amp-base-sanitizer.php
Lines 477 to 481 in 3c29860
if ( null === $node->parentNode ) { | |
// Node no longer exists. | |
return $should_remove; | |
} | |
Though it doesn't hurt to have it in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice the
amp-img
lacks the expected Custom HTML source.
Good catch!
This will probably make this code no longer necessary:
In this context yes, but it could potentially prevent an error being thrown when attempting to remove a node multiple times.
…nt/1420-expose-validation-errors * 'develop' of github.com:ampproject/amp-wp: (39 commits) Fix CachedRemoteGetRequest handling for failures Avoid max-age falling through to 1-month expiry Use wp_remote_get instead of private _wp_http_get_object Change expiry behavior regarding enforced minimum and failed requests Replace B with bytes abbr in summary table Add space before B and add abbr Update lables for stylesheet informatino panel Update dependency @wordpress/block-editor to v3.7.5 (#4393) Update dependency @wordpress/edit-post to v3.13.6 (#4395) Update dependency @wordpress/components to v9.2.4 (#4394) Update dependency browserslist to v4.10.0 (#4406) Show correct PHP requirement message to users Make use of headers more robust and add test Only do iterator_to_array() if Traversable Use static variable for computed constant string Improve phpdoc for Fonts Use iterator_to_array() instead of casting to array Cast Requests_Utility_CaseInsensitiveDictionary to an array Use WP_Http remote requests for external stylesheet fetching Use WP_Http remote requests for the optimizer ...
Summary
This PR adds the missing error titles (and appropriate labels for error sources) for the error codes which have none.
Fixes #1420
Todo
Checklist