Email Registration breaks commerce panes on old versions of commerce

This seems critical, so bumping that and a patch needs review.

The issue says:

with Commerce 2.11 or earlier

The code does:

+  - drupal:commerce (>=8.x-2.11)

Maybe that should be `(>=8.x-2.12)` or `(>8.x-2.11)` ?

This looks ready to RTBC to me, but might we name the module something more generic like email_registration_commerce? That way any future non-checkout pane integration could live there too?

Thanks for the followup. I added this to #3021912: Plan for creating email registration 8.x-1.0 final stable as it seems important to get done soon. I'd like to get 1.0 done in the next month, for what it's worth.

I could get #6 to apply easily with git apply. However, I've not been able to get it to roll successfully with composer patches (patch). It runs something like:

patch '-p1' --no-backup-if-mismatch < 'email_registration--checkout_pane_submodule--6.patch'

Yields:

patching file modules/commerce/email_registration_commerce.info.yml
patching file modules/commerce/email_registration_commerce.module

It creates those two files, but that's it. I'm sure I'm just doing something goofy, but it would be good to make sure.

Also, I think the namespaces of the two plugins could be adjusted from:

Drupal\email_registration\Plugin\Commerce\CheckoutPane;

to

Drupal\email_registration_commerce\Plugin\Commerce\CheckoutPane;

Otherwise, I think it seems great. Thanks for all the work @rwanth

We should use https://getcomposer.org/doc/04-schema.md#conflict
Like core doing for pathauto https://git.drupalcode.org/project/drupal/blob/8.8.x/core/composer.json#L52

EDIT https://www.drupal.org/project/commerce/releases/8.x-2.12

Extra comma

I really can't get why we need a split for 2 modules?
Summary said the issue because old commerce could try use new code - then right fix in #13

I think let's commit fix for composer and then discuss the split

1. +++ b/modules/commerce/email_registration_commerce.info.yml
   @@ -0,0 +1,7 @@
   +dependencies:
   +  - drupal:email_registration
   +  - drupal:commerce (>=8.x-2.12)
   

   plugins should not require their modules enabled

2. +++ b/modules/commerce/email_registration_commerce.module
   @@ -0,0 +1,6 @@
   + * Contains stub of email_registration_commerce.module.
   

   .module is not required in d8

3. +++ b/modules/commerce/src/Plugin/Commerce/CheckoutPane/EmailRegistrationLogin.php
   @@ -35,19 +37,19 @@ class EmailRegistrationLogin extends Login {
   -   * @param \Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowInterface $checkout_flow
   +   * @param CheckoutFlowInterface $checkout_flow
   ...
   -   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   * @param EntityTypeManagerInterface $entity_type_manager
   ...
   -   * @param \Drupal\commerce\CredentialsCheckFloodInterface $credentials_check_flood
   +   * @param CredentialsCheckFloodInterface $credentials_check_flood
   ...
   -   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   * @param AccountInterface $current_user
   ...
   -   * @param \Drupal\user\UserAuthInterface $user_auth
   +   * @param UserAuthInterface $user_auth
   ...
   -   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
   +   * @param RequestStack $request_stack
   ...
   -   * @param \Drupal\Core\Config\ImmutableConfig $config
   +   * @param ImmutableConfig $config
   
   +++ b/modules/commerce/tests/src/Functional/Plugin/Commerce/CheckoutPane/EmailRegistrationLoginTest.php
   @@ -17,7 +20,7 @@ class EmailRegistrationLoginTest extends CommerceBrowserTestBase {
   -   * @var \Drupal\commerce_product\Entity\ProductInterface
   +   * @var ProductInterface
   
   @@ -52,7 +56,7 @@ class EmailRegistrationLoginTest extends CommerceBrowserTestBase {
   -    /** @var \Drupal\commerce_product\Entity\ProductInterface $product */
   +    /** @var ProductInterface $product */
   
   @@ -62,19 +66,18 @@ class EmailRegistrationLoginTest extends CommerceBrowserTestBase {
   -    /** @var \Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowInterface $checkout_flow_plugin */
   +    /** @var CheckoutFlowInterface $checkout_flow_plugin */
   ...
   -    /** @var \Drupal\email_registration\Plugin\Commerce\CheckoutPane\EmailRegistrationLogin $pane */
   +    /** @var EmailRegistrationLogin $er_login_pane */
   ...
   -    /** @var \Drupal\commerce_checkout\Plugin\Commerce\CheckoutPane\Login $pane */
   +    /** @var Login $login_pane */
   
   @@ -263,7 +266,7 @@ class EmailRegistrationLoginTest extends CommerceBrowserTestBase {
   -   * @param \Drupal\commerce_product\Entity\ProductInterface $product
   +   * @param ProductInterface $product
   
   @@ -294,7 +297,7 @@ class EmailRegistrationLoginTest extends CommerceBrowserTestBase {
   -   * @param \Drupal\Core\Session\AccountInterface $account
   +   * @param AccountInterface $account
   

   totally wrong change, drupal coding standards require FQDN in doc blocks

Commited https://www.drupal.org/commitlog/commit/4278/07e15557c81b0f96899d3168a85...

So the main question do we really need the split? what are pro and contra

I was a bit confused on the situation, so I just tested it out.

It seems that Email Registration does not depend on commerce directly, i.e. it does not require a site to install commerce. A better title for the issue as rwanth was working on it might be "Email Registration should not affect Commerce unless configured to do so" and then the solution of splitting it to a module or making the behavior configurable might be worthwhile.

Another perspective on this is that email registration should just do what it can (e.g. the composer change andypost made) to prevent breaking older versions of commerce. I think that's the best solution here so I updated the title and status to match.

I am sorry about the bug with the commerce panes in 2.11. I can definitely see how that would be disorienting to have a completely new feature related to commerce that is enabled by default in an RC release. That's something I'll try to avoid in the future, or maybe call out more explicitly in the upgrade notes.

Yep, the whole idea behind d8-plugins is they should not be used without configuration, but module supposed to be pluggable (install/unistall)

So yep, we could make each name/email replacement in forms configurable runtime but it means that some users can bypass module rules and cause a data-mess in names