Skip to content

Conversation

@haringsrob
Copy link
Contributor

@haringsrob haringsrob commented May 9, 2016

This is a proposal to have the registration form available on the login step.

I added an extra option to the pane config, so that the site builder can still choose not to show the form.

I also added the user_login hook, to redirect after the actual registration.

$this->credentialsCheckFlood = $credentials_check_flood;
$this->currentUser = $current_user;
$this->entityTypeManager = $entity_type_manager;
$this->entityManager = $entity_manager;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why inject the deprecated entityManager when you already have the non-deprecated entityTypeManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change this, after I created this, I worked on an issue about the same in core.. But I forgot I used it here.

@bojanz
Copy link
Contributor

bojanz commented May 9, 2016

Thanks for getting this rolling! The embedding of the entire existing registration form looks a bit messy, it would probably be better to copy over the AccountForm / RegistrationForm logic (but without the "needs admin approval" and the pass reset / preferred language stuff)

@haringsrob
Copy link
Contributor Author

Bojanz:

I wanted to do that, but I feel that that would not be the correct way to go. What if extra other modules modify the registration form? We would then make it impossible to use them? Or am I missing something?

@haringsrob
Copy link
Contributor Author

Also, If we keep it in one form, and we would add required fields, they would become part of the same js validation (tried this first), when you then would submit the login form, you would be blocked by the registration fields.

@bojanz
Copy link
Contributor

bojanz commented May 9, 2016

Both of these dilemmas are the same as for login.

You don't mark any field as required, then validate the fields if one of them was provided.
The context is sufficiently different that I'm not worried about the regular alter hooks not working (due to a different form ID).

@haringsrob
Copy link
Contributor Author

@bojanz, if we do modify, then I assume the following fields are enough (no timezone etc,...):
schermafbeelding 2016-05-10 om 07 34 00

@haringsrob
Copy link
Contributor Author

We do need an option to auto generate the username i think, as email + username is not user friendly.

@haringsrob haringsrob force-pushed the 2716241-registration-login-pane branch 2 times, most recently from c0f4538 to 5867fef Compare May 10, 2016 17:25
@haringsrob
Copy link
Contributor Author

@bojanz, updated the PR, waiting for feedback.

'#description' => $this->t('A valid email address. All emails from the system will be sent to this address. The email address is not made public and will only be used if you wish to receive a new password or wish to receive certain news or notifications by email.'),
'#required' => FALSE,
);
$pane_form['register']['pass'] = array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long array usage.

@haringsrob
Copy link
Contributor Author

I'm working on this.

$cart_link->click();
$this->submitForm([], 'Checkout');
$this->assertSession()->pageTextContains('Create new account');
$this->submitForm([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also test validation? Existing email, invalid email

@haringsrob
Copy link
Contributor Author

I think I have covered all the validations now.

@haringsrob haringsrob force-pushed the 2716241-registration-login-pane branch from 76f3e08 to 03472b4 Compare June 23, 2016 15:22
@bojanz bojanz merged commit c787825 into drupalcommerce:8.x-2.x Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants