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

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

Conversation

Projects
None yet
3 participants
@DavidCramer
Contributor

DavidCramer commented Mar 20, 2018

fixes #1028
fixes #936

@DavidCramer DavidCramer requested a review from westonruter Mar 20, 2018

@DavidCramer

This comment has been minimized.

Contributor

DavidCramer commented Mar 20, 2018

@westonruter you can take a look at this now. I've included #936 as well, since they are closely related.

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Mar 20, 2018

@DavidCramer I don't see how #936 is included here. I was expecting to see something like this: #1020 (comment)

@DavidCramer

This comment has been minimized.

Contributor

DavidCramer commented Mar 20, 2018

@westonruter - I was basing it on your second thought - #1020 (comment)

@DavidCramer

This comment has been minimized.

Contributor

DavidCramer commented Mar 20, 2018

@westonruter - I see I missed the filter, sorry. ignore me, I'll revised it.

@westonruter

The theme support flag does not seem to work with the changes here. When I add the the comments_live_list flag (xwp/ampnews#85) there is still a redirect.

$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'] ) ) {

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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']

// Send redirect header if amp-live-list has been opted-out.
if ( empty( $theme_support['comments_live_list'] ) ) {
header( 'AMP-Redirect-To: ' . $url, true );

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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

} else {
$message = __( 'Your comment has been posted, and is awaiting moderation.', 'default' );
}

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

Missing phpdoc.

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' );

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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.

@@ -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}}}

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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

@@ -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}}}

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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.

// We don't need any data, so just send a success.
wp_send_json_success();
}, PHP_INT_MAX );
wp_send_json_success( $message );

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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

}
$message = apply_filters( 'amp_comment_submitted_message', $message, $comment );
// We don't need any data, so just send a success.

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

This comment is no longer true.

westonruter added some commits Mar 21, 2018

Fix and refactor XHR handling
* Explicitly limit handle_xhr_request to POST requests.
* Make sure original status header is sent back in wp_die() call.
* Fix reading of amp theme support flag since get_theme_support() returns an array of args.
* Add missing paragraph surrounding success message template.
* Fix moderation comment to ensure default core string is re-used.
* Fix PHP notice in regards to additional comment_live_list theme support flag being present.
* Remove obsolete comments.
* Simplify success message.
* Add phpdoc for amp_comment_posted_message filter.
* Pass back success message in same way as error message is returned as named variable.
* Add missing tests and introduce \AMP_Theme_Support::send_header() to avoid having to runInSeparateProcess.

@westonruter westonruter changed the title from #1028 - add amp-redirect if amp-live-list is not declared to Do redirect if amp comments_live_list support is not declared; vary comment success message by approval status Mar 21, 2018

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Mar 21, 2018

@DavidCramer Please test these changes with the corresponding changes in the theme PR xwp/ampnews#85

Test submitting comments with the comments_live_list flag enabled and disabled. Also test Jetpack comment form to ensure it continues to work as expected. It is working for me.

@westonruter westonruter requested a review from ThierryA Mar 21, 2018

@westonruter westonruter added this to the v0.7 milestone Mar 21, 2018

@DavidCramer

This comment has been minimized.

Contributor

DavidCramer commented Mar 21, 2018

@westonruter Thanks for those changes. I've gone over them to get better understanding of it all. Will review and test xwp/ampnews#85

@ThierryA

Great work! Minor review below

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 );

This comment has been minimized.

@ThierryA

ThierryA Mar 21, 2018

Collaborator

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

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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

* }
*/
public static function handle_wp_die( $error, $title = '', $args = array() ) {
$status_code = 500;

This comment has been minimized.

@ThierryA

ThierryA Mar 21, 2018

Collaborator

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

wp_send_json_success();
}, PHP_INT_MAX );
self::handle_xhr_headers_output();
} elseif ( ! empty( self::$purged_amp_query_vars['_wp_amp_action_xhr_converted'] ) ) {

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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

This comment has been minimized.

@westonruter

westonruter Mar 21, 2018

Collaborator

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.

Improve comments regarding XHR handling
Improve default status_code handling

@westonruter westonruter merged commit f957712 into 0.7 Mar 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/1028-amp-live-list-opt-in branch Mar 21, 2018

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