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

Add support for comments #871

Closed
wants to merge 39 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@DavidCramer
Contributor

DavidCramer commented Jan 17, 2018

See #797, #862.

@DavidCramer DavidCramer requested review from westonruter and ThierryA Jan 17, 2018

Show outdated Hide outdated includes/sanitizers/class-amp-form-sanitizer.php Outdated
Show outdated Hide outdated includes/sanitizers/class-amp-form-sanitizer.php Outdated
Show outdated Hide outdated includes/sanitizers/class-amp-form-sanitizer.php Outdated
Show outdated Hide outdated includes/sanitizers/class-amp-form-sanitizer.php Outdated
Show outdated Hide outdated includes/sanitizers/class-amp-form-sanitizer.php Outdated
Show outdated Hide outdated includes/utils/class-amp-dom-utils.php Outdated
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jan 17, 2018

Collaborator

For handling form method=POST submissions I think we can pretty cleanly handle them via https://www.ampproject.org/docs/reference/components/amp-form#redirecting-after-a-submission

Usually a POST request should be accompanied by a redirect. So we should be able to handle that via:

add_filter( 'wp_redirect', function( $location ) {
    $location = wp_sanitize_redirect( $location );
    header( "AMP-Redirect-To: $location" );
    header( "Access-Control-Expose-Headers: AMP-Redirect-To", false );
    return false; // Prevent header( 'Location:' ). I'm not sure this is necessary, or if we can just return $location.
}, 1000 );

Some more examples in general I've found at https://ampbyexample.com/components/amp-form/

Collaborator

westonruter commented Jan 17, 2018

For handling form method=POST submissions I think we can pretty cleanly handle them via https://www.ampproject.org/docs/reference/components/amp-form#redirecting-after-a-submission

Usually a POST request should be accompanied by a redirect. So we should be able to handle that via:

add_filter( 'wp_redirect', function( $location ) {
    $location = wp_sanitize_redirect( $location );
    header( "AMP-Redirect-To: $location" );
    header( "Access-Control-Expose-Headers: AMP-Redirect-To", false );
    return false; // Prevent header( 'Location:' ). I'm not sure this is necessary, or if we can just return $location.
}, 1000 );

Some more examples in general I've found at https://ampbyexample.com/components/amp-form/

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jan 19, 2018

Collaborator

@DavidCramer FYI: There won't need to be a get_scripts() method defined in the form sanitizer once #882 is merged.

Collaborator

westonruter commented Jan 19, 2018

@DavidCramer FYI: There won't need to be a get_scripts() method defined in the form sanitizer once #882 is merged.

@DavidCramer DavidCramer changed the title from [WIP] - Add form sanitization for comment submissions to [WIP] - Add support for comments Jan 19, 2018

@ThierryA

This comment has been minimized.

Show comment
Hide comment
@ThierryA

ThierryA Jan 19, 2018

Collaborator

@DavidCramer #882 was just merged to develop so you can apply @westonruter suggestion here.

Collaborator

ThierryA commented Jan 19, 2018

@DavidCramer #882 was just merged to develop so you can apply @westonruter suggestion here.

DavidCramer added some commits Jan 22, 2018

}
return 'element';
}

This comment has been minimized.

@DavidCramer

DavidCramer Jan 22, 2018

Contributor

@westonruter component scripts are have custom-element in the script tags, however amp-mustache used for amp-list does not, instead it has custom-template. see https://www.ampproject.org/docs/reference/components/amp-mustache
It looks like this is the only component that's different, however I separated it to its own method in case the spec changes, or additionals are added in future.

@DavidCramer

DavidCramer Jan 22, 2018

Contributor

@westonruter component scripts are have custom-element in the script tags, however amp-mustache used for amp-list does not, instead it has custom-template. see https://www.ampproject.org/docs/reference/components/amp-mustache
It looks like this is the only component that's different, however I separated it to its own method in case the spec changes, or additionals are added in future.

This comment has been minimized.

@westonruter

westonruter Jan 23, 2018

Collaborator

I don't think get_component_type would be needed if we sanitize the entire body, right?

@westonruter

westonruter Jan 23, 2018

Collaborator

I don't think get_component_type would be needed if we sanitize the entire body, right?

This comment has been minimized.

@DavidCramer

DavidCramer Jan 23, 2018

Contributor

@westonruter I think we still do, since this is done after the sanitisation and the scripts are placed in. All the scripts are custom-element but the template is custom-template.

@DavidCramer

DavidCramer Jan 23, 2018

Contributor

@westonruter I think we still do, since this is done after the sanitisation and the scripts are placed in. All the scripts are custom-element but the template is custom-template.

@DavidCramer

This comment has been minimized.

Show comment
Hide comment
@DavidCramer

DavidCramer Jan 22, 2018

Contributor

@westonruter Regarding moving the sanitisation from the_content to the output buffer: I needed this in order to have the comments to work, which I have done in my dev, but have not added to this PR. Is that still under discussion or could I do a PR on it based on what I've already got?

Contributor

DavidCramer commented Jan 22, 2018

@westonruter Regarding moving the sanitisation from the_content to the output buffer: I needed this in order to have the comments to work, which I have done in my dev, but have not added to this PR. Is that still under discussion or could I do a PR on it based on what I've already got?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jan 23, 2018

Collaborator

@DavidCramer we'll go ahead and sanitize the entire body, or actually, the entire html document. I was also going to work on a PR, so if you can get one up first that is good as well.

Collaborator

westonruter commented Jan 23, 2018

@DavidCramer we'll go ahead and sanitize the entire body, or actually, the entire html document. I was also going to work on a PR, so if you can get one up first that is good as well.

@DavidCramer DavidCramer changed the title from [WIP] - Add support for comments to Add support for comments Jan 24, 2018

@DavidCramer

This comment has been minimized.

Show comment
Hide comment
@DavidCramer

DavidCramer Jan 25, 2018

Contributor

@westonruter You can start reviewing this, but there is an issue with the url template strings I need to fix. I'm needing to head off now, but will finish when I'm back. If not by the time you get on, I'll sort it in the morning.

Contributor

DavidCramer commented Jan 25, 2018

@westonruter You can start reviewing this, but there is an issue with the url template strings I need to fix. I'm needing to head off now, but will finish when I'm back. If not by the time you get on, I'll sort it in the morning.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jan 25, 2018

Collaborator

there is an issue with the url template strings I need to fix

Shoot. This is a side effect of switching from saveXML to saveHTML in #891.

Similar to what was done in #895 with AMP_DOM_Utils::convert_amp_bind_attributes(), we need to do a temporary replacement of amp-mustache tokens during parsing and serialization, then swap them back after. I'm working on the change.

Collaborator

westonruter commented Jan 25, 2018

there is an issue with the url template strings I need to fix

Shoot. This is a side effect of switching from saveXML to saveHTML in #891.

Similar to what was done in #895 with AMP_DOM_Utils::convert_amp_bind_attributes(), we need to do a temporary replacement of amp-mustache tokens during parsing and serialization, then swap them back after. I'm working on the change.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jan 26, 2018

Collaborator

We talked it over and we decided to try the route of amp-live-list instead and see how it goes. 🎉

Collaborator

westonruter commented Jan 26, 2018

We talked it over and we decided to try the route of amp-live-list instead and see how it goes. 🎉

@westonruter westonruter deleted the add/862-support-comment-submissions branch Jul 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment