Problem/Motivation

When the Inline Form Errors core module is enabled, the Commerce Email Registration Login Checkout Pane will result in an error message for the email address field, password field, and the containing fieldset when submitting the form without an email address or with an email address not associated with a user. This is because the $form_state->setError() function is applied to the containing fieldset instead of only the email address field.

This is particularly unsightly because the fieldset error is unstyled since it appears that the Forms API/Inline Form Errors module does not add the appropriate classes to that message for whatever reason.

Proposed resolution

Change the error to use the email address field.

CommentFileSizeAuthor
#6 Screen Shot 2020-02-18 at 8.15.14 PM.png145.06 KBlkacenja
#6 Screen Shot 2020-02-18 at 8.13.20 PM.png155.56 KBlkacenja
#5 3113922-commerce-empty-email-address-5.patch1.84 KBdaniel korte
#2 3113922-commerce-empty-email-address-2.patch1.04 KBdaniel korte

Issue fork email_registration-3113922

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Daniel Korte created an issue. See original summary.

daniel korte’s picture

daniel korte
he/him/his
Primary language English
Location Brooklyn, NY
commented
Assigned: daniel korte » Unassigned
Status: Active » Needs review
StatusFileSize
new 3113922-commerce-empty-email-address-2.patch1.04 KB
lkacenja’s picture

lkacenja
he/him
commented

Does this have any impact on sites not using the Inline Form Errors module? When I have a moment, I will test #2 against our installation.

daniel korte’s picture

daniel korte
he/him/his
Primary language English
Location Brooklyn, NY
commented

No, it is only with the core Inline Form Errors module installed.

daniel korte’s picture

daniel korte
he/him/his
Primary language English
Location Brooklyn, NY
commented
StatusFileSize
new 3113922-commerce-empty-email-address-5.patch1.84 KB

Cleaned up the code a bit to reuse the existing variable.

lkacenja’s picture

lkacenja
he/him
commented
StatusFileSize
new Screen Shot 2020-02-18 at 8.13.20 PM.png155.56 KB
new Screen Shot 2020-02-18 at 8.15.14 PM.png145.06 KB

I believe I was able to replicate the issue. When I have the Inline Form Errors module installed in combination with the 'email_registration_login' pane I get the following outburst of errors:

Email log in with loads of errors

This only occurs when I put something in the email filed, but nothing in the password field and attempt to submit. When I fill in a nonsense email and password, I get a much better result:

Email registration login password filled out

Patch #5 did not seem to fix the first situation for me. Though I may have applied it incorrectly. I will try to dig deeper when I can.

daniel korte’s picture

daniel korte
he/him/his
Primary language English
Location Brooklyn, NY
commented
Title: Submitting an empty Commerce Checkout Login email address field results in too many error messages » Submitting an empty or non-user Commerce Checkout Login email address field results in too many error messages
Issue summary: View changes

The second screenshot is how the form should look like / what the patch is intended to fix. I updated the issue title/description because this does apply to an empty email address field and one with an email address that is not in the users table.

@lkacenja I am seeing the fix applied for all situations described above so I am not sure how you got your results? Are you applying the patch via Composer or git?

lkacenja’s picture

lkacenja
he/him
commented

I see what's happening now. I believe the patch works fine here except for one edge case. If the user is not found via email and proceeds past the empty check at line 114, but $this->config->get('login_with_username') is enabled nothing prevents the parent method validation from firing off. This causes the plethora of errors in my first screenshot above.

  1. In summary to replicate, set login_with_username to true in the UI.
  2. Check out anonymously.
  3. When you make it to the email registration pane, enter only a faulty email address.
  4. Submit the form.

I can see a few options here, but I'm not sure what is best.

  1. Accept the patch the way it is, knowing this kind of extreme edge case will still be an issue. Could possibly follow up in the Commerce queue for a solution in the main login pane. Effectively it seems like the main Login pane would suffer similar Inline Form Error problems. I don't see a ticket covering this over there.
  2. Add some additional handling to the patch to catch this case. The downside is that we will not get all of the extra validation in the main login pane.
  3. Copy more of the error handling from the main commerce login pane. The downside is that our pane will be a lot less DRY.

What do folks think?

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

Same here, furthermore the class "is-invalid-input" is set on the submit button, what doesn't make any sense.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Priority: Normal » Major

Just ran into this issue and this is indeed a major problem. Especially that "is-invalid" class is set on the submit button!

We're going to work on it now.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work
Related issues: +#2561295: One form validation function should be able to cancel other validation steps

@lkacenja: Re #8 I think maybe we need #2561295: One form validation function should be able to cancel other validation steps here to prevent the upstream validations to fire?

So there are still the issues from #8 and class is-invalid-input is set on the input submit, which is wrong.

grevil’s picture

grevil commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Needs work » Postponed (maintainer needs more info)

What is the status for this in 2.x? Can anybody check?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil
Status: Postponed (maintainer needs more info) » Active

@Grevil as of #9 ff please check it next weeks. We ran into this in a customer project (DD)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Grevil opened merge request !23

grevil’s picture

grevil commented
Assigned: grevil » anybody
Status: Active » Needs review

Alright, all fixed now! I agree with @lkacenja in #8, but I don't think this is an extreme edge case as it can occur quite easily, if "login_with_username" is enabled. I fixed the edge case with the second approach of @lkacenja examples in #8:

Add some additional handling to the patch to catch this case. The downside is that we will not get all of the extra validation in the main login pane.

Basically, if "login_with_username" is enabled, but the username couldn't resolve to an existing user, we also throw the error fixing this edge case quite simple! 😊

Please review!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: anybody » Unassigned
Status: Needs review » Reviewed & tested by the community

Changes LGTM and tests are green! Well done all! :)

  • Anybody committed 95e29806 on 8.x-1.x authored by Grevil 
    Issue #3113922 by Grevil, Daniel Korte, lkacenja, Anybody, thomas....
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Reviewed & tested by the community » Fixed

  • Anybody committed d81d9aa5 on 8.x-1.x 
    Revert "Issue #3113922 by Grevil, Daniel Korte, lkacenja, Anybody,...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Accidentally merged into 8.x-1.x-dev as the target branch was wrong. Reverted and cherry-picked into 2.x!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.