Currently, commerce_cart_add_to_cart_form is calling private functions from the options module to get the #options (allowed values) of the exposed attributes.
As a consequence, if you alter the options list callback of a field through the hook_entity_property_info_alter(), your callback won't be called and you're gonna get the default options from the original field module.

CommentFileSizeAuthor
#5 commerce_cart-wrong-optionslist-call-1730104-5.patch1022 bytesjsacksick
#3 1730104.better_allowed_values.patch1016 bytesrszrama
#1 commerce_cart-wrong-optionslist-call-1730104-1.patch1.03 KBjsacksick

Comments

jsacksick’s picture

jsacksick commented
Status: Active » Needs review
StatusFileSize
new commerce_cart-wrong-optionslist-call-1730104-1.patch1.03 KB

The proposed patch is using the entity_metadata_wrapper instead. (I'm wondering if we need to update or not the commerce_cart_field_attribute_eligible function).

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

My impression is that we don't need to update that function because every module that deals with options list should have it's own $module_options_list() implementation.

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1730104.better_allowed_values.patch1016 bytes

I'm curious about the comments on this function with respect to how the change will affect multilingual sites. Ostensibly the patch affects the fallback to the default language options with code just above it using _options_prepare_options() to get translated option values. Should that previous code not be using a similar approach, and if we're using the metadata wrapper, would it handle translation on its own removing the need for i18n_field there?

Attached patch removes the $function array as it was a single use throwaway variable that I don't think we need to worry about.

jsacksick’s picture

jsacksick commented

I'm prettty sure we still need the code above, because the optionsList() method of the wrapper is doing more or less the same as _options_prepare_options() method, it calls the options list method of the property, which is defaulted for the fields to entity_metadata_field_options_list (details below) :

/**
 * Callback returning the options list of a field.
 */
function entity_metadata_field_options_list($name, $info) {
  $field_property_info = $info;
  if (is_numeric($name) && isset($info['parent'])) {
    // The options list is to be returned for a single item of a multiple field.
    $field_property_info = $info['parent']->info();
    $name = $field_property_info['name'];
  }
  if (($field = field_info_field($name)) && isset($field_property_info['parent'])) {
    // Retrieve the wrapped entity holding the field.
    $wrapper = $field_property_info['parent'];
    try {
      $entity = $wrapper->value();
    }
    catch (EntityMetadataWrapperException $e) {
      // No data available.
      $entity = NULL;
    }
    $instance = $wrapper->getBundle() ? field_info_instance($wrapper->type(), $name, $wrapper->getBundle()) : NULL;
    return (array) module_invoke($field['module'], 'options_list', $field, $instance, $wrapper->type(), $entity);
  }

I'm just wondering if we should replace optionsList() by optionsList('view') ?

jsacksick’s picture

jsacksick commented
StatusFileSize
new commerce_cart-wrong-optionslist-call-1730104-5.patch1022 bytes

Corrected path with the use of 'view'

rszrama’s picture

rszrama commented
Title: Incorrect options list callback call for the commerce_cart_add_to_cart_form attributes » Use the Entity API property info options list callback for attribute field options
Category: bug » feature

I don't see any harm in this change, but I'm still not sure if we should commit it.

As is, I don't think it addresses the concerns of the OP for multilingual sites - it seems right now if you're using i18n_field(), you're going to get translated options based on the original options list instead of a potentially altered list of options if the entity property info has been changed. However, you're still no worse off, so this isn't really a regression.

I think the rub is that I'm not sure you should be altering the options list callback for these fields to do what you want. One inconsistency that could introduce is the widget for the field having different options on the entity edit forms than you would see in Rules settings forms or on the Add to Cart form. Maybe I just don't understand your use case, though. : ?

Would it not have been better to just alter the form directly to reformat options?

rszrama’s picture

rszrama commented
Status: Needs review » Needs work

Thanks to some work on #1721364: Tags wrongly stripped from options product attribute radio widgets , I also think that this approach (using optionsList instead of _options_get_options()) could leave our widget options unsanitized. Note that directly invoking the list module's hook_options_list() results in a call to list_allowed_values(), which explicitly states the returned values must be run through filter_xss(). Seems to me this patch combined with a radio buttons widget for attribute display would result in unsanitized radio button options.

damien tournoud’s picture

damien tournoud commented

I think it makes sense to use ->optionsList(), and as a consequence support more widgets then just the ones provided by the option module.

Thanks to some work on #1721364: Tags wrongly stripped from options product attribute radio widgets , I also think that this approach (using optionsList instead of _options_get_options()) could leave our widget options unsanitized.

Both ->optionsList() and _options_get_options() provide HTML that needs to be either sanitized (for radio buttons) or downgraded to plaintext on output (for select widgets). This patch doesn't change anything here.

damien tournoud’s picture

damien tournoud commented

If we do that, we should remove all calls to private functions in the options module, and all reference to the module itself. Ryan is right that we need to look into how it affects translatability (especially the code calling i18n_field right below).

rszrama’s picture

rszrama commented

_options_get_options() sanitizes values by passing them through _options_prepare_options() along with the $properties array we pass in. Right now it's hard coded to sanitize all options via the select list's defaults (strip all tags), though that issue I linked is seeking to make it use the other options types of sanitization as well.

What I can't figure out is the wrapper's optionsList() function actually is returning sanitized allowed values - but I don't see where that's happening in the chain of events at all. Whether I use taxonomy or a list text field, it's stripping tags on the allowed values. I suppose I'll have to track that down further.

damien tournoud’s picture

damien tournoud commented

Ok, I see. AFAIK, ->optionsList() should return HTML. It's our job to pass it to filter_xss(_admin)() (for radio and checkboxes) or downcase it to plaintext (for select). The implementation of the options list callback for the list module fields simply calls hook_options_list():

return (array) module_invoke($field['module'], 'options_list', $field, $instance, $wrapper->type(), $entity);

(in entity_metadata_field_options_list())

drupalninja99’s picture

drupalninja99 commented

I am very interested in this as well as it is a major limitation to not be able to easily customized the attribute's options list. For example I would really like the price and maybe the title of the product as the label, instead of the same label that the admin sees when they are selected attributes for a product.

drupalninja99’s picture

drupalninja99 commented

Also could someone demonstrate how to implement the options list against the patch attached in #5?