Posted by ultimateboy on June 1, 2009 at 7:09pm
7 followers
| Project: | Ubercart |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
Issue Summary
Attributes of products are loaded into the node object as $node->attributes. This is a very generic term that is possibly used in other ways. I think it is probably best to change this to $node->product_attributes or $node->uc_attributes or something similar.
The word "attributes" has a lot of additional meanings outside of ubercart. Generally, "attributes" are used to store classes, IDs, titles, etc.
Comments
#1
Basic patch which replaces all instances of $node->attributes with $node->uc_attributes. Also removes an entry in hook_nodeapi() $op = 'load', which is not needed.
I have done some basic testing and all functionality appears to remain.
#2
I don't get the purpose of this. $node->attributes is only accessed in Ubercart if the node is a product type. In which case, Ubercart already owns the namespace, so there's no need for a prefix.
#3
TR, my specific issue is a bit specialized. I have created and currently use the Studio base theme. This theme deconstructs many aspects of the theme layer and re-organizes them in a more unified way. In the case of studio, $node->attributes is an array of attributes (classes and ids) to be applied to the node itself. Namespace is fairly important for contrib and in this case, I believe the "attributes" is far too generic. The convention is almost always to prefix names with "uc_" and it is my understanding that this is simply a continuation of that naming convention.
#4
I took a look at the Studio theme. Am I right in saying Studio is adding an "attributes" variable to *all* node types? I guess in that case, it's really Studio that needs to change; Studio, being a theme, deals with every node type, and it should do so without knowing a priori how the type is structured internally. So it seems to me that Studio bears the responsibility of segmenting its own namespace so that it doesn't stomp on elements defined by the modules that "own" the node type.
I guess I would invert your logic and say that "attributes" is far too generic a name for a *theme* to be adding to each and every node type, because at sometime in the future it's bound to conflict with one type or another. Changing Ubercart will just defer this problem until the next node type comes along that uses "attributes". The way to avoid this is to have Studio use something like $node->studio_theme_attributes instead of $node->attributes.
As I said in #2, uc_product.module owns the namespace on product node types. The use of "attributes" in product types doesn't affect any other module unless that other module is manipulating product types - in which case that other module ought to know better. In my experience, Drupal modules never prefix variables in their own namespace. Nor should they have to.
#5
In one perfect world, maybe both theme and modules would prefix their namespace. But that would be dull.
In this case, I'd agree that a theme should not be overwriting node structural data like that.
Ubercart does already own the structure of its own content type. It should be allowed to extend it without problems.
It's not like CCK should have to rename from
$node->fields[]to$node->content_fields[]in case a theme wants to use the word 'fields' (which is also generic).If you think theme data should be getting saved into the node content at all, then using $node->studio_attributes[] would be the safe way - directly following the argument in #3
#6
#7
#9
Woah there, cowboy. ;)
Directly stuffing all your fields directly into $node is quite dangerous. Not just $node->attributes, but what about things like $node->weight? This isn't about UberCart vs. themes, or UberCart "owning" the node. It's about UberCart playing nicely with the rest of the Drupal ecosystem. I've had to deal with numerous bugs over the years in Project* regarding collisions with other modules like this:
#98278: project* namespace bugs in $node
#542150: Make issue $node->project_issue[] namespace consistent
...
For basically all the same reasons that #510382: Ubercart must namespace its hooks is a good idea, I'd strongly encourage y'all to move towards something like $node->uc as an array, with $node->uc['weight'], etc. Or, at the very least, start using $node->uc_weight. It's totally reasonable and fine to postpone this until UberCart 3, given that it's a sweeping API change. But, if you're going to do sweeping API changes to namespace your hooks, please take that opportunity to namespace what you do in $node, too.
#10
Might be of interest to the UberCart folks: the reason I found this issue was working on #620812: Support importing UberCart product fields with migrate.module... That'll be a nice feature once it's committed. In fact, maybe that code should live in UberCart, not migrate, so that as you add/modify your fields and/or how they're represented, you can keep the migrate functionality working properly...
#11
This will be resolved in the next version when "what a product is" is separated from "how a product is displayed." i.e. Products will no longer be nodes, they'll just be displayed through nodes. As such, this won't be an issue any more.
This is actually one of the most exciting things about the next version to me. You can check out a little more at http://d7uc.org/node/49. I still need to make more issues in the Ubercore tracker, but for now I'll point to #616130: [meta] Fieldable Product Class and mark this as duplicate.
#12
I also fear this is uc3.x material at this point.
#13
any solutions?