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

Report removing unrecognized elements. #1287

Merged
merged 2 commits into from Jul 28, 2018

Conversation

Projects
None yet
2 participants
@hellofromtonya
Collaborator

hellofromtonya commented Jul 25, 2018

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.

Closes #1100.

See this comment where the issue was recreated.

Remove unrecognized elements and report.
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 hellofromtonya requested a review from westonruter Jul 25, 2018

@hellofromtonya hellofromtonya changed the title from [WIP] Remove unrecognized elements and report. to [WIP] Report removing unrecognized elements. Jul 25, 2018

@westonruter

Looking good so far, other than the condition of when there is no parent node and no child nodes. This should also get unit tests to check for the new scenarios for when validation errors can be reported for the various removals of invalid elements.

*
* @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 ) {

This comment has been minimized.

@westonruter

westonruter Jul 26, 2018

Member

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.

This comment has been minimized.

@hellofromtonya

hellofromtonya Jul 26, 2018

Collaborator

@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.

This comment has been minimized.

@westonruter

westonruter Jul 26, 2018

Member

Ah, you're right. \AMP_Tag_And_Attribute_Sanitizer::remove_node() will call \AMP_Base_Sanitizer::remove_invalid_child() which reports the validation error. 👍

Test replace_node_with_children validation errors.
These tests check that invalid nodes do pop validation errors.
@hellofromtonya

This comment has been minimized.

Collaborator

hellofromtonya commented Jul 27, 2018

@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 westonruter changed the title from [WIP] Report removing unrecognized elements. to Report removing unrecognized elements. Jul 28, 2018

@westonruter westonruter merged commit 6be1d84 into develop Jul 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/silently-removing-unrecognized-elements branch Jul 28, 2018

@westonruter

This comment has been minimized.

Member

westonruter commented Jul 28, 2018

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.

@westonruter westonruter added this to the v1.0 milestone Jul 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment