Fix dispatch_key handling for NAME_VALUE_DISPATCH; add cdata validation#926
Merged
westonruter merged 5 commits intodevelopfrom Feb 4, 2018
Merged
Fix dispatch_key handling for NAME_VALUE_DISPATCH; add cdata validation#926westonruter merged 5 commits intodevelopfrom
westonruter merged 5 commits intodevelopfrom
Conversation
…ata-multi-size=''] issue h/t @Gregable
ThierryA
approved these changes
Feb 4, 2018
Collaborator
ThierryA
left a comment
There was a problem hiding this comment.
Thanks @westonruter, I added minor comments but this is good to go.
| * @return true|WP_Error Validity. | ||
| */ | ||
| private function validate_amp_keyframe( $style ) { | ||
| if ( strlen( $style->textContent ) > 500000 ) { |
Collaborator
There was a problem hiding this comment.
Maybe better to store 500000 in constant.
| * https://github.com/ampproject/amphtml/blob/eda1daa8c40f830207edc8d8088332b32a15c1a4/validator/validator.proto#L111-L120 | ||
| */ | ||
|
|
||
| // Indicates that the attribute does not form a dispatch key. |
Collaborator
There was a problem hiding this comment.
Would be good to add valid PHPDoc with @SInCE tags for these constances.
| } | ||
|
|
||
| if ( 'body' === $style_element->parentNode->nodeName && $style_element->hasAttribute( 'amp-keyframes' ) ) { | ||
| $validity = $this->validate_amp_keyframe( $style_element ); |
Collaborator
There was a problem hiding this comment.
I guess this doesn't really have to be stored in a var and could part of the conditional statement above.
Member
Author
There was a problem hiding this comment.
The reason behind is is that in #912 we need to add reporting and $validity is a WP_Error instance, so we'd want to report it when this is merged into that PR.
Member
Author
|
Thanks, I'll address these in the next PR. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the longstanding annoyance for when the spec was read, the one
valuehad to be removed fordata-multi-sizeor else unit tests would fail. It turns out to be due to thedispatch_keynot being supported. So this is now fixed and the list of tags and attributes can be re-generated at will.In addition to
dispatch_keybeing included in the generated PHP file, so toocdatais now included, with validation for blacklisted regex and amp-keyframes styles. This is needed for validating elements in thehead. See #926.