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.

Sign up for GitHub

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

Jump to bottom

Do redirect if amp comments_live_list support is not declared; vary comment success message by approval status #1029

Merged
merged 6 commits into from
Mar 21, 2018
Merged

Do redirect if amp comments_live_list support is not declared; vary comment success message by approval status #1029

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb649cc
add amp-redirect if amp-live-list is not declared
DavidCramer Mar 20, 2018
6f00f20
Add message to response based on comment approval
DavidCramer Mar 20, 2018
a0fce3f
Fix and refactor XHR handling
westonruter Mar 21, 2018
1e32d6c
Add validation for __amp_source_origin
westonruter Mar 21, 2018
5c96324
Use scheme-less URL for redirect if non-HTTPS
westonruter Mar 21, 2018
8da1a36
Improve comments regarding XHR handling
westonruter Mar 21, 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
28 changes: 24 additions & 4 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,30 @@ public static function handle_xhr_request() {

if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) {
// We don't need any data, so just send a success.
add_filter( 'comment_post_redirect', function() {
add_filter( 'comment_post_redirect', function( $url, $comment ) {
// Get theme support.
$theme_support = get_theme_support( 'amp' );

// Add the comment ID to the URL to force AMP to refresh the page.
$url = add_query_arg( 'comment', $comment->comment_ID, $url );

// Send redirect header if amp-live-list has been opted-out.
if ( empty( $theme_support['comments_live_list'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work because $theme_support is an array of the args so it needs to be looking at $theme_support[0]['comments_live_list']

header( 'AMP-Redirect-To: ' . $url, true );
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary because wp_redirect() is currently being called. It only needs to return the $url

}
// Create a success message to display to the user.
if ( '1' === (string) $comment->comment_approved ) {
$message = __( 'Your comment has been posted and has been approved.', 'amp' );
} else {
$message = __( 'Your comment has been posted, and is awaiting moderation.', 'default' );
Copy link
Member

Choose a reason for hiding this comment

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

This string has the default text domain because it is re-using a string in core:

https://github.com/WordPress/wordpress-develop/blob/8f95800d52c1736d651ae6e259f90ad4a0db2c3f/src/wp-includes/class-walker-comment.php#L285

We need it to keep it the same for it to re-use core translations.

}

Copy link
Member

Choose a reason for hiding this comment

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

Missing phpdoc.

$message = apply_filters( 'amp_comment_submitted_message', $message, $comment );

// We don't need any data, so just send a success.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is no longer true.

wp_send_json_success();
}, PHP_INT_MAX );
wp_send_json_success( $message );
Copy link
Member

Choose a reason for hiding this comment

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

We should probably apply kses to this to only allow the allowed amp-mustache markup.


}, PHP_INT_MAX, 2 );
self::handle_xhr_headers_output();
} elseif ( ! empty( self::$purged_amp_query_vars['_wp_amp_action_xhr_converted'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I realized that we're now not properly looking at _wp_amp_action_xhr_converted.

Copy link
Member

Choose a reason for hiding this comment

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

On further thought, I don't think the logic this function cares if _wp_amp_action_xhr_converted. If someone is implementing their own action-xhr endpoint, then they're not going to be doing wp_redirect() anyway, but rather should be responding with their own appropriate wp_send_json() call.

add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX );
Expand Down Expand Up @@ -516,7 +536,7 @@ public static function add_amp_comment_form_templates() {
?>
<div submit-success>
<template type="amp-mustache">
<?php esc_html_e( 'Your comment has been posted, but may be subject to moderation.', 'amp' ); ?>
{{{data}}}
Copy link
Member

Choose a reason for hiding this comment

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

This should have a paragraph wrapping it, like for the error message.

Copy link
Member

Choose a reason for hiding this comment

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

The error handler sends back an object with an error key. I think this should be similarly looking for an object returned with just a message key.

</template>
</div>
<div submit-error>
Expand Down