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

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

Show file tree
Hide file tree
Changes from 4 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
204 changes: 161 additions & 43 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ class AMP_Theme_Support {
*/
public static $purged_amp_query_vars = array();

/**
* Headers sent (or attempted to be sent).
*
* @since 0.7
* @see AMP_Theme_Support::send_header()
* @var array[]
*/
public static $headers_sent = array();

/**
* Initialize.
*/
Expand All @@ -87,7 +96,7 @@ public static function init() {
$args = array_shift( $support );
if ( ! is_array( $args ) ) {
trigger_error( esc_html__( 'Expected AMP theme support arg to be array.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
} elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback' ) ) ) !== 0 ) {
} elseif ( count( array_diff( array_keys( $args ), array( 'template_dir', 'available_callback', 'comments_live_list' ) ) ) !== 0 ) {
trigger_error( esc_html__( 'Expected AMP theme support to only have template_dir and/or available_callback.', 'amp' ) ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
}
}
Expand Down Expand Up @@ -310,65 +319,174 @@ public static function purge_amp_query_vars() {
}
}

/**
* Send an HTTP response header.
*
* This largely exists to facilitate unit testing but it also provides a better interface for sending headers.
*
* @since 0.7.0
*
* @param string $name Header name.
* @param string $value Header value.
* @param array $args {
* Args to header().
*
* @type bool $replace Whether to replace a header previously sent. Default true.
* @type int $status_code Status code to send with the sent header.
* }
* @return bool Whether the header was sent.
*/
public static function send_header( $name, $value, $args = array() ) {
$args = array_merge(
array(
'replace' => true,
'status_code' => null,
),
$args
);

self::$headers_sent[] = array_merge( compact( 'name', 'value' ), $args );
if ( headers_sent() ) {
return false;
}

header(
sprintf( '%s: %s', $name, $value ),
$args['replace'],
$args['status_code']
);
return true;
}

/**
* Hook into a form submissions, such as the comment form or some other form submission.
*
* @since 0.7.0
* @global string $pagenow
*/
public static function handle_xhr_request() {
global $pagenow;
if ( empty( self::$purged_amp_query_vars['__amp_source_origin'] ) ) {
if ( empty( self::$purged_amp_query_vars['__amp_source_origin'] ) || empty( $_SERVER['REQUEST_METHOD'] ) || 'POST' !== $_SERVER['REQUEST_METHOD'] ) {
return;
}

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() {
// We don't need any data, so just send a success.
wp_send_json_success();
}, PHP_INT_MAX );
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 );
self::handle_xhr_headers_output();
// Send AMP response header.
$origin = wp_validate_redirect( wp_sanitize_redirect( esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] ) ) );
if ( $origin ) {
self::send_header( 'AMP-Access-Control-Allow-Source-Origin', $origin, array( 'replace' => true ) ); // @todo The $origin needs to be validated.
}

// Intercept POST requests which redirect.
add_filter( 'wp_redirect', array( __CLASS__, 'intercept_post_request_redirect' ), PHP_INT_MAX );

// Add special handling for redirecting after comment submission.
add_filter( 'comment_post_redirect', array( __CLASS__, 'filter_comment_post_redirect' ), PHP_INT_MAX, 2 );

// Add die handler for AMP error display, most likely due to problem with comment.
add_filter( 'wp_die_handler', function() {
return array( __CLASS__, 'handle_wp_die' );
} );

}

