Hi,

when creating a bulk product, I compose SKUs using size and color fields. When I submit the form, I get "This pattern yielded one or more duplicate SKUs, please use a different pattern." error.

Looking into code in commer_bpc.forms.inc file, I've outputed the value of $combinations array in commerce_bpc_create_bulk_form_validate function, just to see what kind of combinations were there (the array output is attached into a file).
At this point, I've seen strange values and I figured out that it could be a localization (multilingual site) problem. Can you confirm that? and how can I fix this problem?

Any help will be appreciated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Status: Active » Postponed (maintainer needs more info)

I don't think this is a multilingual issue ... the structure of the array looks fine (the API docs on the combinations is completely out of date---I apologize), a combination should be a mapping from field names to complete field-api value array (i.e. the actual value for a list field would be at
$combination[field_name][language_code][delta]['value']);

The problem is the content of the array---the first three combinations are all identical:

 array ( 
    'field_tamaina' => array (
      'und' => array ( 
        0 => array ('value' => 'M',), 
      ), 
    ), 
    'field_kolorea' => array ( 
      'und' => array ( 
        0 => array ('value' => 'txuria',), 
      ), 
    ), 
  ), 

Indeed, it seems that every combination was generated three times. Which of course leads to duplicate SKUs.

Can you post the content of the the "Allowed values" setting for both your list fields?

redna’s picture

There are 4 different sizes (M,L,XL,XXL) and 5 different colors (gorria, urdina, berdia, txuria, beltza) in that product type.

But for this product there are 4 different sizes and just one color (txuria) so it was supposed to generate only 4 combinations.

sven.lauer’s picture

Hm, I cannot reproduce the problem.

Can you tell me
- What version of core you are using?
- What version of Commerce?
- I assume you are using the alpha1 of bpc?

Ideally: Can you try to reproduce the problem on a clean install and tell me the steps you took, in order?

I also wonder where the factor three comes from (given that you have 2 fields, with 4 and 5 options and 4 and 1 selected option ...)

sven.lauer’s picture

Also, could you perhaps put a

debug($form_state['values']['combinations']);
debug($form['combinations']);

At the top of the declaration of commerce_bpc_create_bulk_form_validate() (in commerce_bpc.forms.inc) and attach the output here?

Pondering this, with the current code, this could only happen (I think) if the values given by the
radio buttons for kolorea (Basque?) are, for some reason, repeated three times---as if you had
selected the value "txuria" three times (which is not even possible ...).

redna’s picture

FileSize
84.46 KB

Hi,

I attach the output generared by the debug statements and the generated SKUs.

Looking at $form_state['values']['combinations'] it look like data is ok. For $form['combinations'] I'm not able to understand all it's values for now ;) but I'll try through the morning.

redna’s picture

I've looked at the value of $form_state['commerce_bpc'] and I've noticed that there were repeated values:

array (
  'list' => 
  array (
    'combinations' => 
    array (
      0 => 'field_tamaina',
      1 => 'field_kolorea',
      2 => 'field_tamaina',
      3 => 'field_kolorea',
    ),
    'combination_fields' => 
    array (
      0 => 'field_tamaina',
      1 => 'field_kolorea',
      2 => 'field_tamaina',
      3 => 'field_kolorea',
    ),
  ),
)

So I've searched where $form_state['commerce_bpc'] was set and I've made the following change so now the same value is only added once to the array:

