-
Notifications
You must be signed in to change notification settings - Fork 382
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
Conversation
@westonruter I think this is done and ready for you to review. |
includes/amp-helper-functions.php
Outdated
add_action( 'template_redirect', function() { | ||
/* | ||
* Buffering starts here, so unlikely the form has a redirect, | ||
* so force a redirect to the same page. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
includes/amp-helper-functions.php
Outdated
*/ | ||
function amp_handle_general_post( $location ) { | ||
|
||
$url = site_url( $location ); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
$node->setAttribute( 'action-xhr', $action_url ); | ||
// Append error handler if not found. | ||
$this->error_handler( $node ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice.
* @link https://www.ampproject.org/docs/reference/components/amp-form | ||
* @since 0.7 | ||
* | ||
* return DOMElement |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
includes/amp-helper-functions.php
Outdated
@@ -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() { |
There was a problem hiding this comment.
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
.
includes/amp-helper-functions.php
Outdated
}, 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
includes/amp-helper-functions.php
Outdated
* @since 0.7.0 | ||
* @param string $location The location to redirect to. | ||
*/ | ||
function amp_handle_general_post( $location ) { |
There was a problem hiding this comment.
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
.
@westonruter can review this again when you're ready. |
includes/amp-helper-functions.php
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@DavidCramer this is looking good. One thing left is to add unit tests. Please add them specifically for:
Really cool to see that with your changes here, the Jetpack contact form just works. |
3aade9e
to
3791cd8
Compare
@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' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice one!
There was a problem hiding this comment.
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?
@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. |
Cool, I haven't used |
@westonruter unfortunately |
…missions Handle general form posts
Fixes #911