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

Add support for comments #871

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
7ec60e4
Make add `get_dom` method to AMP_DOM_Utils
DavidCramer Jan 17, 2018
eed214f
add amp_form_sanitizer
DavidCramer Jan 17, 2018
4067b59
add form sanitisation on output buuffer
DavidCramer Jan 17, 2018
a8d2ad3
merge develop in
DavidCramer Jan 18, 2018
98874ba
Merge develop
DavidCramer Jan 19, 2018
4f6dcd6
add comments REST endpoints
DavidCramer Jan 19, 2018
317f90f
Add AMP Comments Walker
DavidCramer Jan 19, 2018
451db91
Add comments template overrides and methods
DavidCramer Jan 19, 2018
0541686
add comment sanitizer
DavidCramer Jan 19, 2018
2c8575e
Make walker add the HTML to reduce load in the sanitiser.
DavidCramer Jan 19, 2018
ac0cc53
its a filter, not an action.
DavidCramer Jan 19, 2018
2741fdf
Merge develop
DavidCramer Jan 22, 2018
ebffaae
add form sanitisation changes
DavidCramer Jan 22, 2018
9790be3
make action setting more sane
DavidCramer Jan 22, 2018
b425b19
Add sanitisation and component_type method
DavidCramer Jan 22, 2018
b7c5963
optimize comments endpoint
DavidCramer Jan 23, 2018
94aee02
wp_unslash request_uri for action attribute.
DavidCramer Jan 23, 2018
d0fc6c5
cleaner comments template generation
DavidCramer Jan 23, 2018
6b660b3
merge develop
DavidCramer Jan 23, 2018
69d1342
cleanup formatting
DavidCramer Jan 24, 2018
4200d9e
output comment template on same comments pass
DavidCramer Jan 24, 2018
366bef3
cleanup comments sanitiser and add url template string filtering
DavidCramer Jan 24, 2018
7b63836
add comment template url string filters
DavidCramer Jan 24, 2018
8b289c1
remove unnecessary duplicated `wp_list_comments` pass
DavidCramer Jan 24, 2018
15d327d
Hook into AMP xhr-submissions to handle headers and JSON responses.
DavidCramer Jan 24, 2018
561189b
add amp-list fallback for comments.
DavidCramer Jan 24, 2018
a1bf8d3
add comment form success and error handlers and wrapper code.
DavidCramer Jan 24, 2018
b40e932
lowercase `post` k, thanks.
DavidCramer Jan 24, 2018
dc1f9c3
Merge develop
DavidCramer Jan 24, 2018
187e136
fix formatting fail.
DavidCramer Jan 24, 2018
82e8021
if no template, move on.
DavidCramer Jan 25, 2018
d4fd8bc
No need to check for the `amp_comments` tag anymore.
DavidCramer Jan 25, 2018
bbabbd7
add tests for form and comment sanitisation.
DavidCramer Jan 25, 2018
0fd1ab5
add docs to tests
DavidCramer Jan 25, 2018
9f99abe
No need to get images again.
DavidCramer Jan 25, 2018
9216e4e
check has images before converting urls
DavidCramer Jan 25, 2018
fafa679
fix phpdoc
DavidCramer Jan 25, 2018
9536794
Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…
westonruter Jan 25, 2018
74ae3b5
Prevent saveHTML from munging amp-mustache curly braces via URL-encoding
westonruter Jan 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions amp.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ function amp_after_setup_theme() {
}

add_action( 'init', 'amp_init' );
add_action( 'rest_api_init', 'amp_rest_init' );
add_action( 'admin_init', 'AMP_Options_Manager::register_settings' );
add_filter( 'amp_post_template_analytics', 'amp_add_custom_analytics' );
add_action( 'wp_loaded', 'amp_post_meta_box' );
Expand Down Expand Up @@ -109,6 +110,16 @@ function amp_init() {
}
}

/**
* Init AMP Rest endpoints.
*
* @since 0.1
*/
function amp_rest_init() {
require_once AMP__DIR__ . '/includes/amp-rest-functions.php';
amp_register_endpoints();
}

