Skip to content

Fix first_child_tag_name_oneof constraint to only apply if first child exists#2152

Merged
westonruter merged 1 commit intodevelopfrom
fix/first_child_tag_name_oneof
Apr 20, 2019
Merged

Fix first_child_tag_name_oneof constraint to only apply if first child exists#2152
westonruter merged 1 commit intodevelopfrom
fix/first_child_tag_name_oneof

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Apr 20, 2019

As reported in a support forum topic, the following markup is now erroneously marked as a validation error:

<amp-state id="myRemoteState" src="https://example.com/articles.json"></amp-state>

It worked in 1.0 but it regressed in 1.1. The regression is due to #1860. Namely, the first_child_tag_name_oneof validator constraint is incorrectly being implemented by also implying that there has to be a child to begin with. However, this was an incorrect interpretation it appears.

In any case, the constraints to ensure there is a minimum number of tags is implemented later in the method:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 1859 to 1867 in 621a050

// If there aren't the exact number of elements, then mark this $node as being invalid.
if ( isset( $child_tags['mandatory_num_child_tags'] ) ) {
return ( count( $child_elements ) - $removed_count ) === $child_tags['mandatory_num_child_tags'];
}
// If there aren't enough elements, then mark this $node as being invalid.
if ( isset( $child_tags['mandatory_min_num_child_tags'] ) ) {
return ( count( $child_elements ) - $removed_count ) >= $child_tags['mandatory_min_num_child_tags'];
}

Fixing this doesn't cause any tests to fail, and it also causes the amp-state[src] markup to not fail.

Build for testing: amp.zip (1.1.1-alpha-20190420T060818Z-871da341)

@westonruter westonruter added this to the v1.1.1 milestone Apr 20, 2019
@westonruter westonruter requested a review from felixarntz April 20, 2019 06:09
@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 20, 2019
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Apr 20, 2019

Fix confirmed: https://wordpress.org/support/topic/always-error/#post-11449099

@westonruter westonruter merged commit 801fd56 into develop Apr 20, 2019
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Apr 20, 2019

Cherry-picked onto 1.1 branch: 956a391

@westonruter westonruter deleted the fix/first_child_tag_name_oneof branch April 20, 2019 15:21
@westonruter westonruter mentioned this pull request Apr 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixarntz felixarntz felixarntz approved these changes

Assignees

No one assigned

Labels

cla: yes Signed the Google CLA

Projects

None yet

Milestone

v1.1.1

Development

Successfully merging this pull request may close these issues.

3 participants

@westonruter @felixarntz @googlebot