Fix attributes for product classes [#2763531]

Attributes for product classes do not work. This moves a fix that I have had for a long time in Dropdown Attributes into Ubercart where it belongs. The patch is attached in the following comment.

Comment	File	Size	Author
#2	fix_attributes_for_product_classes-2763531-2.patch	525 bytes	trobey
#2	8.x-4.x: PHP 5.5 & MySQL 5.5, D8.2 214 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

9 July 2016 at 23:53

trobey created an issue. See original summary.

Comment #2

trobey CreditAttribution: trobey as a volunteer commented 9 July 2016 at 23:57

Status:

Active

» Needs review

File	Size
fix_attributes_for_product_classes-2763531-2.patch	525 bytes
8.x-4.x: PHP 5.5 & MySQL 5.5, D8.2 214 pass

Comment #3

TR CreditAttribution: TR commented 10 July 2016 at 04:06

Status:

Needs review

» Postponed (maintainer needs more info)

Please define what you mean by "do not work". The code in uc_attribute_node_load() is essentially the same thing we did in Drupal 7. uc_class_get_attributes() is/should be used only to find out what attributes are defined by default for a class. uc_class_get_attributes() does *not* return the attribute/options customized for a specific product (i.e. specific nid). Once you have a specific instance of a product of a given class, those default attributes are customized and saved on the product (indexed by nid) in the uc_product_attributes table, and from there on act just like normal attributes - they can be customized and even be deleted from the specific product of that class, despite being default for that class. So when you have a concrete product of any class and do a node_load() (which calls uc_attribute_node_load()), all the attributes defined on that specific product *do* get loaded and there is no need to call uc_class_get_attributes(). In fact calling uc_class_get_attributes() here is the wrong thing to do, since it does not return the attributes customized for that specific product.

So it's not clear to me what you see as wrong behaviour here. A test case demonstrating the problem would be appreciated, that way we have a way to validate and maintain the fix. We do currently have test cases which check attributes on product classes, but I'm sure they don't test all uses of the API.

Comment #4

trobey CreditAttribution: trobey as a volunteer commented 10 July 2016 at 05:14

The history on this goes back to issue #1518264: Possible to have the drop downs assigned to the product classes?. As the original poster stated they are dealing with over 800 products so they like to create attributes for a class and not individual products. I have tests for this functionality for Drupal 8 in UCDropdownAttributesClassTest. In order to get these to pass I had to add

/**
 * Implements hook_node_load().
 *
 * Ubercart fails to load attributes for product classes.
 */
function uc_dropdown_attributes_node_load($nodes) {
  foreach ($nodes as $node) {
    if (uc_product_is_product($node)) {
      if (empty($node->attributes)) {
        $node->attributes = uc_class_get_attributes($node->getType());
      }
    }
  }
}

When I comment this out the test fails. This was intended to be a temporary fix until Ubercart was working. I do not recall having to do this for Drupal 7 but perhaps my code has been broken all along.

I give priority to attributes defined for a product but if they do not exist then I use the attributes defined for the product class. This allows product attributes to be override product class attributes. However, if the attributes are copied from the product class to the product, the dependencies are not. So this will break any dependencies. It does not make sense to copy the dependencies to the product. If a store has 800 products in a class then in order to make changes they would have to edit 800 products. This defeats the whole reason for having attributes (and dependencies) at the class level. To put it in other terms, it would be like abandoning CSS and assigning styles to every element in a website. Even if these are generated automatically it creates a maintenance nightmare.

Comment #5

TR CreditAttribution: TR commented 10 July 2016 at 22:46

The way I described attributes in #3 is how they've always worked. Product classes act as templates for creating a concrete product, and after the product is created a change to the class definition (adding, modifying, or deleting attributes to the class, for instance) will NOT affect any existing products. That is by design. Imagine the inverse of your use case, where the user has created and customized 800 products then someone goes and changes something in the product class, blowing away all that customization and additionally requiring editing of those 800 products a second time to fix this. Your use case is more appropriately addressed via #298395: Let admin push class attribute/option changes out to existing nodes, which allows the administrator to explicitly push class changes down to the concrete products, if desired.

I'm not familiar with the dropdown attributes module and how it works, but I would think that #298395: Let admin push class attribute/option changes out to existing nodes would solve the problem by allowing someone to install your module, configure the dropdown attributes on the class, then push the changes out to the existing products. And any products created after the dropdown attributes were configured on the class would automatically get the class configuration.

Comment #6

trobey CreditAttribution: trobey as a volunteer commented 11 July 2016 at 04:45

Dropdown attributes first looks at attributes defined for a product and then, if none are found, it looks for attributes defined for the product class. So if attributes are defined for a product then they override the attributes for the product class. The only possible complication is if there are attributes defined for the product class but the product override is no attributes. There is no need to push the product class attributes because if they are changed they automatically apply to any product in the product class unless that product has attributes defined at the product level. That allows someone to change the attributes for all 800 products in the product class at one time.

It looks like I can override what Ubercart is doing so I can get the correct behavior. Then if someone would rather have that behavior they can install Dropdown Attributes. They do not even have to use the attribute dependencies if all they need is the change in behavior. This would allow users to have a choice.

I went back and checked and Ubercart for Drupal 7 does have the behavior you describe. But I assumed something different for several years and no one pointed out my error. I do not know anywhere that describes this behavior and it is not obvious nor expected.