-
Notifications
You must be signed in to change notification settings - Fork 804
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
Fix form submission with no name but mail #40898
Fix form submission with no name but mail #40898
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. 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so:
Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
@@ -2046,10 +2046,7 @@ public static function parse_fields_from_content( $post_id ) { | |||
if ( str_contains( $content, 'JSON_DATA' ) ) { | |||
$chunks = explode( "\nJSON_DATA", $content ); | |||
$all_values = json_decode( $chunks[1], true ); | |||
if ( is_array( $all_values ) ) { |
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 not related, but I observed that $fields_array
is only used in the else
block.
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.
Nice work!
This now works as expected for me. See
My check though introduces a new edge case where in a form like the one described here, if a user submits an email to both name and email fields, we will not render the name in the success message.
I was able to confirm that this is now an edge case. I don't think we need to solve for it though.
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.
Changes look straightforward and safe, and work as expected. And confirmation looks better without outputting the email as the name.
Agree with @enejb on the edge case. I think it's fine as is.
Lots of e2e testing issues that all seem unrelated?
8574e0d
to
7223022
Compare
Fixes #30456
Proposed changes:
Currently if we have a form with a
name
field and anemail
field where thename
is not required, when we submit the form with an empty name, the success messages shows thename
with theemail
value.This happens because during submission there is code that sets the email as a fallback. That
$comment_author
is later on used in more places (like headers, entity title, etc..) so I didn't make any update there.Instead in the time of rendering the success message I added a check whether name and email have the same values. In that case, we don't render the
name
.Ideally we should check if the original
name
value was provided or not, but I couldn't find a way to get that value and I think it's not stored anywhere else, without having the fallback applied during submission.My check though introduces a new edge case where in a form like the one described here, if a user submits an
email
to bothname and email
fields, we will not render thename
in the success message.I'd consider edge cases both the linked issue and my approach and I'm not sure if it's even worth it to handle this.
Other information:
Testing instructions:
name
field not requiredname
name
is not part of the success messageemail and name
, onlyemail
, etc..).