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 15 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
2 changes: 2 additions & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ function amp_get_content_sanitizers( $post = null ) {
'AMP_Video_Sanitizer' => array(),
'AMP_Audio_Sanitizer' => array(),
'AMP_Playbuzz_Sanitizer' => array(),
'AMP_Form_Sanitizer' => array(),
'AMP_Comments_Sanitizer' => array(),
'AMP_Iframe_Sanitizer' => array(
'add_placeholder' => true,
),
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;
}
}
Loading