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 all 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_handle_xhr_request();
}

// Make sure the `amp` query var has an explicit value.
Expand Down
71 changes: 61 additions & 10 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -400,23 +400,36 @@ function amp_print_schemaorg_metadata() {
}

/**
* Hook into a comment submission of an AMP XHR post request.
*
* This only runs on wp-comments-post.php.
* Hook into a form submissions, such as comment the form or some other .
*
* @since 0.7.0
* @global string $pagenow
*/
function amp_prepare_comment_post() {
function amp_handle_xhr_request() {
global $pagenow;
if ( ! isset( $_GET['__amp_source_origin'] ) ) { // WPCS: CSRF ok. Beware of AMP_Theme_Support::purge_amp_query_vars().
return;
}

// Add amp comment hooks.
add_filter( 'comment_post_redirect', function() {
if ( isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) {
// We don't need any data, so just send a success.
wp_send_json_success();
}, PHP_INT_MAX, 2 );
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_filter( 'wp_redirect', 'amp_intercept_post_request_redirect', PHP_INT_MAX, 2 );
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 @@ -429,12 +442,50 @@ function amp_prepare_comment_post() {
if ( is_wp_error( $error ) ) {
$error = $error->get_error_message();
}
$error = strip_tags( $error, 'strong' );
wp_send_json( compact( 'error' ) );
$amp_mustache_allowed_html_tags = array( 'strong', 'b', 'em', 'i', 'u', 's', 'small', 'mark', 'del', 'ins', 'sup', 'sub' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice one!

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 a little hard to test as it's on a die handler and causes much issues. I left it out of the burrent test. Perhaps you could give a suggestion?

wp_send_json( array(
'error' => wp_kses( $error, array_fill_keys( $amp_mustache_allowed_html_tags, array() ) ),
) );
};
} );

// Send AMP header.
$origin = esc_url_raw( wp_unslash( $_GET['__amp_source_origin'] ) ); // WPCS: CSRF ok.
header( 'AMP-Access-Control-Allow-Source-Origin: ' . $origin, true );
}

/**
* Intercept the response to a non-comment POST request.
*
* @since 0.7.0
* @param string $location The location to redirect to.
*/
function amp_intercept_post_request_redirect( $location ) {

// Make sure relative redirects get made absolute.
$parsed_location = array_merge(
array(
'scheme' => 'https',
'host' => wp_parse_url( home_url(), PHP_URL_HOST ),
'path' => strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ),
),
wp_parse_url( $location )
);

$absolute_location = $parsed_location['scheme'] . '://' . $parsed_location['host'];
if ( isset( $parsed_location['port'] ) ) {
$absolute_location .= ':' . $parsed_location['port'];
}
$absolute_location .= $parsed_location['path'];
if ( isset( $parsed_location['query'] ) ) {
$absolute_location .= '?' . $parsed_location['query'];
}
if ( isset( $parsed_location['fragment'] ) ) {
$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.
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
30 changes: 30 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->ensure_submit_error_element( $node );
} elseif ( 'http://' === substr( $xhr_action, 0, 7 ) ) {
$node->setAttribute( 'action-xhr', substr( $xhr_action, 5 ) );
}
Expand All @@ -108,4 +112,30 @@ 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 $form The form node to check.
*/
public function ensure_submit_error_element( $form ) {
$templates = $form->getElementsByTagName( 'template' );
for ( $i = $templates->length - 1; $i >= 0; $i-- ) {
if ( $templates->item( $i )->parentNode->hasAttribute( 'submit-error' ) ) {
return; // Found error template, do nothing.
}
}

$div = $this->dom->createElement( 'div' );
$template = $this->dom->createElement( 'template' );
$mustache = $this->dom->createTextNode( '{{{error}}}' );
$div->setAttribute( 'submit-error', '' );
$template->setAttribute( 'type', 'amp-mustache' );
$template->appendChild( $mustache );
$div->appendChild( $template );
$form->appendChild( $div );
}
}
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
65 changes: 65 additions & 0 deletions tests/test-amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -362,4 +362,69 @@ public function test_amp_get_schemaorg_metadata() {
$this->assertArrayHasKey( 'did_amp_schemaorg_metadata', $metadata );
$this->assertEquals( 'George', $metadata['author']['name'] );
}

/**
* Test amp_intercept_post_request_redirect().
*
* @runInSeparateProcess
* @preserveGlobalState disabled
* @covers amp_intercept_post_request_redirect()
*/
public function test_amp_intercept_post_request_redirect() {
if ( ! function_exists( 'xdebug_get_headers' ) ) {
$this->markTestSkipped( 'xdebug is required for this test' );
}

add_theme_support( 'amp' );
$url = get_home_url();

add_filter( 'wp_doing_ajax', '__return_true' );
add_filter( 'wp_die_ajax_handler', function () {
return '__return_false';
} );

ob_start();
amp_intercept_post_request_redirect( $url );
$this->assertEquals( '{"success":true}', ob_get_clean() );

$this->assertContains( 'AMP-Redirect-To: ' . $url, xdebug_get_headers() );
$this->assertContains( 'Access-Control-Expose-Headers: AMP-Redirect-To', xdebug_get_headers() );

ob_start();
amp_intercept_post_request_redirect( '/new-location/' );
$this->assertEquals( '{"success":true}', ob_get_clean() );
$this->assertContains( 'AMP-Redirect-To: https://example.org/new-location/', xdebug_get_headers() );

ob_start();
amp_intercept_post_request_redirect( '//example.com/new-location/' );
$this->assertEquals( '{"success":true}', ob_get_clean() );
$headers = xdebug_get_headers();
$this->assertContains( 'AMP-Redirect-To: https://example.com/new-location/', $headers );

ob_start();
amp_intercept_post_request_redirect( '' );
$this->assertEquals( '{"success":true}', ob_get_clean() );
$this->assertContains( 'AMP-Redirect-To: https://example.org', xdebug_get_headers() );
}

/**
* Test amp_handle_xhr_request().
*
* @runInSeparateProcess
* @preserveGlobalState disabled
* @covers amp_handle_xhr_headers_output()
*/
public function test_amp_handle_xhr_request() {
global $pagenow;
if ( ! function_exists( 'xdebug_get_headers' ) ) {
$this->markTestSkipped( 'xdebug is required for this test' );
}

$_GET['__amp_source_origin'] = 'https://example.org';
$pagenow = 'wp-comments-post.php';

amp_handle_xhr_request();
$this->assertContains( 'AMP-Access-Control-Allow-Source-Origin: https://example.org', xdebug_get_headers() );

}
}