-
Notifications
You must be signed in to change notification settings - Fork 812
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
Forms: Fix form submissions across pages that have pagination applied #41407
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
|
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 duplicated the original issue on trunk, and confirmed that the issue is fully resolved with this PR. Forms on paginated posts now submit accurately.
The code changes seem to make sense. Added a few notes. I still lack some context on the surrounding code and logic, and was trying to think through backwards compatibility issues or edge cases in how pagination works. I can't identify specific concerns other than those I've already commented on.
ffb899a
to
09f25e0
Compare
@edanzer Can you take another look? I addressed all the issue you found. :) |
@@ -774,7 +780,7 @@ public function grunion_manage_post_column_response( $post ) { | |||
echo '<div class="feedback_response__item-value">' . esc_html( $content_fields['_feedback_ip'] ) . '</div>'; | |||
} | |||
echo '<div class="feedback_response__item-key">' . esc_html__( 'Source', 'jetpack-forms' ) . '</div>'; | |||
echo '<div class="feedback_response__item-value"><a href="' . esc_url( get_permalink( $post->post_parent ) ) . '" target="_blank" rel="noopener noreferrer">' . esc_html( get_permalink( $post->post_parent ) ) . '</a></div>'; | |||
echo '<div class="feedback_response__item-value"><a href="' . esc_url( $url ) . '" target="_blank" rel="noopener noreferrer">' . esc_html( $url ) . '</a></div>'; |
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 think changes in this file are new since last time I reviewed. Good to include. Links under Feedback now link to the correct page where the form submission originated.
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.
Retested and still working great. Issues addressed. Good addition to update links in the Feedback list so they go to right page.
@@ -123,7 +123,7 @@ public function __construct( $attributes, $content = null ) { | |||
if ( ! isset( $attributes['id'] ) ) { | |||
$attributes['id'] = ''; | |||
} | |||
$attributes['id'] = $attributes['id'] . '-' . ( count( self::$forms ) + 1 ); | |||
$attributes['id'] = $attributes['id'] . '-' . ( count( self::$forms ) + 1 ) . '-' . $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.
page isn't an object here? should we use https://developer.wordpress.org/reference/functions/get_the_id/
instead?
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.
$page
is a WP global that is a number and tells WP that page to show.
This Pr adds support for form processing across multiple pages
09f25e0
to
1f84eb8
Compare
rebased |
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.
Did a basic test, and this seems to work well:
- Two forms separated by a page break
- Two forms on a single page without page break
- Three forms, first and 2nd separated by page break
Everything submits correctly and multiple forms appearing on the same page get individual IDs.
I noticed that when two forms are separated by page break, first from on both pages gets ID by page ID id="contact-form-950"
, i.e. there are two forms with the same ID on the "same page", just separated by the page break. Not an issue now and things still work as we intended, but this just highlights how we need to later work on true unique IDs.
Correct source URL shows up in the email as well:

Good catch on the |
@simison Can you take another look when you have the chance. Thank you! |
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.
Tests well now!
Fixes #41334
Currently if you create a page and split it up using the block or code.
Forms on pages other then the first one will not work. See issue #41334.
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
no
Testing instructions: