Report removing unrecognized elements. #1287
Conversation
Closes #1100. Fixes the problem of silently removing unrecognized elements. Uses the same code pattern and methodology as `remove_invalid_child`: - Prevent double-reporting nodes that are rejected. - Check through should_sanitize_validation_error(). - Replace if true. - Else, store in the `should_not_replace_nodes` property.
hellofromtonya
changed the title
| * | ||
| * @param DOMNode $node Node. | ||
| */ | ||
| private function replace_node_with_children( $node ) { | ||
|
|
||
| // If node has no children or no parent, just remove the node. | ||
| if ( ! $node->hasChildNodes() || ! $node->parentNode ) { |
There was a problem hiding this comment.
Should not there be a $this->should_sanitize_validation_error( array(), compact( 'node' ) ) call in here as well? Otherwise the removal won't be reported as a validation error.
There was a problem hiding this comment.
@westonruter the if code block flows through to remove_node() where it’s handled. I did confirm it there. When I write the error unit tests, I’ll validate that too to ensure we have reporting for both paths.
There was a problem hiding this comment.
Ah, you're right. \AMP_Tag_And_Attribute_Sanitizer::remove_node() will call \AMP_Base_Sanitizer::remove_invalid_child() which reports the validation error. 👍
These tests check that invalid nodes do pop validation errors.
|
@westonruter I've added tests to ensure invalid elements with and without child nodes do get the right validation errors. Check out commit 65c9cde. |
westonruter
changed the title
|
Good work! Great to have this as part of 1.0. This will help a lot when someone tries to use an AMP component that is misspelled or tries to use a new AMP component that is not yet recognized due to the spec not being updated. |
Fixes the problem of silently removing unrecognized elements. Uses the same code pattern and methodology as
remove_invalid_child:should_not_replace_nodesproperty.Closes #1100.
See this comment where the issue was recreated.