Commerce_profile_select reusable profiles

Yes, this looks right. But I think we could postpone until the dependency lands.

ShippingProfileWidget needs to change too.

The ProfileSelect RenderElement does not properly select the initial profile with #5. ProfileSelect handles creating a new profile if necessary, so the shipping pane only needs to provide a profile if one was selected.

Use the patch here in combo with #2951637: Provide more context to initial values event to pre-seed the values on the shipping form from the billing address. If the shipping form is on a separate page than the billing details.

I also re-added the snippet removed in #6. We cannot be sure that the billing profile is available if the billing profile and the shipping profile are on different pages.

@heddn

The patch from #7 doesn't work with 8.x-2.0-beta4, I get unexpected error when I proceed to checkout. I applied also patch for Address. Should I install dev version of shipping?

@heddn
I installed dev version of shipping, still the same error when proceeding to checkout forms.

@jakub_89
There are two patches that are required before using the patch in this issue.
Address: https://www.drupal.org/project/address/issues/2951637#comment-12518591
Profile Select: https://www.drupal.org/project/commerce/issues/2844920#comment-12518712

I imagine the reason you are getting the error is because you don't have the Profile Select patch. If this doesn't fix your error, make sure you have error messages turned on in admin/config/development/logging so that we have a better idea of what the error is.

@ericchew
Hi, I followed your advice and tried to apply 2844920-156.patch
However, it failed to apply.

Here is error message:

