Handle general form posts by DavidCramer · Pull Request #923 · ampproject/amp-wp

DavidCramer · 2018-02-01T06:07:19Z

Fixes #911

…ted submits.

DavidCramer · 2018-02-01T08:21:41Z

@westonruter I think this is done and ready for you to review.

westonruter · 2018-02-02T00:39:47Z

includes/amp-helper-functions.php

+		add_action( 'template_redirect', function() {
+			/*
+			 * Buffering starts here, so unlikely the form has a redirect,
+			 * so force a redirect to the same page.


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.

@westonruter this is somewhat a hack and will probably have side effects see a9d9bfd

westonruter · 2018-02-02T00:41:37Z

includes/amp-helper-functions.php

+ */
+function amp_handle_general_post( $location ) {
+
+	$url = site_url( $location );


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

@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 :)

westonruter · 2018-02-02T00:44:37Z

includes/sanitizers/class-amp-form-sanitizer.php

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


westonruter · 2018-02-02T00:46:54Z

includes/sanitizers/class-amp-form-sanitizer.php

+	 * @link https://www.ampproject.org/docs/reference/components/amp-form
+	 * @since 0.7
+	 *
+	 * return DOMElement


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

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

@westonruter I really need to focus on phpdoc tags. thanks.

westonruter · 2018-02-02T01:25:22Z

includes/amp-helper-functions.php

 */
-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() {


Let's rename this to amp_handle_xhr_request.

westonruter · 2018-02-02T01:26:20Z

includes/amp-helper-functions.php

+		}, 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;


I suggest inverting the isset check and moving the redirect filter from else to here.

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

@westonruter see 83a0799 does that work?

westonruter · 2018-02-02T01:30:47Z

includes/amp-helper-functions.php

+ * @since 0.7.0
+ * @param string $location The location to redirect to.
+ */
+function amp_handle_general_post( $location ) {


Let's call this amp_intercept_post_request_redirect.

DavidCramer · 2018-02-02T05:41:31Z

@westonruter can review this again when you're ready.

westonruter · 2018-02-02T22:29:44Z

includes/amp-helper-functions.php

+		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.


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.

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

westonruter · 2018-02-02T23:06:07Z

@DavidCramer this is looking good. One thing left is to add unit tests. Please add them specifically for:

amp_intercept_post_request_redirect()
amp_handle_xhr_headers_output()
amp_handle_xhr_request()

Really cool to see that with your changes here, the Jetpack contact form just works.

DavidCramer · 2018-02-03T06:22:20Z

@westonruter I used the jetpack contact form as well as a basic custom made for for testing, so I'm glad they work without effort. Will get onto those unit tests.

DavidCramer · 2018-02-03T06:24:29Z

includes/amp-helper-functions.php

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


Oh nice one!

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

DavidCramer · 2018-02-06T05:55:22Z

@westonruter finally managed to push these unit tests. I'm not that good with unit tests yet, but fortunately @ThierryA lent a hand. You can review this now.

westonruter · 2018-02-07T07:45:49Z

Cool, I haven't used xdebug_get_headers() previously. That's a handy one to know. But is it what requires the use of @runInSeparateProcess? That really slows the tests down so we should try to find an alternative.

DavidCramer · 2018-02-07T07:48:52Z

@westonruter unfortunately @runInSeparateProcess is required. The setup process echos status which then gives you headers already sent causing the test to fail. Generally we would group them and have them as exclusions so you can run them individually. Which is what WordPress does for things like ajax tests.

…missions Handle general form posts

DavidCramer added 5 commits February 1, 2018 08:04

handle general form requests around comment post logic

c229cfc

Merge develop

e68a752

record that action was converted to action-xhr and only handle conver…

96c0bac

…ted submits.

rename function to be less ambiguous

024d4e0

dont add wp_redirect filter on comment-handler

967a8c6

DavidCramer requested a review from westonruter February 1, 2018 06:56

DavidCramer added 3 commits February 1, 2018 10:12

add appending error template for converted forms

4b259df

redirect page if redirect-template hits

ac86bf2

update docs

eca4789

DavidCramer changed the title ~~[WIP] - handle general form posts~~ handle general form posts Feb 1, 2018

westonruter requested changes Feb 2, 2018

View reviewed changes

westonruter changed the title ~~handle general form posts~~ Handle general form posts Feb 2, 2018

DavidCramer added 8 commits February 2, 2018 06:37

merge develop

1cbfff7

rename handler function

9c965ad

simplify handler function

83a0799

correct phpdoc return tag

09de420

update @link with correct url

40934c1

rename redirect function

6dd4e9d

catch data for redirection and reload inclusion hackery

a9d9bfd

check redirect location has a host first

17ab3fd

Allow all formatting tags to be returned in submit error response

92f3a76

westonruter reviewed Feb 2, 2018

View reviewed changes

Remove support for handling non-redirecting post requests, for now

1593f88

westonruter assigned DavidCramer Feb 3, 2018

Ensure relative redirect URLs get made absolute for AMP

3791cd8

westonruter force-pushed the add/911-support-form-post-submissions branch from 3aade9e to 3791cd8 Compare February 3, 2018 01:28

DavidCramer commented Feb 3, 2018

View reviewed changes

westonruter added this to the v0.7 milestone Feb 5, 2018

DavidCramer added 4 commits February 5, 2018 16:14

add process isolation

7a4b398

remove isolation tag

d38f03e

add test for amp_intercept_post_request_redirect

3b3b545

add test for amp_handle_xhr_request

4090e4e

Augment amp_intercept_post_request_redirect test with checking output

ee75de2

westonruter approved these changes Feb 7, 2018

View reviewed changes

westonruter merged commit 79b8b83 into develop Feb 7, 2018

westonruter deleted the add/911-support-form-post-submissions branch February 7, 2018 07:45

kienstra pushed a commit that referenced this pull request Feb 9, 2018

Merge pull request #923 from Automattic/add/911-support-form-post-sub…

a493975

…missions Handle general form posts

westonruter mentioned this pull request Feb 9, 2018

Fix handling of response when it is not HTML #947

Merged

westonruter mentioned this pull request May 26, 2019

Improve form handling when submission handler does no redirection #2425

Merged

9 tasks

westonruter mentioned this pull request Feb 5, 2020

Improve handling of form submissions that redirect and implement form submission proxy #4212

Closed

3 tasks

Conversation

DavidCramer commented Feb 1, 2018 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidCramer commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidCramer commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidCramer commented Feb 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidCramer commented Feb 6, 2018

Uh oh!

westonruter commented Feb 7, 2018

Uh oh!

DavidCramer commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DavidCramer commented Feb 1, 2018 •

edited by westonruter

Loading

westonruter commented Feb 2, 2018 •

edited

Loading