Putting the line item type in the 'add to cart' form settings in the product ref field's display settings feels really brittle to me.

The link between a product type and the sort of customization it allows should be fixed in one place. But putting it in display settings means that connection has to be repeated if there are several node view modes (such as teaser, search listings, etc). It means that any creation of an add to cart form somewhere else (eg, by another module, or remotely over Services) has to be aware and sniff out this property from the node display settings.

However, I see that this setting is actually part of the cart module, and not this one. I'm therefore not sure how this could be fixed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

malberts’s picture

Category: feature » bug
Status: Needs review » Active

I started modifying this module to allow the Product Type to override the Add to Cart line item type. As a first requirement, we need #1799376: Provide alter hook for Line Item in Add to Cart form.

Here is my roadmap for some extensions to this module:

  1. Product Type: Define Line Item type for Add to Cart form. [almost done]
  2. Product: Define Line Item type for Add to Cart form. Overrides Product Type settings.
  3. Product Type: Define a mapping of which Line Item fields to display on Add to Cart form (using #access).
  4. Product: Define a mapping of which Line Item fields to display on Add to Cart form. Overrides Product Type settings.

Getting the Product (Type) - Line Item association working should be the first goal. Thereafter we should get the field associations done. I will post a patch when I'm done with each item.

I also need the following functionality which is unrelated to this issue but dependent on this functionality:

  • Modify Line Item fields' allowed values on Add to Cart form:

    I have to filter the allowed values for the fields on the Line Item in the Add to Cart form. This means that within the list of allowed values resulting from the Field settings, I need to be able to select a subset that will be allowed on the Add to Cart form. For now this will only work with Options-based (_options_list) fields. Additionally, it will not be possible to add additional allowed values not part of _options_list() to the list.

    Should this be part of this module too?

malberts’s picture

Status: Needs review » Active
FileSize
9.64 KB

This patch adds the functionality to have a Line Item type associated with a Product Type (task 1). This Line Item type will then be used in the Add to Cart form.

If your Product Display references different Product types which have different Line Item types, it will display the correct Line Item fields for the selected Product during attribute AJAX refresh.

malberts’s picture

Title: line item type should be baked into the field settings rather than display settings » Add to Cart Line Item type should be determined by product type or product instance
Assigned: Unassigned » malberts
Status: Active » Needs review

In the patch I defined a new Line Item view mode ("Customizations") with the intent of it being used in the cart instead of adding the line item fields directly to the view. This allows you to have one field in your cart/order/etc. views for displaying any kind of line item.

However, there is a problem because when I add a field (Commerce Line Item: Rendered Commerce Line item) with view mode Customizations it keeps on rendering the Line Item's product, even when I set the view mode not to display the referenced product. When I debug the $build data in commerce_custom_product_entity_view_alter() I can see a few "product:..." items. I am not sure if those being there is resulting in them being displayed alongside the Line Item fields.

malberts’s picture

I think I attached the previous patch incorrectly.

malberts’s picture

Category: bug » feature
Status: Active » Needs review
FileSize
16.92 KB

Here's a new version of the patch. Included in this one:
- Both the Product Type and the Product entity can now determine the Line Item used in the Add to Cart form.
- Product Type setting path: admin/commerce/products/types/[product-type]/edit
- Product Type setting storage: in new database table
- Product entity setting when editing a Product directly or with Inline Entity Form.
- Product entity setting has precedence over Product Type setting.

The default line item type setting on a Product Type can also be defined in code. All Product entities get a new (locked) Text List field that references all the possible Product Line Item types. This field is used to override the Product Type selection.

Example usage: One product display can now contain products with different Line Item types. On attribute refresh it will select the correct Line Item, from either the Product entity or the Product Type, and then render it. Basically, you can have a mix of different product types with default line item types and products with overridden line item types.

Some of the things I did in here I've never done before, so I would appreciate it if someone with more experience could review it. After that, and if everything is OK, I'd like to see this get committed before I start working on Line Item field mappings (which should actually be a separate issue).

joachim’s picture

Category: bug » feature
Status: Active » Needs review

Looks like you've been busy! I'll try and take a look at this tomorrow.

agileadam’s picture

commerce_custom_product-lineitem-1798006-5.patch applied cleanly. I forgot to run a drush updb, but after doing so everything is clean. I'll be testing it out now and will report my findings.

agileadam’s picture

While talking with malberts in IRC (#drupal-commerce) we discovered that, in order to get the "Add to Cart line item type" field to show up on the product entity edit form, you currently have to disable and re-enable commerce_custom_product.

joachim’s picture

Lots of code to review here, and this isn't in huge depth. Will look at this more tomorrow.

+++ b/commerce_custom_product.module
@@ -5,6 +5,11 @@
+define('COMMERCE_CUSTOM_PRODUCT_DEFAULT_LINE_ITEM_TYPE', 'product');

I don't think we need to define this as a constant. We know it's 'product' can just use that.

+++ b/commerce_custom_product.module
@@ -191,3 +196,307 @@ function commerce_custom_product_line_item_type_delete($type) {
+/**
+ * Gets the associated line item type from a product type.
+ * ¶
+ * @param $object
+ *   The object from which to determine the Line Item type.
+ * @param $type
+ *   A string indicating the type of the object.
+ *   Allowed values: 'product' or 'product_type'.
+ * ¶
+ * @return
+ *   The line item type machine name, or 'product' if not found.
+ */
+function commerce_custom_product_object_line_item_type($object, $type) {
+  if ($type == 'product') {

I'm really iffy about a function that takes a generic $object that could be an entity or an entity type.

What I think I'd rather have is something that gets the line item type from a product and looks at, in cascade:

- the product entity
- the product type

This could be a metadata property getter.

+++ b/commerce_custom_product.module
@@ -191,3 +196,307 @@ function commerce_custom_product_line_item_type_delete($type) {
+      $product_setting = $wrapper->commerce_product_line_item_type->value();
...
+/**
+ * Ensures a line item type field is present on a product type bundle.
+ */
+function commerce_custom_product_configure_product_type($type) {
+  $description = t('The line item type to use in Add to Cart forms. This
+    setting will override the Product bundle setting.');
+  commerce_custom_product_line_item_type_create_instance('commerce_product_line_item_type', 'commerce_product', $type, t('Add to Cart line item type'), $description);
+}

Do we have a line item type field type defined somewhere?

I think this might be better done with http://drupal.org/project/list_predefined_options.

agileadam’s picture

Well, I'm pretty impressed! The changes you've made to this module make it significantly more powerful. It is now very easy for me to "attach" extra fields to the "Add to Cart" form.

May I suggest updating your docs to include that you must choose "Include this field on Add to Cart forms for line items of this type." when setting up custom fields in the custom line item types? Other than that, and the "disable/re-enable to see the select list" bug, this seems rock solid. I'll report back if I see anything funky.

Thanks a ton!

malberts’s picture

@joachim: Thanks for the review so far.

Do we have a line item type field type defined somewhere?

I think this might be better done with http://drupal.org/project/list_predefined_options.

I followed the pattern used in commerce_price and commerce_shipping.

In commerce_price_create_instance() it creates the field if it does not exist and then creates the instance if that does not exist. While commerce_price_create_instance() creates a field of type 'commerce_price', I just create a field of type 'list_text' with an allowed values callback. This is similar to what commerce_shipping does in commerce_shipping_line_item_configuration().

Won't list_predefined_options then just be an unnecessary dependency? I don't really see a need for it unless we want other, unrelated list fields to also be able to reference line items. As I understand it, it just allows you to define a reusable list that can be selected from the list field settings UI. Or did you have something else in mind with it?

What I think I'd rather have is something that gets the line item type from a product and looks at, in cascade:

- the product entity
- the product type

This could be a metadata property getter.

The Product's "line item type" is a field and can therefore be accessed with the field's default metadata getter. Should I add another dummy property that checks both the Product and the Product Type, or should I override the field's "getter callback" to rather check both the field's value and the Product Type's value?

I will make some changes and upload a new patch soon.

joachim’s picture

> I just create a field of type 'list_text' with an allowed values callback. This is similar to what commerce_shipping does in commerce_shipping_line_item_configuration().

Ah right. Sorry, I hadn't spotted that part. (I was reading the patch late last night...)

In which case, yes, you're right, that looks fine.

But shouldn't the field be created in hook_install() rather than hook_enable()? And in any case, all those functions should live in MODULE.install.

I'm also wondering if there's a better way of saving the product type -> line item type relationship that doesn't require us to have a whole extra field.

Could this perhaps be a setting we save in the field instance options? (Not an actual FieldAPI default, as IIRC those get written to the database for every row; they're a 'write default' rather than a 'read default'.)

> The Product's "line item type" is a field and can therefore be accessed with the field's default metadata getter. Should I add another dummy property that checks both the Product and the Product Type, or should I override the field's "getter callback" to rather check both the field's value and the Product Type's value?

Yup, that's what I meant. Something like $product_wrapper->line_item_type->value() and then let the getter callback for that property deal with figuring out whether to read something from the particular product or from the product type. That then encapsulates this bit of the architecture completely.

joachim’s picture

> I'm also wondering if there's a better way of saving the product type -> line item type relationship that doesn't require us to have a whole extra field.

Wacky idea -- why not just go by the machine names of the types to match them up?

I've been doing this with nodes/products on my site already: a node of type 'product_shirt' always refers to a product of type 'shirt'.

So if there's a line item of type 'foo' then a product of type 'foo' should use that line item.

EDIT: I guess that won't work if you want several product types to point to the same line item type :/

joachim’s picture

Ok so going back to my earlier idea:

> Could this perhaps be a setting we save in the field instance options? (Not an actual FieldAPI default, as IIRC those get written to the database for every row; they're a 'write default' rather than a 'read default'.)

:)

joachim’s picture

To explain more about what I mean:

It's quite easy for any module to add custom options to a field by form_altering 'field_ui_field_edit_form'. Any form elements you add to $form['instance'] will get saved into the field instance's options, and can be retrieved whenever you have hold of the field.

For an example, see http://drupalcode.org/project/field_instance_cardinality.git/blob/622209...

malberts’s picture

I had a look at it. Turns out I can only use that method if the field is not locked, but I am currently locking it (like the Price field). Should we go this route and rather prevent deletion of the field programmatically? Or should I just return the default line item type if the field is deleted?

malberts’s picture

Lots of changes and slightly smaller:

1. Most importantly, #1799376: Provide alter hook for Line Item in Add to Cart form is no longer needed. I only had to rebuild the line item which turns out to have required less code in addition to not needing a core hook.

2. Added the field instance creation function to an update hook so it is no longer necessary to disable and enable the module after applying the patch or updating (agileadam #10).

3. Added the field instance creation function to the product type insert hook so that new product types get this field added.

4.

But shouldn't the field be created in hook_install() rather than hook_enable()? And in any case, all those functions should live in MODULE.install

I followed what commerce_product did, i.e. creating the field during enable and having hook_enable in the MODULE.module file. However, I now moved hook_enable to MODULE.install. I suspect that having it in hook_enable instead of hook_install allows it to still attach the field instances in the following use case:
enable commerce_custom_product --> disable commerce_custom_product --> enable a module with product type(s) defined in code --> enable commerce_custom_product
In the last step hook_install won't trigger again so the fields wouldn't be attached. If this is not a valid reason I'll change it.

5.

I'm also wondering if there's a better way of saving the product type -> line item type relationship that doesn't require us to have a whole extra field.

Could this perhaps be a setting we save in the field instance options?

I implemented that and removed the 'locked' property on the field. To get the field relatively unchangeable I remove some form items on the field edit UI and hooked the field delete form to not allow deleting the line item type field. The field can also only be added to entities of type 'commerce_product', so the field should be close to being locked without actually being locked.

I think this patch is now very close to what we need to get the line item association into this module.

malberts’s picture

A quick update. I've moved development of this functionality to a standalone sandbox module: http://drupal.org/sandbox/malberts/1810868 http://drupal.org/sandbox/malberts/1947762
Since Commerce Customizable Products is mostly just a line item type UI and not really required (though recommended) for this functionality, I decided to move this as it might not be desirable in all cases. However, if consensus is that it should be part of this module I will port it back.

One thing I've added is the ability for a Product Display to also have the line item type field. If this field has a value then it will override the line item type settings on the product/product type level. This is to simulate the default Commerce behaviour of setting a line item type on a Product Display level. However, this field cannot replicate the possibility of having a different line item type for the same product reference field on different display modes. In my case I have no need to ever want to do that. I tried to alter the Field Display settings form with Field formatter settings to provide an 'override product setting' but I couldn't get it to save the value. Because I have no need for this and it just adds and extra dependency, I've decided not to investigate it further.

agileadam’s picture

Assigned: malberts » Unassigned
Status: Needs review » Needs work

#17 applied cleanly to 7.x-1.0-beta2.

@malberts, the sandbox project you suggested in #18 doesn't exist... which is why I used the patch in #17. What's the scoop on this? Thanks for your work on this.

malberts’s picture

@agileadam, I think I moved it to http://drupal.org/sandbox/malberts/1810868

Note that I haven't worked on it for quite some time and it isn't used on any live site. To be honest I'm not sure if that is the best way of working with Commerce. However, I believe the last code I submitted there should do the same as the earlier sandbox. IIRC I added the ability to select the line item type on a specific Product instance, the Product Type (on the field settings) and on a Display instance.

If this method works for you and satisfies your use case then by all means use it. The reason I probably won't be looking at it again (at least for now) is because I want a solution that doesn't alter core Commerce functionality which is what that does. I'm basically snubbing the Product Reference formatter settings and rendering a new line item type in the place of the one in the field settings. I'm not sure if this is good in this case. Also, I'm not happy with the way the alternate line item is injected into the form. At the moment the line item type selected on the Product Display gets rendered by the core add to cart form anyway. When my alter kicks in I replace the wrong line item type with the new one. This might have performance issues, especially when the form needs to do many AJAX calls. I have not yet found a satisfactory way of supplying the line item type *before* the add to cart form gets rendered. Also, there's no easy way to alter a field's formatter settings form. Which means that the form will still be as if the original functionality is present, but it won't be used and thus is useless. But maybe none of this is really a problem. Ideally someone higher up in the Commerce world could give some thoughts on this issue.

However, for my purposes there's a different path that I'm investigating where the Product Display type, Product type and Line Item type is maintained in sync. In conjunction with commerce_backoffice (I think) or with custom code, you can modify the node types listing page and menu items so that Product Displays are not mixed up with your other node types. That way even if you have quite a few Product Displays, they will be listed in a separate place, so as not to clutter up the normal node "workflow". The downside to this is that you will potentially have 3 things (Display + Product + Line Item) for every product class. Managing this by hand would become really annoying really fast which is why I'm aiming for increased automation there. However, this alternate method does not specifically solve this issue - which is to allow the line item type to be selected in a place other than the Product Field formatter (Product Display) - so it might be completely irrelevant to what you want to do. I think I simply do not have a good reason to want to do this any more, the only gain for me would be in having only a generic "Product" Display instead of one for each product type.

Either way, this issue still stands and I'm none the wiser. As soon as I get my new method to some usable point I'll upload that code too.

agileadam’s picture

Wow. Thank you for taking the time to explain!

the only gain for me would be in having only a generic "Product" Display instead of one for each product type.

That's exactly why I originally used your patch. It definitely seems like a burden to have to create new product displays for every product type (especially when you simply need to add a few user input fields to a product or group of products. I have a site with 8000+ products; it'd be crazy to have to build out new product displays given all of the theming, views, code, etc. involved.

malberts’s picture

Now that you mention it, is the Product Type level the only place where you'd want to associate a Line Item type? In other words, you do not need to assign a Line Item type to a specific product instance or a product display instance?

I remember now that I tried a different method that went around the double rendering issue I mentioned. I created a new display formatter for Product Reference that injects the proper line item type before the form is rendered. The only issue there was that I needed to duplicate most of the code that was used when rendering the Product Reference field. The end result, however, was also to call the add to cart form. With this separate formatter you could decide whether the Display should render it in the standard way, or in this new way.

I don't know if you came across it, but when your Display's product reference field allows multiple types and you also marked the setting that displays the Product entity fields on the Product Display display settings then you get a massive list of all their possible fields. That makes it harder to use only 1 general Product Display if your Product types have many fields and you want some of them to be switched with the attribute AJAX code. I think that might have been one reason why I decided to create separate Display types. But I must re-investigate this. Did you have any issues when you used just one Product Display with my old line item code?

agileadam’s picture

I'm assigning on individual products, not on the product display or at the product type level.
Currently, the products that are custom line items are the only products in their product displays, so I haven't seen anything like you're mentioning.

I've just tested everything again and my current implementation is working fine. I have several new custom line item types, and a handful of products that make use of a custom type through a single-value select list on the product edit form. The fields all show up correctly and are associated with the product/order correctly.

Leo Pitt’s picture

I'm also looking for this functionality (line item fields associated with individual products, not product types). Has there been any development upon this?

I've been doing some googling and it doesn't look as if anything has happened beyond the sandbox in #20. Is that right?

malberts’s picture

@leopitt, as far as I know there still isn't an "official" way of doing this.

The sandbox was one of my earlier attempts at finding a solution. However, I decided I no longer needed the flexibility of doing a line item type per product instance, so I haven't worked on it since.

The code there should work though. My issue in #20 still stands as I don't think it's possible to inject a different line item type cleanly into the point where it is determined in the core module. Perhaps I'm wrong. So probably a better way of doing this would be to create a new Product reference display formatter which determines the line item type as per the sandbox code. That way you don't need to change the default functionality should you ever want that together with this.

Leo Pitt’s picture

@malberts - thanks. I have downloaded your sandbox module and it seems to do exactly what I required for now.

agileadam’s picture

I don't have time to write a full explanation, but I have been using, successfully, the patch from #17.
I added an additional field today that I needed to use as an attribute and noticed the option to "Enable this field to function as an attribute field on Add to Cart forms" was missing. To get this (and the "Number of values" options back, I had to comment-out two lines from the patch:

function commerce_custom_product_form_field_ui_field_edit_form_alter(&$form, &$form_state) {
  ...
  //$form['field']['cardinality']['#access'] = FALSE;
  //$form['instance']['commerce_cart_settings']['#access'] = FALSE;
}

@malberts, what was the reason behind hiding these? Sorry, I know you've not continued development on this...

malberts’s picture

I had to look at my sandbox code because I wasn't sure. I think the intention was to hide those values when you are editing the line item type field because it does not make sense to allow that field as either an attribute or to have a cardinality.

In the sandbox code, that form alter checks the following:

if ($field['field_name'] == 'commerce_field_line_item_type' && $instance['entity_type'] == 'commerce_product') {

whereas the patch does not. However, removing #access is mostly a cosmetic thing and not really necessary.

By the way, is there a specific reason why you prefer using the patch instead of the sandbox code? A major difference is it's a separate module vs a patch, but the code in there should better and there might be some extra things I implemented there (I don't remember now).

agileadam’s picture

Thanks for the quick response!

The only reason I'm using the patch code is because the sandbox wasn't available when I first implemented your code... you'll see this if you navigate the comments on this issue... the link wasn't working for me when I needed to implement it. No worries.

I'd rather be using your separate module, definitely... next time I have to make any changes I'll switch over to your sandbox module.

Thanks for explaining, by the way!

malberts’s picture

Update: I've rewritten the original sandbox code in #18. Turns out I do not like multiple product displays :).

The new sandbox: http://drupal.org/sandbox/malberts/1947762

Changes:
- Product settings are no longer stored in a field, but instead uses the data property and an entity wrapper property to make accessing it easier.
- Settings now capture "default quantity", "show quantity" and "combine line items" in addition to the line item type.
- Product type settings moved to the product type edit page instead of being part of field instance settings.

There was some admin stuff in the original sandbox that I did not bother to add. The new settings also doesn't plug into Views automatically due to having a field.

Moving from a proper field to a pseudo-property means you lose the ability to query these settings, for example to find all products using a certain line item type setting. This is not a big loss for me right now and it does make the module simpler because there is no need for a dedicated field nor to alter the database.

I still haven't found an elegant way to the change the values going into the add to cart form builder. So at the moment the normal add to cart form is built with the display formatter settings and then I alter the form with the correct line item settings. An alternative to this would be to create a new display formatter but then I'd end up duplicating most of the original code and there's a likelihood that some module alters something based on the original naming and then I won't get those changes.

Kazanir’s picture

I am going to install and give this a whirl today; I'd love to contribute if I can although my API chops are pretty limited still.

T.Mardi’s picture

Hi all, does anyone have a way of doing this?

I've tried malberts sandbox module in #30 but it isn't working unfortunately. The line item field isn't added when checking the option on the product types setting page.

I've also tried using a line item reference field to select a different line item per product instance but I can't enter a value. This field is locked from what I've read?.

Use case is:

I have product type called "Football shirt". This includes short and long sleeve tops. Customisation options are "name on sleeve" and "name on cuff" amongst others. "Name on cuff" would not apply to the short sleeve tops as there is no sleeve...

So I have created 2 vocabularies which contain relevant options for each instance of this product and attached them to 2 different line item types. I just need a way to be able to select which line item type is applied per product instance but can't seem to find a way...

I would be very greatful for some insight on this.

pouldenton’s picture

Issue summary: View changes

Hello! Is there some news about this problem? What is the right way to use assigned to product (or product display) line item type in views product field now?

nlambert’s picture

I've created a feature request in the Drupal commerce issue queue: #2263067: Line item determined in product entity type

Maybe this issue should be closed...

rv0’s picture

Also interested in a solution for this usecase
been toying with @malberts sandbox project, which partially works

maxplus’s picture

Hi,
Would be great for me too, I also need the link between a product variation type and an custom line item type.
I think this indeed makes much more sense then setting the Line item type at the product display node level!

SocialNicheGuru’s picture

The patch adds 'Product Type Add to Cart line item type' to a number of fields.
In addition there is no 'None' or 'NA' option. It defaults to 'Product'.
So now every piece of content is a product.