/**
* Handle the AMP XHR headers and output errors.
* Strip tags that are not allowed in amp-mustache.
*
* @since 0.7.0
*
* @param string $text Text to sanitize.
* @return string Sanitized text.
*/
public static function handle_xhr_headers_output() {
// Add die handler for AMP error display.
add_filter( 'wp_die_handler', function() {
/**
* New error handler for AMP form submission.
*
* @param WP_Error|string $error The error to handle.
*/
return function( $error ) {
status_header( 400 );
if ( is_wp_error( $error ) ) {
$error = $error->get_error_message();
}
$amp_mustache_allowed_html_tags = array( 'strong', 'b', 'em', 'i', 'u', 's', 'small', 'mark', 'del', 'ins', 'sup', 'sub' );
wp_send_json( array(
'error' => wp_kses( $error, array_fill_keys( $amp_mustache_allowed_html_tags, array() ) ),
) );
};
} );
protected static function wp_kses_amp_mustache( $text ) {
$amp_mustache_allowed_html_tags = array( 'strong', 'b', 'em', 'i', 'u', 's', 'small', 'mark', 'del', 'ins', 'sup', 'sub' );
return wp_kses( $text, array_fill_keys( $amp_mustache_allowed_html_tags, array() ) );
}

/**
* Handle comment_post_redirect to ensure page reload is done when comments_live_list is not supported, while sending back a success message when it is.
*
* @since 0.7.0
*
* @param string $url Comment permalink to redirect to.
* @param WP_Comment $comment Posted comment.
* @return string URL.
*/
public static function filter_comment_post_redirect( $url, $comment ) {
$theme_support = get_theme_support( 'amp' );

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.

// Cause a page refresh if amp-live-list is not implemented for comments via add_theme_support( 'amp', array( 'comments_live_list' => true ) ).
if ( empty( $theme_support[0]['comments_live_list'] ) ) {

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

Choose a reason for hiding this comment

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

Return statement can happen on this line, no need to assign it to a var.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, but the idea is that eventually this should not be necessary at all. I've opened an issue in amphtml to address this and I'll add a comment here to reference it: ampproject/amphtml#14170


// Pass URL along to wp_redirect().
return $url;
}

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

/**
* Filters the message when comment submitted success message when
*
* @since 0.7
*/
$message = apply_filters( 'amp_comment_posted_message', $message, $comment );

// Message will be shown in template defined by AMP_Theme_Support::add_amp_comment_form_templates().
wp_send_json( array(
'message' => self::wp_kses_amp_mustache( $message ),
) );
}

/**
* New error handler for AMP form submission.
*
* @since 0.7.0
* @see wp_die()
*
* @param WP_Error|string $error The error to handle.
* @param string|int $title Optional. Error title. If `$message` is a `WP_Error` object,
* error data with the key 'title' may be used to specify the title.
* If `$title` is an integer, then it is treated as the response
* code. Default empty.
* @param string|array|int $args {
* Optional. Arguments to control behavior. If `$args` is an integer, then it is treated
* as the response code. Default empty array.
*
* @type int $response The HTTP response code. Default 200 for Ajax requests, 500 otherwise.
* }
*/
public static function handle_wp_die( $error, $title = '', $args = array() ) {
$status_code = 500;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That could move in a else statement at the end of the statements below.

if ( is_int( $title ) ) {
$status_code = $title;
} elseif ( is_int( $args ) ) {
$status_code = $args;
} elseif ( is_array( $args ) && isset( $args['response'] ) ) {
$status_code = $args['response'];
}
status_header( $status_code );

// Send AMP header.
$origin = esc_url_raw( self::$purged_amp_query_vars['__amp_source_origin'] );
header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true );
if ( is_wp_error( $error ) ) {
$error = $error->get_error_message();
}

// Message will be shown in template defined by AMP_Theme_Support::add_amp_comment_form_templates().
wp_send_json( array(
'error' => self::wp_kses_amp_mustache( $error ),
) );
}

/**
* Intercept the response to a non-comment POST request.
* Intercept the response to a POST request.
*
* @since 0.7.0
* @see wp_redirect()
*
* @param string $location The location to redirect to.
*/
public static function intercept_post_request_redirect( $location ) {
Expand All @@ -378,7 +496,7 @@ public static function intercept_post_request_redirect( $location ) {
array(
'scheme' => 'https',
'host' => wp_parse_url( home_url(), PHP_URL_HOST ),
'path' => strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ),
'path' => isset( $_SERVER['REQUEST_URI'] ) ? strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ) : '/',
),
wp_parse_url( $location )
);
Expand All @@ -395,9 +513,9 @@ public static function intercept_post_request_redirect( $location ) {
$absolute_location .= '#' . $parsed_location['fragment'];
}

header( 'AMP-Redirect-To: ' . $absolute_location );
header( 'Access-Control-Expose-Headers: AMP-Redirect-To' );
// Send json success as no data is required.
self::send_header( 'AMP-Redirect-To', $absolute_location );
self::send_header( 'Access-Control-Expose-Headers', 'AMP-Redirect-To' );

wp_send_json_success();
}

Expand Down Expand Up @@ -516,7 +634,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' ); ?>
<p>{{{message}}}</p>
</template>
</div>
<div submit-error>
Expand Down
Loading