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

Handle general form posts #923

Merged
merged 24 commits into from
Feb 7, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c229cfc
handle general form requests around comment post logic
DavidCramer Feb 1, 2018
e68a752
Merge develop
DavidCramer Feb 1, 2018
96c0bac
record that action was converted to action-xhr and only handle conver…
DavidCramer Feb 1, 2018
024d4e0
rename function to be less ambiguous
DavidCramer Feb 1, 2018
967a8c6
dont add wp_redirect filter on comment-handler
DavidCramer Feb 1, 2018
4b259df
add appending error template for converted forms
DavidCramer Feb 1, 2018
ac86bf2
redirect page if redirect-template hits
DavidCramer Feb 1, 2018
eca4789
update docs
DavidCramer Feb 1, 2018
1cbfff7
merge develop
DavidCramer Feb 2, 2018
9c965ad
rename handler function
DavidCramer Feb 2, 2018
83a0799
simplify handler function
DavidCramer Feb 2, 2018
09de420
correct phpdoc return tag
DavidCramer Feb 2, 2018
40934c1
update @link with correct url
DavidCramer Feb 2, 2018
6dd4e9d
rename redirect function
DavidCramer Feb 2, 2018
a9d9bfd
catch data for redirection and reload inclusion hackery
DavidCramer Feb 2, 2018
17ab3fd
check redirect location has a host first
DavidCramer Feb 2, 2018
92f3a76
Allow all formatting tags to be returned in submit error response
westonruter Feb 2, 2018
1593f88
Remove support for handling non-redirecting post requests, for now
westonruter Feb 2, 2018
3791cd8
Ensure relative redirect URLs get made absolute for AMP
westonruter Feb 2, 2018
7a4b398
add process isolation
DavidCramer Feb 5, 2018
d38f03e
remove isolation tag
DavidCramer Feb 5, 2018
3b3b545
add test for amp_intercept_post_request_redirect
DavidCramer Feb 6, 2018
4090e4e
add test for amp_handle_xhr_request
DavidCramer Feb 6, 2018
ee75de2
Augment amp_intercept_post_request_redirect test with checking output
westonruter Feb 7, 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
5 changes: 1 addition & 4 deletions amp.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ function amp_after_setup_theme() {
* @global string $pagenow
*/
function amp_init() {
global $pagenow;

/**
* Triggers on init when AMP plugin is active.
Expand All @@ -128,9 +127,7 @@ function amp_init() {
if ( class_exists( 'Jetpack' ) && ! ( defined( 'IS_WPCOM' ) && IS_WPCOM ) ) {
require_once AMP__DIR__ . '/jetpack-helper.php';
}
if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) {
amp_prepare_comment_post();
}
amp_prepare_xhr_post();
}

// Make sure the `amp` query var has an explicit value.
Expand Down
47 changes: 39 additions & 8 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -406,17 +406,33 @@ function amp_print_schemaorg_metadata() {
*
* @since 0.7.0
*/
function amp_prepare_comment_post() {
if ( ! isset( $_GET['__amp_source_origin'] ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars().
function amp_prepare_xhr_post() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename this to amp_handle_xhr_request.

global $pagenow;
if ( ! isset( $_GET['__amp_source_origin'] ) || ! isset( $pagenow ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars().
return;
}

// Add amp comment hooks.
add_filter( 'comment_post_redirect', function() {
// We don't need any data, so just send a success.
wp_send_json_success();
}, PHP_INT_MAX, 2 );

if ( 'wp-comments-post.php' === $pagenow ) {
// This only runs on wp-comments-post.php.
add_filter( 'comment_post_redirect', function() {
// We don't need any data, so just send a success.
wp_send_json_success();
}, PHP_INT_MAX, 2 );
} elseif ( ! isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok.
// submission was from a set action-xhr, implying it's being handled already.
return;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest inverting the isset check and moving the redirect filter from else to 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 The wp_die_handler and AMP-Access-Control-Allow-Source-Origin are fall throughs for both the comments and general submissions. moving the filter in here would make these happen for handled and unhandled. But I see what you're saying. and know how to simplify it.

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 see 83a0799 does that work?

} else {
// Add amp redirect hooks.
add_filter( 'wp_redirect', 'amp_handle_general_post', PHP_INT_MAX, 2 );
add_action( 'template_redirect', function() {
/*
* Buffering starts here, so unlikely the form has a redirect,
* so force a redirect to the same page.
Copy link
Member

Choose a reason for hiding this comment

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

This is clever, but I'm worried it could have unintended results. In particular, when a form submission does not do a redirect after a POST then the data from the POST submission is often displayed in some way on the response. If we do the redirect, then it could cause the user to be redirected back to a blank page. Consider something like this:

<form method="post">
     <?php if ( ! empty( $_POST ) ) : ?>
          Submission successful!
          <pre><?php echo esc_html( wp_json_encode( wp_unslash( $_POST ) ) ); ?></pre>
     <?php else : ?>
         <input name="data"><button>Submit</button>
     <?php endif; ?>
</form>

Something like this will break if we do the redirect 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 this is somewhat a hack and will probably have side effects see a9d9bfd

*/
$location = esc_url_raw( wp_unslash( $_SERVER['REQUEST_URI'] ) ); // WPCS: CSRF ok, input var ok.
amp_handle_general_post( $location );
}, 0 );
}
// Add die handler for AMP error display.
add_filter( 'wp_die_handler', function() {
/**
Expand All @@ -438,3 +454,18 @@ function amp_prepare_comment_post() {
$origin = esc_url_raw( wp_unslash( $_GET['__amp_source_origin'] ) ); // WPCS: CSRF ok.
header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true );
}

/**
* Handle a general, non comment AMP XHR post.
*
* @since 0.7.0
* @param string $location The location to redirect to.
*/
function amp_handle_general_post( $location ) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this amp_intercept_post_request_redirect.


$url = site_url( $location );
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't $location already include the host? If so, then I think this would break because site_url() takes a path as its argument. I think rather that the $location should be checked to see if it contains a host already, and if not, then grab the host from home_url() (not site_url(), as it is for the admin not the frontend).

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 Sorry I always get those two mixed up. was supposed to be home_url() since the $location is a relative link. But this may or maynot be, so should check for host first.
Good catch :)

header( 'AMP-Redirect-To: ' . $url );
header( 'Access-Control-Expose-Headers: AMP-Redirect-To;' );
// Send json success as no data is required.
wp_send_json_success();
}
3 changes: 2 additions & 1 deletion includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public static function init() {
self::register_paired_hooks();
}

self::purge_amp_query_vars(); // Note that amp_prepare_comment_post() still looks at $_GET['__amp_source_origin'].
self::purge_amp_query_vars(); // Note that amp_prepare_xhr_post() still looks at $_GET['__amp_source_origin'].
self::register_hooks();
self::$embed_handlers = self::register_content_embed_handlers();
self::$sanitizer_classes = amp_get_content_sanitizers();
Expand Down Expand Up @@ -214,6 +214,7 @@ public static function register_hooks() {
public static function purge_amp_query_vars() {
$query_vars = array(
'__amp_source_origin',
'_wp_amp_action_xhr_converted',
'amp_latest_update_time',
);

Expand Down
43 changes: 43 additions & 0 deletions includes/sanitizers/class-amp-form-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ public function sanitize() {
} elseif ( 'post' === $method ) {
$node->removeAttribute( 'action' );
if ( ! $xhr_action ) {
// record that action was converted tp action-xhr.
$action_url = add_query_arg( '_wp_amp_action_xhr_converted', 1, $action_url );
$node->setAttribute( 'action-xhr', $action_url );
// Append error handler if not found.
$this->error_handler( $node );
Copy link
Member

Choose a reason for hiding this comment

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

Very nice.

} elseif ( 'http://' === substr( $xhr_action, 0, 7 ) ) {
$node->setAttribute( 'action-xhr', substr( $xhr_action, 5 ) );
}
Expand All @@ -108,4 +112,43 @@ public function sanitize() {
}
}
}

/**
* Checks if the form has an error handler else create one if not.
*
* @link https://www.ampproject.org/docs/reference/components/amp-form
* @since 0.7
* @param DOMElement $node The form node to check.
*/
public function error_handler( $node ) {
$templates = $node->getElementsByTagName( 'template' );
if ( $templates->length ) {
for ( $i = $templates->length - 1; $i >= 0; $i-- ) {
if ( $templates->item( $i )->parentNode->hasAttribute( 'submit-error' ) ) {
return; // Found error template, do nothing.
}
}
}
$node->appendChild( $this->create_error_template() );
}

/**
* Creates a error handler element node.
*
* @link https://www.ampproject.org/docs/reference/components/amp-form
* @since 0.7
*
* return DOMElement
Copy link
Member

Choose a reason for hiding this comment

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

Missing @ before return, and missing description. Should be something like:

* @return DOMElement The div[submit-error] element.

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 I really need to focus on phpdoc tags. thanks.

*/
public function create_error_template() {
$node = $this->dom->createElement( 'div' );
$template = $this->dom->createElement( 'template' );
$mustache = $this->dom->createTextNode( '{{{error}}}' );
$node->setAttribute( 'submit-error', '' );
$template->setAttribute( 'type', 'amp-mustache' );
$template->appendChild( $mustache );
$node->appendChild( $template );

return $node;
}
}
4 changes: 2 additions & 2 deletions tests/test-amp-form-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public function get_data() {
),
'form_with_post_method_http_action_and_no_target' => array(
'<form method="post" action="http://example.org/example-page/"></form>',
'<form method="post" action-xhr="//example.org/example-page/" target="_top"></form>',
'<form method="post" action-xhr="//example.org/example-page/?_wp_amp_action_xhr_converted=1" target="_top"><div submit-error=""><template type="amp-mustache">{{{error}}}</template></div></form>',
),
'form_with_post_method_http_action_and_blank_target' => array(
'<form method="post" action-xhr="http://example.org/example-page/" target="_blank"></form>',
Expand All @@ -58,7 +58,7 @@ public function get_data() {
),
'form_with_post_method_https_action_and_custom_target' => array(
'<form method="post" action="https://example.org/" target="some_other_target"></form>',
'<form method="post" target="_blank" action-xhr="https://example.org/"></form>',
'<form method="post" target="_blank" action-xhr="https://example.org/?_wp_amp_action_xhr_converted=1"><div submit-error=""><template type="amp-mustache">{{{error}}}</template></div></form>',
),
);
}
Expand Down