Update spec generation to file revision 712 (amphtml tag 1533692711873) #1315
Conversation
There was a problem hiding this comment.
@westonruter I just ran the update spec generator. It performed as expected. It did add the new updates from the new specification, including allowing the amp-lightbox animation attributions to pass through the sanitizer.
As we discussed, we should also update the amphtml-update.py instructions, i.e. to point it to the Updating Allowed Tags and Attributes section. Done in d576d44.
| if ( $node->hasAttribute( $attr_name ) ) { | ||
| $attr_value = strtolower( $node->getAttribute( $attr_name ) ); | ||
| if ( $attr_value === (string) $rule_value ) { | ||
| return AMP_Rule_Spec::PASS; | ||
| } else { | ||
| return AMP_Rule_Spec::FAIL; | ||
| $result = AMP_Rule_Spec::FAIL; |
There was a problem hiding this comment.
@westonruter Why are we allowing the rule iteration to continue if it fails? With this approach, it could fail on one of the values, but then pass on another. When it passes, it would return a pass and bail out, even though it failed previously. Is this what we want here?
There was a problem hiding this comment.
Good question. The reason is that the spec changed from value_casei being a scalar value to being an array. So instead of testing a single value, we now need to check if any match. You can see in the if condition immediately preceding that it will return immediately upon a match. But then if it does not match then this FAIL will end up being the return value after all of the values are matched. But if the element doesn't have the attribute at all, then the NOT_APPLICABLE value is returned.
There was a problem hiding this comment.
@westonruter Thank you for clarifying. That makes sense.
| @@ -1099,13 +1122,13 @@ private function check_attr_spec_rule_value_casei( $node, $attr_name, $attr_spec | |||
| if ( $attr_value === (string) $rule_value ) { | |||
| return AMP_Rule_Spec::PASS; | |||
| } else { | |||
| return AMP_Rule_Spec::FAIL; | |||
| $result = AMP_Rule_Spec::FAIL; | |||
There was a problem hiding this comment.
Same quest here as above.
There was a problem hiding this comment.
Also, this change was required to get unit tests to pass, as I recall.
There was a problem hiding this comment.
@westonruter What you have presently looks good. Ping me when you the other TODOs ready for review.
westonruter
changed the title
westonruter
dismissed
hellofromtonya’s stale review
Changes have been completed
westonruter
changed the title
|
Moving To 'Ready For Merging' If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing. Feel free to move it back. |
This is a follow-up on #1312. Since there are validator spec changes another PR was needed to deal with them.
amp-embedly-cardandamp-embedly-key.minimum-nightsattribute toamp-date-picker.animate-inattribute toamp-lightbox.submittingandverify-errortoform.amp-fxvalues.Fixes #1330.