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

Handle general form posts #923

Merged
merged 24 commits into from
Feb 7, 2018
Merged

Handle general form posts #923

Show file tree
Hide file tree
Changes from 16 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
View file
Open in desktop
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_handle_xhr_request();
}

// Make sure the `amp` query var has an explicit value.
Expand Down
68 changes: 61 additions & 7 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,17 +406,54 @@ 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_handle_xhr_request() {
global $pagenow;
if ( isset( $_GET['__amp_redirect'] ) ) { // WPCS: CSRF ok.
add_action( 'template_redirect', function() {
// grab post data.
$transint_name = wp_unslash( $_GET['__amp_redirect'] ); // WPCS: CSRF ok, input var ok.
$_POST = get_transient( $transint_name );
delete_transient( $transint_name );
}, 0 );
}
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 );
amp_handle_xhr_headers_output();
} elseif ( isset( $_GET['_wp_amp_action_xhr_converted'] ) ) { // WPCS: CSRF ok.
// Add amp redirect hooks.
add_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 );
add_action( 'template_redirect', function() {
// grab post data.
$transient_name = uniqid();
set_transient( $transient_name, wp_unslash( $_POST ), 60 ); // WPCS: CSRF ok, input var ok.
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 do fear it could lead to unexpected results like you said. So I think for now we should omit it and instead revisit it later if we find that it is a common problem.

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 Ye I thought so to. I was going to just remove it, but wanted to see if I could at least make your example work. It's better not being there for now.


/*
* 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.
$location = add_query_arg( '__amp_redirect', $transient_name, $location );
amp_intercept_post_request_redirect( $location );
}, 0 );
amp_handle_xhr_headers_output();
}

}

/**
* Handle the AMP XHR headers and output errors.
*
* @since 0.7.0
*/
function amp_handle_xhr_headers_output() {
// Add die handler for AMP error display.
add_filter( 'wp_die_handler', function() {
/**
Expand All @@ -438,3 +475,20 @@ 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_intercept_post_request_redirect( $location ) {
$host = wp_parse_url( $location, PHP_URL_HOST );
if ( is_null( $host ) ) {
$location = home_url( $location );
}
header( 'AMP-Redirect-To: ' . $location );
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
View file
Open in desktop
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
View file
Open in desktop
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#success/error-response-rendering
* @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#success/error-response-rendering
* @since 0.7
*
* @return DOMElement The div[submit-error] element.
*/
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
View file
Open in desktop
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