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

Conversation

Projects
None yet
2 participants
@DavidCramer
Copy link
Contributor

commented Feb 1, 2018

Fixes #911

@DavidCramer DavidCramer requested a review from westonruter Feb 1, 2018

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

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

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.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 2, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

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

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 2, 2018

Author Contributor

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

$node->setAttribute( 'action-xhr', $action_url );
// Append error handler if not found.
$this->error_handler( $node );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

Very nice.

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

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

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

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

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 2, 2018

Author Contributor

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

@@ -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() {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

Let's rename this to amp_handle_xhr_request.

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

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

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

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 2, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 2, 2018

Author Contributor

@westonruter see 83a0799 does that work?

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

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

Let's call this amp_intercept_post_request_redirect.

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

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2018

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

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 comment has been minimized.

Copy link
@westonruter

westonruter Feb 2, 2018

Member

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.

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 3, 2018

Author Contributor

@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

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

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

@westonruter westonruter force-pushed the add/911-support-form-post-submissions branch from 3aade9e to 3791cd8 Feb 3, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

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

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

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 3, 2018

Author Contributor

Oh nice one!

This comment has been minimized.

Copy link
@DavidCramer

DavidCramer Feb 6, 2018

Author Contributor

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

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

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

@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

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

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.

@westonruter westonruter merged commit 79b8b83 into develop Feb 7, 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/911-support-form-post-submissions branch Feb 7, 2018

@DavidCramer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2018

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.