error: while searching for:
    }
    else {
      $store = $this->order->getStore();
      $billing_profile = $this->order->getBillingProfile();
      if (!$billing_profile) {
        $billing_profile = $this->entityTypeManager->getStorage('profile')->create([
          'uid' => $this->order->getCustomerId(),
          'type' => 'customer',
        ]);
      }

      $pane_form['billing_information'] = [
        '#type' => 'commerce_profile_select',
        '#default_value' => $billing_profile,
        '#default_country' => $store->getAddress()->getCountryCode(),
        '#available_countries' => $store->getBillingCountries(),
      ];

error: patch failed: modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php:138
error: modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentInformation.php: patch does not apply
Checking patch modules/payment/src/PluginForm/PaymentMethodAddForm.php...

When I apply this patch and want to checkout, I get the error:

InvalidArgumentException: The commerce_profile_select element requires the #default_value property. in Drupal\commerce_order\Element\ProfileSelect::processForm() (line 80 of /var/www/html/web/modules/contrib/commerce/modules/order/src/Element/ProfileSelect.php).

I guess the error is related to the patch from:
Profile Select: https://www.drupal.org/project/commerce/issues/2844920#comment-12518712

Would be nice to get this working. Customer with an account get really frustrated to fill in their details every time (twice! shipping and billing)

The patches are working for me together without issue as far as I can tell. Here are the relevant bits I've got in composer.json:

"drupal/commerce": {
    "#2844920: Profile select": "https://www.drupal.org/files/issues/2018-03-09/2844920-156.patch"
},
"drupal/commerce_shipping": {
    "#2948954: Commerce_profile_select reusable profiles": "https://www.drupal.org/files/issues/2018-03-09/2948954-7.patch"
}

Is this the same combination of patches you're using where you're running into that error?

I experience one problem with the latest patch here. Everything technically works fine, but we have shipping on a separate step before billing, and the first time you try to check out, it pre-selects a profile but doesn't actually render the profile or the Edit button for it. However, if you switch to another profile, and optionally then switch back to the initial one, it all shows up fine.

Additionally, if I move on to the next step and then return to the shipping step, the selected profile renders fine.

Is this because there's no billing profile so technically the profile has no default value, even though it pre-selects the first option in the select list?

Edit: No, I don't think that's the issue. I do see that in buildPaneForm(), the initial $form_state->get('shipping_profile') returns nothing, but then a shipping profile is set immediately after that.

Edit 2: This seems to be because the shipping profile is considered "new" even though an existing one is selected, so the profile select element when it is rendered renders as if it's a new profile (e.g. nothing to render, and no Edit button). Not sure yet what the solution is, still troubleshooting.

Edit 3: In ShippingInformation.php, in the getShippingProfile method, the first time checkout loads it is always creating a new customer profile, even though it does in fact seem to actually select an existing profile if one exists. Shouldn't it try to look up an existing user profile before creating a new one?

Edit 4: Found a fix based on my previous findings. Before creating a new profile, I have it looking up the first available profile for the current user if one exists. If not (or if anonymous), it falls back to the existing behavior. I'll post a patch shortly.

Here's a patch with interdiff of what I added to fix the issue I just reported.

Actually, the issue that I had which caused me to need the last patch I made could be caused by something else. I'm also finding that, for me, anonymous users can't calculate shipping because the profile info they enter seems to not be available during shipping calculation. This makes me think the same thing is happening to the initial load of logged-in users, which is what's causing that final conditional to create a new profile during the shipping calculation just like it does for anonymous users. My last patch seems to just be a workaround for whatever is actually causing this bug for me, and the workaround doesn't work for anonymous users.

I'm still trying to figure out what's going on. I'm not sure if it's even related to this patch ultimately or if I have some other issue entirely that's causing this behavior with this functionality.

Edit: The problem disappears when I remove both this patch and the main commerce profile select patch, so it's one of the two that are causing the issue. But this patch is so simple that I'm assuming it's the other profile select patch.

@bmcclure Yeah I'm also seeing this issue you describe in #14.

For my use case we don't have anonymous checkouts so I'm going to use your patch, but if you ever get to the bottom of it please post here (and I'll do the same!)

Thanks for confirming @orphans! It's great to know this isn't some hidden conflict with some of our custom code on our site at least.

We do need anonymous checkout on our site, so I've been attempting to find a proper solution, but so far haven't been successful. As far as I can tell, it seems like for an anonymous user, when they submit their initial shipping details and attempt to calculate shipping, the getShippingProfile() method ends up getting called several times, and always ends up with a blank profile the first time.

Without my last patch on here, this issue partially applies to authenticated users, too--when the shipping information form first loads, it selects the first profile in the select list, but the form state acts as if no profile is selected (the profile is not rendered and there's no Edit button) until you switch profiles at least once.

I'm wondering if this is caused by the form state being rebuilt on the initial submission causing it to revert to the default (or maybe an empty) value.

The FormStateInterface itself has the submitted values in it (or a reference to the correct profile ID for authenticated users), but the profile entity associated with it seems to have been lost on that request.

I might see if I can just create a new profile or reference the correct existing profile manually in this case for now, until a proper solution can be found. If I do figure out a way to do that, I'll post a temporary patch for anyone else running into this.

I think this fixes up the logic flaw in #7.

And even more better.

In general, I like the way this patch is going (other than the bug with initial profile selection I mentioned above). However, I'm not sure that I understand the logic in hard-coding it to get the billing profile in order to pre-populate the shipping profile. In many (maybe most?) cases, the shipping information in Commerce 2.x is entered before the billing information, because it seems counter-intuitive to start entering payment details before you know how much you're paying for shipping (but also simply because that's the default order).

A more logical progression in my mind, if it has to be hard-coded, would be to make it so the shipping information pre-populated the billing profile. However, since I know there are some people (most likely you, @heddn) who prefer the billing information come first, that wouldn't work for all cases either. Maybe there could instead be an option in both the shipping and billing panes whether you want to copy profile data from the other pane?

I suppose this will be moot once this functionality is included with Commerce anyway, but since I'm not sure what that functionality will look like yet, this is still quite relevant to one of our websites which uses it.

This is a side tangent, but it seems like it might be useful for Commerce to have a built-in concept of multiple keyed profiles being attached to an order, so that commerce core and external contrib modules could all make use of available profiles on an order in a standard way without needing to use special module-specific methods for retrieving each profile. But anyway, I'm going back to troubleshooting this issue now!

Just to verify, I tested the latest patch and it still has the issue I mentioned in comments 14-16. I'm still not positive whether this issue is related to this patch or to the larger profile select patch for commerce itself, but just wanted to mention that it is still an issue.

Also, with the latest versions of this patch, I'm discovering that when I'm an authenticated user, the correct default profile is selected in the dropdown, but the profile that's rendered is actually for a different user and isn't even associated with the account I'm logged in as. I haven't discovered why that is yet as I'm still troubleshooting the other issue. Perhaps because I'm logged in as admin, it's just grabbing the most recent created profile by anyone?

This patch (with the related patch) is causing issues for anonymous users and new accounts.

See: https://www.drupal.org/project/commerce_shipping/issues/2971164

Shipping methods with condition per shipping address (country in my case) are hidden during anonymous checkout or for checkout with account made during that checkout (in step 1). It only works for logged in users.

When you continue anonymous checkout to the review step, and then click on edit shipping information (you go back to the previous step), you will see the shipping methods available for the selected country.

Also: "shipping same as billing" doesn't work. (https://www.drupal.org/project/commerce_shipping/issues/2852207#comment-...)

This should fix up the bug mentioned in #25. Thanks for such well documented steps to reproduce. It made it much easier to find the issue and fix it.

I've tried the patch #26 in conjunction with the patch in https://www.drupal.org/project/commerce/issues/2844920 (comment #209).

I'm still seeing the "An illegal choice has been detected" error when no profile is existing on an account. This appears when clicking calculate shipping or "Continue to review" at the bottom.

EDIT: I see that this error is caused by no radio buttons being available for the shipping method. Setting "Hide shipping options until address is entered" resolves the issue for me.

I take it then that #27 is more of a works by design, yes? Not sure if I should try to troubleshoot this or not.

Well if these patches force the option to be selected, I guess they should either;

- Handle the case where the 'Hide shipping options' might be unticked (even if that just means improving the error shown).
- Remove the pane option entirely and set to always be hidden until an address is entered

Any thoughts?

Thanks for your work everyone.

After updating to Commerce 2.9 and Commerce Shipping beta5 the latest patch (26) is giving me the following error:

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">InvalidArgumentException</em>: The commerce_profile_select element requires the #default_value property. in <em class="placeholder">Drupal\commerce_order\Element\ProfileSelect::validateElementProperties()</em> (line <em class="placeholder">101</em> of <em class="placeholder">modules/contrib/commerce/modules/order/src/Element/ProfileSelect.php</em>). <pre class="backtrace">Drupal\commerce_order\Element\ProfileSelect::processElement(Array, Object, Array)

It appears to be because the '#default_value' has been removed from the $pane_form['shipping_profile'].

I've attached a re-rolled patch to restore this property.

Whoops! Try this patch/interdiff.

Hi,

I think this patch has a fairly critical security issue. This part assumes that profile_id and revision_id are identical. If they differ then you get contamination of the checkout pages with another customer's profile data.

+    // If we still don't have a profile, assign the default profile with
+    // fallback to most recently created.
+    if (empty($shipping_profile) && \Drupal::currentUser()->isAuthenticated()) {
+      $query = $this->entityTypeManager
+        ->getStorage('profile')
+        ->getQuery();
+      $result = $query
+        ->condition('uid', \Drupal::currentUser()->id())
+        ->sort('is_default')
+        ->sort('created', 'DESC')
+        ->execute();
+      if ($result) {
+        $shipping_profile = $this->entityTypeManager
+          ->getStorage('profile')
+          ->loadRevision(reset($result)); // <- This assumes profile_id and revision_id are equal!!
+      }
+    }

This should probably be changed as follows:

+    // If we still don't have a profile, assign the default profile with
+    // fallback to most recently created.
+    if (empty($shipping_profile) && \Drupal::currentUser()->isAuthenticated()) {
+      $query = $this->entityTypeManager
+        ->getStorage('profile')
+        ->getQuery();
+      $result = $query
+        ->condition('uid', \Drupal::currentUser()->id())
+        ->sort('is_default')
+        ->sort('created', 'DESC')
+        ->execute();
+      if ($result) {
+        $keys = array_keys($result);
+        $shipping_profile = $this->entityTypeManager
+          ->getStorage('profile')
+          ->loadRevision(reset($keys));
+      }
+    }

Attached is for the latest version of the patch upstream. The patch in #2844920-243: Allow customer profiles to be reused doesn't work with #2849756: Auto-recalculate shipping when the address changes.

@heddn,

Assuming #35 is to work in accord with the patch in https://www.drupal.org/project/commerce/issues/2844920,
I have done a lot of testing and I am seeing the following issue.
If you update the Shipping Address, or change it do a different profile, this update is not reflected on the Shipment entity itself.

This means, whenever the getShippingProfile() is called on the Shipment (e.g. when getting Shipping Rates), it uses incorrect Address Information. This also means that for Users that do not have a profile yet, it is basically impossible for them to get Shipping Rates.

What are your thoughts?

It was actually this patch that I was missing, in case anyone else has this problem.
https://www.drupal.org/project/commerce_shipping/issues/2988710

Could we close this issue in favor of https://www.drupal.org/project/commerce_shipping/issues/3054351?

Commerce -dev and shipping -dev have a functional address book, this issue is now outdated.