Skip to content
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

Fix dispatch_key handling for NAME_VALUE_DISPATCH; add cdata validation #926

Merged
merged 5 commits into from Feb 4, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

commented Feb 3, 2018

This fixes the longstanding annoyance for when the spec was read, the one value had to be removed for data-multi-size or else unit tests would fail. It turns out to be due to the dispatch_key not being supported. So this is now fixed and the list of tags and attributes can be re-generated at will.

In addition to dispatch_key being included in the generated PHP file, so too cdata is now included, with validation for blacklisted regex and amp-keyframes styles. This is needed for validating elements in the head. See #926.

@westonruter westonruter added this to the v0.7 milestone Feb 3, 2018

@westonruter westonruter requested review from ThierryA and kienstra Feb 3, 2018

@ThierryA
Copy link
Collaborator

left a 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 ) {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Feb 4, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@ThierryA

ThierryA Feb 4, 2018

Collaborator

Would be good to add valid PHPDoc with @SInCE tags for these constances.

@@ -88,6 +88,13 @@ public function collect_style_elements() {
continue;
}
if ( 'body' === $style_element->parentNode->nodeName && $style_element->hasAttribute( 'amp-keyframes' ) ) {
$validity = $this->validate_amp_keyframe( $style_element );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Feb 4, 2018

Collaborator

I guess this doesn't really have to be stored in a var and could part of the conditional statement above.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 5, 2018

Author Member

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.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

Thanks, I'll address these in the next PR.

@westonruter westonruter merged commit f2c38bd into develop Feb 4, 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/spec-dispatch-key-handling branch Feb 4, 2018

@westonruter westonruter referenced this pull request Feb 4, 2018

Merged

Add full document sanitization in theme support #929

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.