--- commerce_bpc.hooks_list.inc 2011-08-01 11:19:25.472157367 +0200
+++ commerce_bpc/commerce_bpc.hooks_list.inc 2011-07-23 08:35:36.000000000 +0200
@@ -18,14 +18,12 @@
unset( $element[$lang]['#options']['_none']);
// Move to comibinations-fieldset
$form['combinations'][$key] = $element;
- if(! in_array($key,$form_state['commerce_bpc']['list']['combinations'])){
$form_state['commerce_bpc']['list']['combinations'][] = $key;
+ unset($form['static_values'][$key]);

// record what we have done
$form_state['commerce_bpc']['list']['combination_fields'][] = $element[$lang]['#field_name'];
}
- unset($form['static_values'][$key]);
- }
}
/**
* Implements hook_commmerce_bpc_get_combinations().

Now I can succesfully create bulk products. Tell me if it is the right place to change the code.

redna’s picture

Due to extra debug information on my screen I haven't noticed that the previous change outputs some warnings. To avoid those warnings this might be better:

--- commerce_bpc.hooks_list.inc	2011-08-01 12:19:16.000000000 +0200
+++ commerce_bpc/commerce_bpc.hooks_list.inc	2011-07-23 08:35:36.000000000 +0200
@@ -18,14 +18,12 @@
     unset( $element[$lang]['#options']['_none']);
     // Move to comibinations-fieldset
     $form['combinations'][$key] = $element;
-    if(!isset($form_state['commerce_bpc']) || !in_array($key,$form_state['commerce_bpc']['list']['combinations'])){
 		$form_state['commerce_bpc']['list']['combinations'][] = $key;
+    unset($form['static_values'][$key]);
 
 		// record what we have done
 		$form_state['commerce_bpc']['list']['combination_fields'][] = $element[$lang]['#field_name'];
 	}
-    unset($form['static_values'][$key]);
-  }
 }
 /**
  * Implements hook_commmerce_bpc_get_combinations().
sven.lauer’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
664 bytes

Thanks for the detective work!

Indeed, I see how the repetition you found would result in duplication of combinations.

However, while something like your change probably should be made as a failsafe measure, it does not really address the problem---for some reason, this hook gets called twice on each field in your setup. I'd like to get to the bottom of this and will leave the issue open for now ...

Could you apply the attached patch (against alpha1, assuming you are still using that) and post the debug output? The patch only adds a debug statement and can/should be reverted afterwards.

Sorry to keep asking you this, but as I cannot reproduce the problem ...

redna’s picture

FileSize
275 bytes

Hi,

I'm glad on helping you (and other), because I find this module very useful.

Well, I've done what you said (undo my changes to match alpha1 and add the debug line) and I think I've found the origin of the problem 'by surprise'. Let me explain:

After preparing the scenario, I went to create a bulk product and see what the debug line produced. I didn't care on what product I was creating because I thought that the duplicated SKUs error would raise and the products were not being created. But, for my surprise, the products were successfully created.

After this, I tryed to create another bulk product taking care of the values I was entering. At this time, the error raised, I was unable to create the products.

And what was the main difference between the two process: I added an image to the second bulk product. I tryed again without images and the products were sucessfully created.

So, at this point I can say that a image field using 'Media file selector' widget is creating the problem. Try it by yourself and confirm that you can also reproduce this behaviour.

I attach you also, the output for the debug line.

Hope all this helps

sven.lauer’s picture

Hm, sadly, I still cannot reproduce the problem. But I think we are getting closer, now that we've found a way to selectively produce the issue on your system.

A question: On the two runs where the problem did not occur first and then occurred again, did you:
(a) use two distinct product types, one without an image field one with an image field OR
(b) use the same product type, but did not supply a value in the first run but did supply one in the second run?

Another thing: As you may have guessed, I screwed up when I rolled the debug patch: The line should have been debug($form['static_values']);. Could you change the line and run it again? I could roll another patch if it is too much of a hassle to do this by hand.

redna’s picture

Hi,

let's go with more information.

A question: On the two runs where the problem did not occur first and then occurred again, did you:
(a) use two distinct product types, one without an image field one with an image field OR
(b) use the same product type, but did not supply a value in the first run but did supply one in the second run?

Answer B : i used the same product type but I did not suply a value in the first run but I did supply it in the second run.

Well, now, running new tests, I've realized that the problem comes when you supply more than one image into the image field. I supply two pictures for every product (T-shirt front side and T-shirt back side) so when I said that I was inserting an image (just one field in the form) I didn't realized that I was really inserting two images (sorry :( )

The 'Media file selector' widget provides "Add media" and "Add another item" buttons. And it looks like the "Add another item" button does some kind of (underliying?) form reloading when you press it: there is no need to select an image, just pressing the button the error will raise.

So, try inserting two pictures via 'media file selector' to see if you can reproduce the error

And here goes the debug line output

array (
  '#type' => 'fieldset',
  '#title' => 'Static values',
  '#description' => 'The values of these fields will be shared by all generated products.',
  '#collapsible' => true,
  '#collapsed' => false,
  '#tree' => true,
  '#parents' => 
  array (
    0 => 'static_values',
  ),
  'commerce_price' => 
  array (
    '#type' => 'container',
    '#attributes' => 
    array (
      'class' => 
      array (
        0 => 'field-type-commerce-price',
        1 => 'field-name-commerce-price',
        2 => 'field-widget-commerce-price-full',
      ),
    ),
    '#weight' => '0',
    '#tree' => true,
    '#language' => 'und',
    'und' => 
    array (
      0 => 
      array (
        '#entity_type' => 'commerce_product',
        '#bundle' => 'txirrindularitza_erropa',
        '#field_name' => 'commerce_price',
        '#language' => 'und',
        '#field_parents' => 
        array (
          0 => 'static_values',
        ),
        '#columns' => 
        array (
          0 => 'amount',
          1 => 'currency_code',
          2 => 'data',
        ),
        '#title' => 'Price',
        '#description' => '',
        '#required' => true,
        '#delta' => 0,
        '#weight' => 0,
        '#attached' => 
        array (
          'css' => 
          array (
            0 => 'sites/all/modules/commerce/modules/price/theme/commerce_price.css',
          ),
        ),
        'amount' => 
        array (
          '#type' => 'textfield',
          '#title' => 'Price',
          '#default_value' => NULL,
          '#size' => 10,
          '#prefix' => '<div class="commerce-price-full">',
        ),
        'currency_code' => 
        array (
          '#type' => 'select',
          '#description' => '',
          '#options' => 
          array (
            'EUR' => 'EUR',
            'USD' => 'USD',
          ),
          '#default_value' => 'EUR',
          '#suffix' => '</div>',
        ),
        'data' => 
        array (
          '#type' => 'value',
          '#default_value' => 
          array (
            'components' => 
            array (
            ),
          ),
        ),
        '#element_validate' => 
        array (
          0 => 'commerce_price_field_widget_validate',
          1 => 'commerce_tax_price_field_validate',
        ),
      ),
      '#theme' => 'field_multiple_value_form',
      '#field_name' => 'commerce_price',
      '#cardinality' => '1',
      '#title' => 'Price',
      '#required' => true,
      '#description' => '',
      '#prefix' => '<div id="static-values-commerce-price-add-more-wrapper">',
      '#suffix' => '</div>',
      '#max_delta' => 0,
      '#after_build' => 
      array (
        0 => 'field_form_element_after_build',
      ),
      '#language' => 'und',
      '#field_parents' => 
      array (
        0 => 'static_values',
      ),
    ),
  ),
  'field_gallery' => 
  array (
    '#type' => 'container',
    '#attributes' => 
    array (
      'class' => 
      array (
        0 => 'field-type-image',
        1 => 'field-name-field-gallery',
        2 => 'field-widget-media-generic',
      ),
    ),
    '#weight' => '37',
    '#tree' => true,
    '#language' => 'und',
    'und' => 
    array (
      0 => 
      array (
        '#entity_type' => 'commerce_product',
        '#bundle' => 'txirrindularitza_erropa',
        '#field_name' => 'field_gallery',
        '#language' => 'und',
        '#field_parents' => 
        array (
          0 => 'static_values',
        ),
        '#columns' => 
        array (
          0 => 'fid',
          1 => 'alt',
          2 => 'title',
        ),
        '#title' => '',
        '#description' => '',
        '#required' => false,
        '#delta' => 0,
        '#weight' => 0,
        '#type' => 'media',
        '#collapsed' => true,
        '#default_value' => 
        array (
        ),
        '#media_options' => 
        array (
          'global' => 
          array (
            'types' => 
            array (
              0 => 'image',
            ),
            'schemes' => 
            array (
              0 => 'public',
              1 => 'private',
            ),
          ),
        ),
        '_weight' => 
        array (
          '#type' => 'weight',
          '#title' => 'Weight for row 1',
          '#title_display' => 'invisible',
          '#delta' => 0,
          '#default_value' => 0,
          '#weight' => 100,
        ),
      ),
      '#theme' => 'field_multiple_value_form',
      '#field_name' => 'field_gallery',
      '#cardinality' => '-1',
      '#title' => 'Irudia',
      '#required' => 0,
      '#description' => '',
      '#prefix' => '<div id="static-values-field-gallery-add-more-wrapper">',
      '#suffix' => '</div>',
      '#max_delta' => 0,
      'add_more' => 
      array (
        '#type' => 'submit',
        '#name' => 'static_values_field_gallery_add_more',
        '#value' => 'Add another item',
        '#attributes' => 
        array (
          'class' => 
          array (
            0 => 'field-add-more-submit',
          ),
        ),
        '#limit_validation_errors' => 
        array (
          0 => 
          array (
            0 => 'static_values',
            1 => 'field_gallery',
            2 => 'und',
          ),
        ),
        '#submit' => 
        array (
          0 => 'field_add_more_submit',
        ),
        '#ajax' => 
        array (
          'callback' => 'field_add_more_js',
          'wrapper' => 'static-values-field-gallery-add-more-wrapper',
          'effect' => 'fade',
        ),
      ),
      '#after_build' => 
      array (
        0 => 'field_form_element_after_build',
      ),
      '#language' => 'und',
      '#field_parents' => 
      array (
        0 => 'static_values',
      ),
    ),
  ),
  'field_tamaina' => 
  array (
    '#type' => 'container',
    '#attributes' => 
    array (
      'class' => 
      array (
        0 => 'field-type-list-text',
        1 => 'field-name-field-tamaina',
        2 => 'field-widget-options-buttons',
      ),
    ),
    '#weight' => '39',
    '#tree' => true,
    '#language' => 'und',
    'und' => 
    array (
      '#entity_type' => 'commerce_product',
      '#bundle' => 'txirrindularitza_erropa',
      '#field_name' => 'field_tamaina',
      '#language' => 'und',
      '#field_parents' => 
      array (
        0 => 'static_values',
      ),
      '#columns' => 
      array (
        0 => 'value',
      ),
      '#title' => 'Tamaina',
      '#description' => '',
      '#required' => true,
      '#delta' => 0,
      '#type' => 'checkboxes',
      '#default_value' => 
      array (
      ),
      '#options' => 
      array (
        'S' => 'S',
        'M' => 'M',
        'L' => 'L',
        'XL' => 'XL',
        'XXL' => 'XXL',
      ),
      '#value_key' => 'value',
      '#element_validate' => 
      array (
        0 => 'options_field_widget_validate',
      ),
      '#properties' => 
      array (
        'filter_xss' => true,
        'strip_tags' => false,
        'empty_option' => false,
        'optgroups' => false,
      ),
      '#after_build' => 
      array (
        0 => 'field_form_element_after_build',
      ),
    ),
  ),
  'field_kolorea' => 
  array (
    '#type' => 'container',
    '#attributes' => 
    array (
      'class' => 
      array (
        0 => 'field-type-list-text',
        1 => 'field-name-field-kolorea',
        2 => 'field-widget-options-buttons',
      ),
    ),
    '#weight' => '41',
    '#tree' => true,
    '#language' => 'und',
    'und' => 
    array (
      '#entity_type' => 'commerce_product',
      '#bundle' => 'txirrindularitza_erropa',
      '#field_name' => 'field_kolorea',
      '#language' => 'und',
      '#field_parents' => 
      array (
        0 => 'static_values',
      ),
      '#columns' => 
      array (
        0 => 'value',
      ),
      '#title' => 'Kolorea',
      '#description' => '',
      '#required' => false,
      '#delta' => 0,
      '#type' => 'checkboxes',
      '#default_value' => 
      array (
      ),
      '#options' => 
      array (
        'gorria' => 'gorria',
        'berdea' => 'berdea',
        'txuria' => 'txuria',
        'beltza' => 'beltza',
      ),
      '#value_key' => 'value',
      '#element_validate' => 
      array (
        0 => 'options_field_widget_validate',
      ),
      '#properties' => 
      array (
        'filter_xss' => true,
        'strip_tags' => false,
        'empty_option' => false,
        'optgroups' => false,
      ),
      '#after_build' => 
      array (
        0 => 'field_form_element_after_build',
      ),
    ),
  ),
  '#pre_render' => 
  array (
    0 => '_field_extra_fields_pre_render',
  ),
  '#entity_type' => 'commerce_product',
  '#bundle' => 'txirrindularitza_erropa',
)
sven.lauer’s picture

Assigned: Unassigned » sven.lauer

Aha! When I read your post, it immediately clicked. The problem is not the media module: You can reproduce the problem with any multi-value field.

The problem is that additional fields are loaded via ajax ... and in fact after every ajax-request, the form is being rebuilt (as the ajax-call submits the whole form, so it must be rebuilt in order to submit the form again later). Which means that the hook gets called again.

So, indeed, your fix above is exactly what should happen. The hook surely should run again if the form is being rebuilt, but it needs to take care to not log its activity twice.

Will roll a patch later today. Good work! This is a very general bug that surely would have bitten a large number of people!

sven.lauer’s picture

Assigned: sven.lauer » Unassigned
Status: Active » Fixed

Fixed!

Thanks again, redna, for you patient help!

Status: Fixed » Closed (fixed)

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