If a custom product type is defined, the attributes do not appear. Need to accommodate all products defined, not just those with machine name 'product'.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 1517084-alpha-patch-remove-hardcoded-type-10.patch | 1.91 KB | elc |
| #9 | dc-example-types-output.txt | 2.64 KB | elc |
| #9 | 1517084-alpha-patch-config-line-item-types-attr-display-9.patch | 3.89 KB | elc |
| #2 | all_product_types-1517084-2.patch | 1.81 KB | fmitchell |
| #1 | all_product_types-1517084-1.patch | 1.71 KB | fmitchell |
Comments
Comment #1
fmitchell commentedPatch attached.
Comment #2
fmitchell commentedThis is a better fix of the above patch. It makes sure the line item type matches the product type when adding attributes.
Comment #3
olafkarsten commentedI didn't have the time to test this thorough. But i will happily commit this, if somebody else test it and confirm that it works.
Comment #4
elc commentedSomething is very strange with this bug and solution. This patch does not make sense and should not be included in its current state.
The two values being compared at not the same thing. One is "Line Item Type", and the other is "Product Type" and those should not be matching up.
After creating a series of random product types (product, tshirt), product display content types (product, notproduct), example products and nodes, I found that all of the line items had the $line_item->type as 'product' no matter what the product type or display content type was that was added to the cart.
The only way I could get a different $line_item->type value was to install Commerce Customizable Products, and add a new line item type, and then add that to the cart. I later found that the coupon module added a line item type too, and so do things like shipping modules. These additional types do not need to have attributes view run on them I would have thought? The Product Types don't even enter into it.
I would think that the original problem was not setting up the "Attributes view" in "Managed Display" on the relevant products. The previous two patches should not have fixed attribute display.
As for the hard coding of "$line_item->type == 'product'", I'm finding it hard to justify working around it. What else in a site, other than a variety of 'product', requires the "Attribute view" to be triggered for it? Attributes are after a product specific thing. My line item type list currently consists of product, the added custom product from above, plus a coupon; of these, I only want the 'product' line item type to have the attributes which seems to be the original goal of the condition. After experimentation, only 'product' ended up in the cart view by default. I had to manually add my special line item type to an order to get it to present at all.
Is the hard coding really something that needs to be attended to? I could see it could perhaps need a small configuration form on the line items editing pages. It would be perhaps an extra option on admin/commerce/config/line-items, or a tab off that to allow selection of which line items should call the contents of commerce_product_attributes_attribute_field().
Comment #5
elc commented#1411310: Using a new Line Item attributes disappear in the Cart & Order seems to be working around the exact same piece of code, but goes off on a tangent to an unrelated problem. I'm thinking there is a need for a configuration form for this now.
- Grab list of available line items types
- Show in checkbox list of machine name|Human name
- Only those items that are ticked will get passed the check; defaults to 'product'
OR
Only include those line item types which have a "Product reference" field type on them. This would take out the ability to configure something like a Coupon to display attributes when it has no ability to do so.
Comment #6
fmitchell commented@ELC
This isn't products that are created through the UI. This is custom products you define in code using hook_commerce_product_type_info(). Not every 'product' will be of the product type, especially if you use the API instead of the UI.
Ex: https://github.com/fmitchell/musicinst/blob/master/modules/custom/mic_co...
The patch simply takes that into account by cycling through everything defined in commerce_product_types() and using the same logic.
Comment #7
elc commentedHaving created a small module to add a new product type using the API call hook_commerce_product_types()[1], I can confirm that my statement is true. The comparison in the patch is invalid.
The line items are not the same thing as the product types.
$line_item->type is a line item type.
$products = commerce_product_types() returns an array of product types.
Comparing the two is a logical error because line item types are not product types. If they happen to have the same names, then it's a co-incidence, not expected behaviour.
If it was comparing commerce_line_item_types()[2], then at least it would be comparing the same thing, but then it would also be in error since commerce_product_attributes_attribute_field() is only ever called for product line items which will result in the comparison always being true and provide no benefit.
In my test site I have 3 different product types; product, tshirt, shoe. Shoe was added using the API, tshirt via the UI. If I add a shirt and shoe to the cart, I get this debugging result:
The line item type is always "product" for a product line item because this is the entity type - all the product types are bundles - and it does not matter which method was used to create them. The current code will display the attributes on any product bundle added provided it is configured (manage display + view). The product type is not what is being compared against.
The only time I can see it not happening is when using the Commerce Customizable Products module for custom line items which do not have the same line item type. It is in this case that I could see having a configurable method to add machine names to check against, but it wont be comparing against the result of commerce_product_types() or commerce_line_item_types(). It will need to be a list of admin set values chosen from a generated list of line item types that have a "Product reference" field on them as these should be the only line items that will have a product with attributes associated to them.
[1]http://drupalcontrib.org/api/drupal/contributions!commerce!modules!produ...
[2]http://drupalcontrib.org/api/drupal/contributions!commerce!modules!line_...
Comment #8
fmitchell commentedAlright. I won't argue with you. I do have different product types other than 'product'. I thought there were defined by the API, but I maybe I did it through an update hook or the UI.
If you don't want to include the patch, fine with me. I just know the assumption that everything is of 'product' type is wrong, so why not account for them all. Not sure why the patch removing that assumption is 'invalid', but do what you want.
Comment #9
elc commentedThe comparison "$line_item->type != 'product'" is against a "Line Item Type", not a "Product Type". The default line item type just happens to be called 'product', but it is in itself not a "Drupal Commerce Product".
You can view the Line Item Types and Product Types your site has registered with the following drush script:
See the attached output from my test site. The types having the same bundle name is a co-incidence, not a repeatable and reliable feature. Do all of the line item types you have added to the site come up in the 'line_item_types' part of the array?
I'm not saying that this shouldn't be dealt with, but I am saying that it needs to be comparing two things that are the same type of thing. Only a subset of the available Line Item Types should be available for admin choice and used in this comparison. To that end, I provide the following temporary patch to enable processing on any line item type. This may not be the best solution as it appears that the "Attribute View" view mode is only added to "commerce_product" entity types anyway, meaning that any additional Line Item Types that appear will not have this view mode to configure.
Do all of your Line Item Types have the field "commerce_product" or a "Product reference" type field on them? If I could use that for the condition, then it wouldn't need the administration form.
Please note that this patch may not apply without errors. I have been tinkering.
Comment #10
elc commentedAssuming that you can only print attributes for a product when there is a product available, and that the product is always referenced with the field "commerce_product", then this is a much simpler solution. The second assumption was already being made by the existing code.
Only print the attributes when there is a product available. This patch is mutually exclusive of the one above.
Comment #11
summit commentedComment #12
anybody@Module maintainer: Any progress here?