// Make sure the `amp` query var has an explicit value.
// Avoids issues when filtering the deprecated `query_string` hook.
function amp_force_query_var_value( $query_vars ) {
Expand Down
69 changes: 69 additions & 0 deletions includes/amp-rest-functions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php
/**
* AMP Rest Functions
*
* @package AMP
*/

/**
* Register the comments REST endpoint.
*
* @since 0.7
*/
function amp_register_endpoints() {

register_rest_route( 'amp/v1', '/comments/(?P<id>\d+)', array(
'methods' => 'GET',
'callback' => 'amp_get_comments',
) );

}

/**
* Get comments for the Post and Parent
*
* @since 0.7
* @param WP_REST_Request $request The REST request.
*
* @return array The array of comments.
*/
function amp_get_comments( $request ) {

$id = $request->get_param( 'id' );
$return = array(
'items' => array(
'comment' => amp_get_comments_recursive( $id, 0 ),
),
);

return $return;
}

/**
* Recursively get comments for the Post and Parent
*
* @since 0.7
* @param int $id The post ID.
* @param int $parent The comment parent.
*
* @return array The array of comments.
*/
function amp_get_comments_recursive( $id, $parent ) {

$comments = get_comments( array(
'post_ID' => $id,
'parent' => $parent,
'order' => 'ASC',
) );

$return = array();
foreach ( $comments as $comment ) {
$GLOBALS['comment'] = $comment; // WPCS: override ok.
$comment->comment_date = get_comment_date();
$comment->comment_time = get_comment_time();
$comment->comment = amp_get_comments_recursive( $id, $comment->comment_ID );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will have performance problems, as it could result in a lot of DB queries. If you look at wp_list_comments() I believe you can see it gets a flat list of all comments for the post, and then it passes it into Walker::paged_walk() which then goes about identifying which comments are children of other comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, AMP_Comment_Walker:: paged_walk() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Agreed. Can revise it to be more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter how about this? b7c5963

$return[] = $comment;
}

return $return;
}
3 changes: 3 additions & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class AMP_Autoloader {
*/
private static $_classmap = array(
'AMP_Theme_Support' => 'includes/class-amp-theme-support',
'AMP_Comment_Walker' => 'includes/class-amp-comment-walker',
'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer',
'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box',
'AMP_Post_Type_Support' => 'includes/class-amp-post-type-support',
Expand Down Expand Up @@ -64,6 +65,8 @@ class AMP_Autoloader {
'AMP_Blacklist_Sanitizer' => 'includes/sanitizers/class-amp-blacklist-sanitizer',
'AMP_Iframe_Sanitizer' => 'includes/sanitizers/class-amp-iframe-sanitizer',
'AMP_Img_Sanitizer' => 'includes/sanitizers/class-amp-img-sanitizer',
'AMP_Form_Sanitizer' => 'includes/sanitizers/class-amp-form-sanitizer',
'AMP_Comments_Sanitizer' => 'includes/sanitizers/class-amp-comments-sanitizer',
'AMP_Playbuzz_Sanitizer' => 'includes/sanitizers/class-amp-playbuzz-sanitizer',
'AMP_Style_Sanitizer' => 'includes/sanitizers/class-amp-style-sanitizer',
'AMP_Tag_And_Attribute_Sanitizer' => 'includes/sanitizers/class-amp-tag-and-attribute-sanitizer',
Expand Down
107 changes: 107 additions & 0 deletions includes/class-amp-comment-walker.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<?php
/**
* Class AMP_Comment_Walker
*
* @package AMP
*/

/**
* Class AMP_Comment_Walker
*
* Walker to wrap comments in mustache tags for amp-template.
*/
class AMP_Comment_Walker extends Walker_Comment {

/**
* The original comments arguments.
*
* @since 0.7
* @var array
*/
public $args;

/**
* Starts the element output.
*
* @since 2.7.0
*
* @see Walker::start_el()
* @see wp_list_comments()
* @global int $comment_depth
* @global WP_Comment $comment
*
* @param string $output Used to append additional content. Passed by reference.
* @param WP_Comment $comment Comment data object.
* @param int $depth Optional. Depth of the current comment in reference to parents. Default 0.
* @param array $args Optional. An array of arguments. Default empty array.
* @param int $id Optional. ID of the current comment. Default 0 (unused).
*/
public function start_el( &$output, $comment, $depth = 0, $args = array(), $id = 0 ) {
$output .= '{{#comment}}';
parent::start_el( $output, $comment, $depth, $args, $id );
}
/**
* Ends the element output, if needed.
*
* @since 2.7.0
*
* @see Walker::end_el()
* @see wp_list_comments()
*
* @param string $output Used to append additional content. Passed by reference.
* @param WP_Comment $comment The current comment object. Default current comment.
* @param int $depth Optional. Depth of the current comment. Default 0.
* @param array $args Optional. An array of arguments. Default empty array.
*/
public function end_el( &$output, $comment, $depth = 0, $args = array() ) {
if ( ! empty( $args['end-callback'] ) ) {
ob_start();
call_user_func( $args['end-callback'], $comment, $args, $depth );
$output .= ob_get_clean();
} else {
if ( 'div' === $args['style'] ) {
$output .= "</div><!-- #comment-## -->\n";
} else {
$output .= "</li><!-- #comment-## -->\n";
}
}
$output .= '{{/comment}}';
}

/**
* Output amp-list template code and place holder for comments.
*
* @see Walker::paged_walk()
* @param array $elements List of comment Elements.
* @param int $max_depth The maximum hierarchical depth.
* @param int $page_num The specific page number, beginning with 1.
* @param int $per_page Per page counter.
*
* @return string XHTML of the specified page of elements.
*/
public function paged_walk( $elements, $max_depth, $page_num, $per_page ) {
if ( empty( $elements ) || $max_depth < - 1 ) {
return '';
}

$args = array_slice( func_get_args(), 4 );
if ( ! empty( $args[0]['comments_template_placeholder'] ) ) {
return $args[0]['comments_template_placeholder'];
}

$template = '<comments-template>';
$template .= parent::paged_walk( $elements, $max_depth, $page_num, $per_page, $args[0] );
$template .= '</comments-template>';

$url = get_rest_url( get_current_blog_id(), 'amp/v1/comments/' . get_the_ID() );
if ( strpos( $url, 'http:' ) === 0 ) {
$url = substr( $url, 5 );
}
// @todo Identify arguments and make filterable/settable.
$template .= '<amp-list src="' . esc_attr( $url ) . '" height="400" single-item="true" layout="fixed-height">';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling the actual comments walker here too and provide the content as placeholder/fallback content? Maybe that is overkill.

Copy link
Contributor Author

@DavidCramer DavidCramer Jan 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea. However, it would increase page weight. But setting some kind of fallback is great idea.

$template .= '<template type="amp-mustache"></template>';
$template .= '</amp-list>';

return $template;
}
}
121 changes: 121 additions & 0 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ class AMP_Theme_Support {
*/
const CUSTOM_STYLES_PLACEHOLDER = '/* AMP:CUSTOM_STYLES_PLACEHOLDER */';

/**
* Replaced with the comments template.
*
* @var string
*/
const COMMENTS_TEMPLATE_PLACEHOLDER = '/* AMP:COMMENTS_TEMPLATE_PLACEHOLDER */';

/**
* AMP Scripts.
*
Expand Down Expand Up @@ -189,6 +196,8 @@ public static function register_hooks() {

add_filter( 'the_content', array( __CLASS__, 'filter_the_content' ), PHP_INT_MAX );

// Add Comments hooks.
add_filter( 'wp_list_comments_args', array( __CLASS__, 'add_amp_comments_template' ), PHP_INT_MAX );
// @todo Add character conversion.
}

Expand Down Expand Up @@ -301,6 +310,100 @@ public static function add_amp_component_scripts() {
echo self::COMPONENT_SCRIPTS_PLACEHOLDER; // phpcs:ignore WordPress.Security.EscapeOutput, WordPress.XSS.EscapeOutput
}

/**
* Add the comments template placeholder marker
*
* @param array $args the args for the comments list..
* @return array Args to return.
*/
public static function add_amp_comments_template( $args ) {
if ( ! isset( $args['amp_comments'] ) ) {
$amp_walker = new AMP_Comment_Walker();
$args['walker'] = $amp_walker;
$args['amp_comments'] = true;
$template = self::get_comments_template( $args );
wp_cache_add( 'amp_comments_template', $template, 'amp' );
$args['comments_template_placeholder'] = self::COMMENTS_TEMPLATE_PLACEHOLDER;
}

return $args;
}

/**
* Generate a threaded mustache template based on the themes settings.
*
* @param array $args the args for the comments list.
* @return string HTML mustache template.
*/
public static function get_comments_template( $args ) {

$_comment = array(
'comment_ID' => '{{comment_ID}}',
'comment_post_ID' => get_the_ID(),
'comment_author' => '{{comment_author}}',
'comment_author_email' => '{{comment_author_email}}',
'comment_author_url' => '{{comment_author_url}}',
'comment_author_IP' => '{{comment_author_IP}}',
'comment_date' => '{{comment_date}}',
'comment_date_gmt' => '{{comment_date_gmt}}',
'comment_content' => '{{comment_content}}',
'comment_karma' => '{{comment_karma}}',
'comment_approved' => '{{comment_approved}}',
'comment_agent' => '{{comment_agent}}',
'comment_type' => '{{comment_type}}',
'comment_parent' => '',
'user_id' => '{{user_id}}',
);

$comments = array();
$depth = ( $args['max_depth'] ? $args['max_depth'] : 5 );
for ( $i = 0; $i < $depth; $i ++ ) {
$comment = new stdClass();
foreach ( $_comment as $key => $value ) {
if ( 'comment_ID' === $key ) {
$value = $i + 1;
}
if ( 'comment_parent' === $key && $i > 0 ) {
$value = $i;
}
$comment->{$key} = $value;
}
$comments[] = $comment;
}

$args['echo'] = false;
$args['amp_pass'] = true;
if ( empty( $args['walker'] ) ) {
$args['walker'] = new AMP_Comment_Walker();
}

// Filter dynamic data with musatche variable strings.
// @todo add additional filters for dynamic data like "get_comment_author_link", "get_comment_author_IP" etc...
add_filter( 'get_comment_date', array( __CLASS__, 'get_comment_date_template_string' ) );
add_filter( 'get_comment_time', array( __CLASS__, 'get_comment_time_template_string' ) );

return wp_list_comments( $args, $comments );

}

/**
* Get somment date string for template.
*
* @return string Mustache template string.
*/
public static function get_comment_date_template_string() {
return '{{comment_date}}';
}

/**
* Get somment time string for template.
*
* @return string Mustache template string.
*/
public static function get_comment_time_template_string() {
return '{{comment_time}}';
}

/**
* Get canonical URL for current request.
*
Expand Down Expand Up @@ -509,7 +612,25 @@ public static function finish_output_buffering( $output ) {
1
);

// Inject comments template.
$output = preg_replace(
'#' . preg_quote( self::COMMENTS_TEMPLATE_PLACEHOLDER, '#' ) . '#',
$args = wp_cache_get( 'amp_comments_template', 'amp' ),
$output,
1
);

$dom = AMP_DOM_Utils::get_dom( $output );
// Sanitize forms in the document.
$sanitizer = new AMP_Form_Sanitizer( $dom );
$sanitizer->sanitize();

$sanitizer = new AMP_Comments_Sanitizer( $dom );
$sanitizer->sanitize();

// @todo Add more validation checking and potentially the whitelist sanitizer.
$output = $dom->saveHTML();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidCramer I chatted about this with @ThierryA, but I think we should avoid having to load the entire response into the DOM for sanitization. I think that we should limit sanitization to just the post content, 3rd-party widgets, and other “leaf nodes” which we know are liable to be invalid AMP.

Is this right? Or am I off track? My worry is that loading the HTML into a DOMDocument for every request will be too heavy. But at the same time, if we're already spinning up a DOMDocument for every instance of the_content() maybe it is actually better to just do one for the entire HTML. This would certainly simplify somethings for sanitizing widgets (cc @kienstra). It could also mean that we wouldn't have to have this ad hoc logic to search for elements requiring components: https://github.com/Automattic/amp-wp/blob/e60cb152a50e2ffbf5504d41aee859ad1d0e0baa/includes/class-amp-theme-support.php#L448-L455

Maybe we should decide to not prematurely optimize and instead keep things simple at first and just go ahead and sanitize the entire response. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See findings at #875 (comment)

It seems we should definitely not sanitize the entire output buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter whats the consensus now on this? It looks like your findings indicated it may be a good idea to sanitize the whole buffer output. I personally think this is the most efficient. I spoke with @ThierryA about this yesterday and he was going to do some tests, like you did.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning toward sanitizing the entire output buffer now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidCramer do you want to build off of #875 (comment) in a new PR to serve as a basis for what you're introducing here with form sanitization?


return $output;
}
}
Loading