I don't really know why, but the commerce_customer_profile_manager_validate acts more as a submit than a validate handler in that it is in charge of saving the profiles:


// Notify field widgets to validate their data.
      field_attach_form_validate('commerce_customer_profile', $profile, $element['profiles'][$key], $form_state);

      // TODO: Trap it on error, rebuild the form with error messages.
      // Notify field widgets to save the field data.
      field_attach_submit('commerce_customer_profile', $profile, $element['profiles'][$key], $form_state);

      // Only save if values were actually changed.
      if ($profile != $element['profiles'][$key]['profile']['#value']) {
        commerce_customer_profile_save($profile);
      }

      // Add the profile ID to the current value of the reference field.
      $value[] = array('profile_id' => $profile->profile_id);

I'm guessing there's a reason for this, I should think that the validate handler should be used to validate rather than saving data. Anyways, this has an unfortunate side effect. Whenever you complete an ajax callback validate handlers will be called, and in this case you will save data to the database. Under most circumstances this works okay, but problems starts when a profile isn't created at the time the form is created. The problem is a new profile will be created each time commerce_customer_profile_manager_validate is called. This can be a lot of times, since it's called for every ajax callback, like changing countries on the address field, using the Add another item on a field etc.

The result is that the db is filled with incomplete profile data, that isn't associated to anything.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

googletorp’s picture

Status: Active » Needs review

I've created a patch for this, at my github repo

rszrama’s picture

Will give this a review in a bit. Just to shed some light on the situation in case it affects your patch, the reason we're saving customer profiles (and on the order edit form, line items) in the validate handler is that the validate handler is for the reference field itself and we need to be able to use form_set_value() to reference actual profile IDs. Technically, form_set_value() is supposed to be used during form validation, though it may work fine in submission... but by the time we get to submission, we can't guarantee an execution order that will result in the referenced entities being saved first.

rszrama’s picture

Status: Needs review » Needs work

I read through your patch, and I think I understand the purpose here. However, I'm having a hard time duplicating the problem. Are you saying you've built a custom form that doesn't create a new customer profile when the form is created? Or did you actually run into this problem on a core form?

Also, if I'm not mistaken, this patch won't work for cases where two customer profile reference fields exist on the same page. I believe we have a hard limitation on profile reference fields being single value, but it's worth checking, as I'm guessing this would also fail for multi-value references... the reason is you're using a single static variable in the form state to track your ID. It seems better for this to be based on the field name and delta value.

googletorp’s picture

Status: Needs work » Closed (works as designed)

I have a bit to learn about fields and entities it seems.

I don't know why, but adding validation and field_attach_form_validate has the side effect of adding info about the entity itself when the form is rebuilt. This was the reason why the profiles ids wasn't add to commerce_customer_field_widget_form when it was called. Apparently form validation is critical for AJAX handling.

I'm not thrilled that profiles are saved during validation, but I wouldn't know how to change this and it works, so I'm happy for now.

rszrama’s picture

Ok, well, I'll keep this in the back of my mind, as I'm sure it's going to cause problems eventually. It's probably more of a problem for line items than profiles.

amateescu’s picture

Status: Closed (works as designed) » Active

I just got hit by this bug. Here are the steps to reproduce:

1. Add a Customer profile reference field to the User entity
2. When registering a new account or when a user created before the Customer profile reference field was added, if the user changes the Country value, a new Customer profile is added and it's assigned to Anonymous.

I'll try the patch from #1 in a bit :)

amateescu’s picture

Status: Active » Needs work

Tested the patch from #1 and anonymous user profiles are still created.

johnstav’s picture

subscribe

recidive’s picture

I got a duplicate customer profile, not sure if it's related to ajax.

I added a new field to billing information profile type and edited a few profiles to fill out this field manually.

I ended up with entirely new profiles when changing the field, the one I was editing remained unchanged and kept assigned to the order. I then moved to a default commerce install, and reproduced the problem by just changing the content of the name field. Nothing happen if you don't edit any information.

kotnik’s picture

rszrama’s picture

Revisiting this, but what recidive pointed out is just normal customer profile duplication for profiles that are already referenced by existing orders. Unless you edit those profiles through the order edit form that references them (and that is the only order referencing them), we duplicate the profile so historical order data isn't lost or changed unexpectedly.

I was able to reproduce the behavior using amateescu's instructions, so it appears we need to ensure the customer profile ID is getting saved earlier enough to avoid saving too early or creating duplicates. For reference, on the checkout form it won't save the profile when you change the country, so I'm not entirely sure why it is on the registration form. We'll need to add in a user association like we do for orders, too.

rszrama’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

Well, I was getting very frustrated with this issue when I realized that what I thought was an inconsistency was actually the key to the problem. This behavior wasn't plaguing the checkout form, just the registration form, and I forgot that those forms are built from two different places. The problem here is with any use of the customer profile manager, not just any place we might be embedding a customer profile form, and it turns out we actually handle this properly on the checkout form.

On the checkout form, since we're using button level validate / submit handlers, the checkout pane's validation function is never called nor is its submit function that would save the customer profile. With the manager widget, though, because we use an #element_validate property, it's being called any time it's on a form and an #ajax request happens.

What's interesting about #limit_validation_errors, which I first thought should be preventing this bug, is that it doesn't prevent validation, just suppresses the error messages (as the name of the key implies; it's not about limiting validation, just the error messages). There are certain circumstances in which a form might actually submit even if #limit_validation_errors is used (see forms.inc line 1352), in which case #limit_validation_errors would just be ignored.

What I think we can do here is update commerce_customer_profile_manager_validate() to check to see if:

  1. $form_state['triggering_element'] instructed the form to limit validation errors,
  2. the customer profile reference field wasn't exempted,
  3. and the form isn't being submitted.

If those three conditions are met, we can exit before processing the data / creating a new customer profile if so. We know the form is going to be rebuilt (or if not that the data on it wasn't worth jack anyways), so there's no need to save anything.

Patch attached for test bot consumption.

rszrama’s picture

Status: Needs review » Fixed

Alrighty, committed.

Status: Fixed » Closed (fixed)

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