Fix DOM exception "Not Found Error" when cleaning up after invalid attribute removal

[westonruter]

Summary

Defer cleanup removal of invalid attributes until determining if they are actually removed. It turns out that calling \AMP_Base_Sanitizer::clean_up_after_attribute_removal() inside of \AMP_Base_Sanitizer::remove_invalid_attribute() can cause subsequent calls to \AMP_Base_Sanitizer::remove_invalid_attribute() to throw a DOMException “Not Found Error” if the former removed an attribute that needs to actually be removed by the latter.

For example, if there is a <a href="…" target="…"> element, and both href and target are causing validation errors, then if the target attribute gets cleaned up at the same time as href is removed, then when target tries to get removed as well, it will cause an error because it no longer is in the DOM.

Fixes issues reported on support forum:

• https://wordpress.org/support/topic/php-fatal-error-uncaught-domexception-not-found-error-in/
• https://wordpress.org/support/topic/page-unable-to-reach/

Regression introduced 1.3.0 via #3151.

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

[kienstra]

Approved

Hi @westonruter,
This looks good. It prevents the error, as you mentioned.

Before

On adding Weston's test snippet to a Custom HTML block and saving:

<a href=“%E2%80%9Chttps://example.com/path/to/post/%E2%80%9D“ target=“_blank“ rel=“noopener“>Whatever</a>

...there was a fatal error at:

amp-wp/includes/sanitizers/class-amp-base-sanitizer.php

Line 489 in 24a8e48

$element->removeAttributeNode( $node );

Error log:

$ tail -f /path/to/log/php7.2_errors.log
#2 /path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php(304): AMP_Tag_And_Attribute_Sanitizer->process_node(Object(DOMElement))
#3 /path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php(287): AMP_Tag_And_Attribute_Sanitizer->sanitize_element(Object(DOMElement))
#4 /path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php(287): AMP_Tag_And_ in path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-base-sanitizer.php on line 489
[07-Nov-2019 18:13:23 UTC] PHP Fatal error:  Uncaught DOMException: Not Found Error in /path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-base-sanitizer.php:489
Stack trace:
#0 /path/to/wp-content/plugins/amp/includes/sanitizers/class-amp-base-sanitizer.php(489): DOMElement->removeAttributeNode(Object(DOMAttr))

There was also an indication of a fatal error in the validator:

After

With this PR, there was no fatal error. Validation worked as expected, without the message shown above.

Also, quickly doing wp amp validation run didn't show any issue.

[westonruter]

I've fleshed out the error message to make request checking the error log in the future when something like this happens again:

[kienstra]

Ah, good idea!

[kienstra]

Testing Steps

1. Create a new post
2. Add a Custom HTML block with Weston's snippet:

<a href=“%E2%80%9Chttps://example.com/path/to/post/%E2%80%9D“ target=“_blank“ rel=“noopener“>Whatever</a>

3. Save
4. There should be a notice of a validation error, click to see the details
5. Expected: The validator screen should display without a 'URL validation failed' notice:

[csossi]

Verified in QA