Posted by DanZ on January 9, 2013 at 7:21pm
6 followers
| Project: | Ubercart |
| Version: | 7.x-3.x-dev |
| Component: | Orders |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Ordered products are already entities set up with EntityAPI. However, they are not fieldable. Make them so.
Making them fieldable would allow admins to add custom fields that they need, but that would not be appropriate for the general case. For instance, I have an author that gets a royalty of 20% of the list price of books sold, but the same book may sell for different prices. I need the SUM of that for all ordered products over a date range. Adding a custom Computed Field that calculates the Product list price times the Ordered Product quantity would solve the problem, since Views can take a SUM of that. It has the added benefit of working even if the list price changes.
Comments
#1
Could it really be as simple as this, or is there more to it?
#2
It looks like there is more to it. See hook_entity_info() comments and the example at http://drupal.stackexchange.com/questions/50294/entity-api-fieldable-ent....
#3
This patch adds a new tab to /admin/store/settings/orders to configure fields. Field UI also adds a field display configuration tab, but the patch disables it, as at present we don't actually use the field display settings anywhere.
#4
Heh, I was going to do that, once I got the time.
I'll test this to see if I can use this to attach a computed field and then display it in a View.
Meanwhile, if I see something I think I can handle in the near future, I'll mention that, so you'll work on something else good.
#5
Ok, I tested it by installing the Computed Field module.
I was able to add the field to the type. The machine name of the field is field_total_full_price. Adding the field correctly created the field_data_field_total_full_price and field_revision_field_total_full_price tables in the DB. That's the good news.
Issues:
The config tab is called "product fields". It ought to be called something like "ordered product fields", to avoid confusion with the regular product fields. It might also be somewhere else in the menu tree. Maybe.
The page needs a little help text to explain why one might want to add fields. Computed fields and Views are the big use case, of course.
Creating an order did not add any field data to the DB. Presumably, this is because uc_order_product_save() just uses drupal_write_record() to save its values and never calls any of the field_attach_*() functions. There is information at http://drupal.org/node/474582 about doing this, but I think there's probably some way to use EntityAPI controller to do this more automatically. DrupalDefaultEntityController::attachLoad(), for instance, attaches the fields, so there's probably some way to use the Entity API load, save, create, etc. features to do this automatically. Maybe just calling entity_load() and entity_save() instead of the drupal_*_record() functions would do it. That's how it's done in this great tutorial.
The new fields do not show up in Views. Presumably, this is because uc_order_views_data() is implemented directly, instead of letting Entity API do it. Either uc_order_views_data() has to provide information about the fields, or this should be switched to letting the Entity API Views controller provide the Views data and then using hook_views_data_alter() to provide the additional custom information. See http://drupal.org/node/1208874.
It looks like the Entity API automation uses entity_views_field_definition() to set up the Views data for entity properties, including fieldable stuff. There's more to it, but this might be a model to work from if we don't use EntityAPI to do it.
Why use hook_menu_alter() instead of just putting the new stuff in via hook_menu()? Just curious.
And yes, I'll work on this one, although it's going to take me some research to get it done.
#6
Actually, it looks like calling entity_views_table_definition('uc_order_product') will get a generic table entry with the Field properties defined. This entry could then be modified to add the Ubercart special sauce before adding it to the uc_order_views_data() return value. Worth a try.
#7
This adds the attached Fields to Views with the method described in #6..
I still am not getting the fields to be saved into the database, despite trying a few things. I think 'save callback' may be the wrong approach here. Instead, overriding EntityAPIController::save() might work better.
Here's a patch with progress so far, as I'm done for a while now.
#8
The last submitted patch, ubercart_fieldable_ordered_products-1884512-7.patch, failed testing.
#9
I've not yet tried to apply the patch in #7, but I noticed this piece of code in your patch:
+ foreach ($product_ids as $product_id) {+ uc_order_product_delete($product_id);
+ }
I think it's better to use the
entity_delete_multiple()function here. Of course, you should ensure that the hookhook_uc_order_product_delete()is still invoked.I'm interested where this is going to, I'm also trying to get familiar with the Entity API.
#10
Moving the functionality into the controller class and getting rid of the callbacks seems to do the trick. This patch works; I can see the value of attached fields in a View.
Notes:
Help added for the attach fields page.
Some new API functions added (which just call the entity functions).
I didn't add uc_order_product_view(). Maybe I should. The cart and review pages just call entity_view() instead.
The uc_order_product_entity_save() function is probably obsolete.
Direct access to the database replaced by calls to uc_order_product_save(), uc_order_product_load_multiple(), and uc_order_product_delete_multiple(). The latter means that hook_uc_order_product_delete() is invoked whenever an order is updated, a cart is rebuilt, etc. I'm not sure that's OK. If not, something will have to be done to make the hook invoke optional.
If this is the right approach, I'll hunt for other direct access to that table.
Some interesting new fields shows up in Views, including "Rendered Order Product", "data" (which I removed), and "Weight units" (which I cleaned up). These are in addition to the attached Fields.
#11
The last submitted patch, ubercart_fieldable_ordered_products-1884512-10.patch, failed testing.
#12
Hmmm. The Ubercart tests error out on my system. Guess I'll try it here. This patch attempts to fix the errors found in 10, and also gets rid of the direct table access in uc_stock to uc_order_product_load().
It also adds an option to indicate if hook_uc_order_product_delete() should be invoked when deleting an item, because invoking it every time messes up the stock levels. It would probably be better to somehow mark the entities if they represent stock that's been decremented, and acting accordingly. However, that would probably require a schema change, so let's do it this way for now.
#13
The last submitted patch, ubercart_fieldable_ordered_products-1884512-12.patch, failed testing.
#14
This should get rid of at least some of the auto-test errors.
#15
The last submitted patch, ubercart_fieldable_ordered_products-1884512-14.patch, failed testing.
#16
Well, those error messages are baffling. Anyone know how to get a backtrace out of that so I can find out where it's messing up?
I tried running the Ubercart tests locally, but got a bunch of error messages. Are those working right now?
#17
Well, that was tricky. It turns out that EntityAPI also generates a hook_uc_order_product_delete() when you use its controller. That conflict caused the errors. I fixed it by overriding EntityAPIController::invoke() so that it invokes hook_uc_order_product_entity_delete(), instead. (Try finding that without a backtrace.)
Please evaluate if this is the correct approach. The other possibilities are to get rid of the entity version of the hook entirely, or to update uc_stock_uc_order_product_delete() to use the entity hook and get rid of the Ubercart hook. (No contributed modules use that hook, so the latter would be better, but this would break any custom modules that use it.)
While I was there, I added APIs for the rest of the automatic EntityAPI hooks: hook_uc_order_product_load, hook_uc_order_product_insert, hook_uc_order_product_presave, and hook_uc_order_product_update. I didn't try to document the views hooks at http://drupal.org/node/999954.
I also added/modified API functions to wrap entity_view(), entity_load(), entity_delete(), and entity_save(), and replaced all the direct access I could find to the {uc_order_products table} and calling entity_*('uc_order_product, ...).
This should be everything needed for this issue.
So, it turns out that answer is "no". It's not as simple as the patch in #1.
#18
Further thoughts:
Pre-patch, uc_order_product_delete() is called only from uc_order_edit_products_form(). It seems like a good idea to move the hook invocation there. That would get rid of the annoying extra argument that I had to add to uc_order_product_delete(), and the entity hook is now there for anyone who needs it.
#19
This patch moves the uc_order_product_delete() invocation as suggested in #18. It also cleans up some comments.
I think that covers everything. If this is a reasonable approach, this patch is ready to go.
#20
Thanks for this. A few points:
+function uc_order_product_load_multiple($opids = array(), $reset = FALSE) {+ return entity_load('uc_order_product', $opids, array(), $reset);
+}
+function uc_order_product_view_multiple($order_products, $view_mode = 'full', $langcode = NULL, $page = NULL) {+ return entity_view('uc_order_product', $order_products, $view_mode, $langcode, $page);
}
In Drupal 8, these one-line wrapper functions seem to be discouraged in favour of direct calls to entity_*(). As we are adding this quite late in the 7.x cycle, I am not sure we need to add them here either.
+function hook_uc_order_product_entity_delete(object $order_product) {function hook_uc_order_product_delete($order_product_id) {
I wonder if this has ever been implemented in contrib, or if we can get away with a small API break here and change to doing things the proper Entity API way, instead of having two near-identical hooks. As the existing hook_uc_order_product_delete() is only invoked from the admin edit form, can we get away with simply documenting this in the release notes?
function uc_stock_uc_order_product_delete($order_product_id) {// Put back the stock.
- $product = db_query("SELECT model, qty FROM {uc_order_products} WHERE order_product_id = :id", array(':id' => $order_product_id))->fetchObject();
+ $product = uc_order_product_load($order_product_id);
uc_stock_adjust($product->model, $product->qty);
}
It almost feels like this should be done with Rules. I was not even really aware this triggered a stock adjustment, to be honest... However this should probably be taken to #1514756: Stock transaction should be more flexible through Rules
#21
Eh, I suppose it's not too important, but I like the wrappers, and I already wrote and tested them. The code is a little easier to understand with them.
The thing to note here is that entity_view() takes an array of entities, so there's no entity_view_multiple(), but I made uc_order_product_view() and uc_order_product_view_multiple(). Entity_load() is the same way.
No contributed modules use that hook.
The issue is that the entity hook gets triggered every single time an ordered product is removed (deleting abandoned orders, etc.). It should not usually trigger a stock decrement. The existing Ubercart hook gets invoked only from the order edit form, and it should trigger a stock decrement. So, if you only have one of the hooks, you have to indicate whether it was invoked from the order edit form.
The real problem here is that the original hook has an inappropriate name. I can think of three possible fixes that let the entity hook have the proper name:
$product->delete_reason = 'admin order edit';. That way, the entity hook implementations could see why the deletion happened. The uc_stock implementation could decide whether or not to decrement the stock.Given that it isn't used in contrib, probably yes. The two big wins here would be more consistent naming of the entity hooks, and the ability to get rid of the override function UcOrderProductController::invoke(). That one is troublesome because it is mainly a copy-paste of its parent, and never calls its parent. That means that it has to be maintained and updated whenever EntityAPIController is updated. Yuck.
Option #3 is probably the most complete solution. However, I'm in favor of option #1, mainly because it's simplest, and also because it doesn't preclude doing #3 later, if there's ever a call for it.
Sure. A rule event invocation could be inserted into the order edit submit handler, the uc_stock hook implementation removed, and uc_stock could add a default rule to decrement the stock. It could be done lots of other ways, too. The original hook might no longer be needed, although it wouldn't hurt to keep it around. I'll put a note in #1514756 pointing to this issue.
#22
Note this code I found in EntityAPIController::invoke():
<?php// Invoke rules.
if (module_exists('rules')) {
rules_invoke_event($this->entityType . '_' . $hook, $entity);
}
?>
This means we have the Rules events uc_order_product_* for insert, update, delete, presave, and maybe more. I'm not sure if more work needs to be done here, but this probably helps with #1514756: Stock transaction should be more flexible through Rules, at least.
#23
This implements idea #1 from comment #21 above.
#24
Just to let you know I'm not ignoring this, but as this touches code at a fundamental low level it takes a bit longer to review and think about than many other patches, so bear with me.
See also #1492626: "Cart Items" triggers dont work which implements related changes for the cart item entity.
#25
Since UcCartItemController extends UcOrderProductController, this patch will enhance cart items, too. That includes adding the rules events mentioned in #1492626: "Cart Items" triggers dont work. So, fully implementing Entity API is probably the right way to do it.
It also takes a while because it's large. The good news is that, except for the delete hook, it doesn't change existing functionality. It just adds new functionality.
The important questions are:
Is the approach from comment #21 idea #1 the right approach for dealing with that hook?
Should the rules events mentioned in #22 be tested/documented/implemented? Actually, this could be added in a future patch, if needed. This would probably help with #1492626: "Cart Items" triggers dont work. However, I don't know if additional work need to be done to register the events. I assume that it does.
Do more auto-tests need to be added?
In other news, I've been using #17, #19, and then #23 to attach a computed field and haven't found any serious problems. It's quite useful for a report I need. The minor problem I've found is that if you add a field and then use it in a View, older records won't have the attached field, and if the View tries to use those records, it gets a lot of errors. That's to be expected, though.
#26
Applying this patch causes WSOD on the checkout page for me if there is a product kit in the order. The cart page is fine. So, this one is pretty tricky to troubleshoot.
Is anyone else experiencing this with #23? Do the auto-tests cover this situation? I'm surprised that they passed....
#27
Yeah, it can't be committed with that problem.
#28
Oh, wow. I tracked this down to an unbelievably dirty hack in uc_product_kit_uc_cart_display($item). Look at this:
<?phpif (!isset($item->type)) {
$node = node_load($item->nid);
$item->type = $node->type;
}
$build = node_view($item);
?>
This worked so long as $item wasn't an entity. Now, though, it's running
node_view()on auc_order_productentity, and that barfs all over everything, obviously.My approach to fix it is to change the above abomination to this:
<?php$node = node_load($item->nid);
if (!isset($item->type)) {
$item->type = $node->type;
}
$build = node_view($node);
?>
...or even just this seems to work. Is there any reason to worry about
$item->typeafter this?<?php$node = node_load($item->nid);
$build = node_view($node);
?>
Does this look right? Are there any other land mines to watch out for?
Anyway, patch with the (second) fix attached.
#29
The last submitted patch, ubercart_fieldable_ordered_products-1884512-28.patch, failed testing.
#30
That "dirty hack" was deliberately added in #1626838: Review Order fails with product kit in cart and required fields left empty, so we need a new fix or workaround for that problem.
#31
Actually, the hack is the
$build = node_view($item);, and #1626838: Review Order fails with product kit in cart and required fields left empty didn't touch that. Maybe I should avoid the name-calling, anyway.Do you remember the root cause of #1626838, and would the first fix (setting $item->type) fix it? With that fix in place, I'm not seeing the errors mentioned in #1626838. I I'll run the first fix through my own auto-test before I upload a patch.
#32
I think we should just remove hook_uc_order_product_delete()/hook_uc_order_edit_form_product_remove(). uc_order_edit_form_submit() already has some module_exists('uc_stock') checks to decrement stock if you change the quantity, so I don't really know why product removal isn't handled here as well. This solution is still ugly but better to keep all the stock related code in the one submit handler, where it can be refactored out later.
#33
It can't all be put in uc_order_edit_form_submit() without some major changes. Check the definition of the delete buttons on that form:
<?php$form['products'][$i]['remove'] = array(
'#type' => 'image_button',
'#title' => t('Remove this product.'),
'#src' => drupal_get_path('module', 'uc_store') . '/images/error.gif',
'#button_type' => 'remove',
'#submit' => array('uc_order_edit_products_remove', 'uc_order_edit_form_submit'),
'#return_value' => $product->order_product_id,
);
?>
So, pushing the button calls uc_order_edit_products_remove(), then uc_order_edit_form_submit(). By the time it gets to uc_order_edit_form_submit(), the product item is already deleted. I suppose it would be possible to move all the code over to the one submit function, figure out which button triggered it, then execute the delete code, but it's easier to just leave the delete code there and add the stock increment to uc_order_edit_products_remove().
So, I put the stock increment in uc_order_edit_products_remove(), removed the hook, and implemented the first fix from #28. Product kits on checkout are still causing WSOD on one of my test platforms, but not on other hosts. No idea why, especially since I have error reporting turned on, and it makes finding the bug really hard. Any assistance in troubleshooting would be welcome.
One hint: Attempting to add a product kit via the admin interface generates an AJAX/Krumo errors like "Undefined index: sub_products in uc_product_kit_uc_order_product_alter() (line 1035 of ...sites/all/modules/ubercart/uc_product_kit/uc_product_kit.module)" and "Notice: Undefined property: stdClass::$order_product_id in uc_order_edit_products_form() (line 385 of ...sites/all/modules/ubercart/uc_order/uc_order.order_pane.inc.
#34
The last submitted patch, ubercart_fieldable_ordered_products-1884512-33.patch, failed testing.
#35
Still haven't had much time to review this but your hint reminded me of #1105518: hook_uc_order_product_alter() is broken, which is another ugly part of the order editing code, and may be related.
#36
I'm now officially out of my depth, at least until I research this code some more . At least that gave me somewhere new to look. Here's some stuff I found. Any help with figuring this out is appreciated.
It didn't show up on the screen (just got WSOD), but this showed up in the log:
Notice: Undefined index: #total in theme_uc_cart_review_table() (line 498 of /var/www/html/drupal_dev/sites/all/modules/ubercart/uc_cart/uc_cart_checkout_pane.inc).Line 498 is
<?php$total = $display_item['#total'];
?>
There were similar messages for 'title' and 'qty'. So, the table of kit items is not properly populated at this point, possibly due to some sort of failure involving uc_product_kit_uc_cart_display().
uc_product_kit_uc_order_product_alter() has this bit of code:
<?php
$kit_product = uc_product_load_variant($kit_product->nid, $data);
$kit_product->qty = $qty;
drupal_alter('uc_order_product', $kit_product, $order);
// Save the individual item to the order.
uc_order_product_save($order->order_id, $kit_product);
?>
If I'm reading this right, this means that $kit_product is a product type (bundle?) of a Node object/entity, so using it in uc_order_product_save() (which expects a stdClass uc_order_product entity) is probably bad. Could this be the problem? What's the right way to transform it?
uc_order_edit_products_add() does almost the same thing.
Another hint: Products kits do work at checkout with #33 on a different test install with no fields attached to the uc_order_product entity. When I attach a field, I get the WSOD on that site, too.
Maybe there needs to be an auto-test involving putting a product kit through checkout, maybe with inclusive taxes and attributes involved.
#37
UbercartInclusiveTaxesTestCase which fails in the above tests is exactly that, a product kit involving products with attributes and inclusive taxes.
The code you're digging into is in some of the most complicated and fragile parts of Ubercart, and I'm not sure I fully understand it all either. As part of this patch perhaps we should try to at least add comments to explain what's going on, if not clean up some of this where possible.
#38
I still don't fully understand uc_product_kit_uc_cart_display() but let's see if this passes the tests.
#39
The last submitted patch, 1884512-order-product-entity-38.patch, failed testing.
#40
Eureka! I just found (at least) part of the problem:
It's in theme_uc_cart_review_table()
<?phpforeach (element_children($display_items['uc_order_product']) as $key) {
$display_item = $display_items['uc_order_product'][$key];
if (count(element_children($display_item))) {
?>
The last line is the problem. Normally, a product kit display sub-item shouldn't be displayed here, and all of its elements will start with "#", and element_children() will return an empty array. However, if it's fieldable and has a field, that's no longer the case. It will have a non-# item.
It needs to ignore those items.
This fixes the checkout screen problem. Not sure about other problems.
#41
Here's the change mentioned in #40.
It might be a good idea to put some sanity checking into uc_order_product_save() to deal with with this being called on non-stdClass objects, but let's try it with just this change for now.
Figuring this out required that I learn xdebug and Netbeans, but I'm not bitter.
#42
The last submitted patch, ubercart_fieldable_ordered_products-1884512-41.patch, failed testing.
#43
Oops, bad patch. Trying again. Also putting in the change in #38.
#44
#45
The last submitted patch, ubercart_fieldable_ordered_products-1884512-43.patch, failed testing.
#46
I'm likely to dive into the inclusive taxes problem this weekend, but I know nothing about that code yet. Any pointers on where to look so I don't spend hours (or days) barking up the wrong tree? I'm guessing that there's some code to load inclusive taxes into the $oproduct->data array, right? (I also wouldn't be offended if someone found and fixed this before I did.)
#47
I've done some troubleshooting. I haven't pinned it down, yet, but I've discovered that it's not an issue with the inclusive taxes. The issue is that it doesn't load in attribute prices for products in a kit when at the checkout page. The attribute prices are fine at the cart page.
It seems to be coming back with the wrong prices after the entity_view. I haven't determined if that's because it's failed to get the right prices, or because it isn't supposed to have the right prices yet.
#48
Got it.
The problem was my "fix" from #28. Node_view() needs to have the contents of
$item->attributes(not just$item->data->attributes) in order to add in the attribute price to$build['display_price']['#value'].One fix would be to copy the entire contents of $item to $node before calling node_view(). This would cover any other modules that generate a variant of a product.
However, I tried putting the hack back in (reverting the "fix" in #28), and that worked for my tests. There was a crash there before, but it seems to have gone away now. Perhaps the crash was simply a delayed effect of other issues, now fixed.
There are plenty of places where it would be good to clean up the code so the object types and entity types all line up properly. Also, calculation of attribute prices of ordered products via node_view() doesn't feel right. At this point, though, It might be better to get this feature committed and do the cleanup in a separate pass.
Anyway, here's a patch with #40 fixed, but #28 reverted. Let's see what the testbot thinks.
#49
Thanks, this is looking good now. A possible improvement is attached; this separates concerns between UcCartItemController and UcOrderProductController, as there doesn't seem to be much reason that one should extend the other. This moves the cart build mode into UcCartItemController and simplifies theme_uc_cart_review_table() and uc_taxes_entity_view_alter() as a result.
I am also not sure why we need to add uc_order_product_view(), nothing calls it and I don't really see when you would want to render a single order product rather than all of them (and you can just call entity_view() directly if you really need to).
#50
The last submitted patch, 1884512-order-product-entity-49.patch, failed testing.
#51
Failed because testbot's disk is full.
#49: 1884512-order-product-entity-49.patch queued for re-testing.
#52
What I can grok of that is a pretty good improvement, actually. It simplifies the code by separating those out.
uc_order_product_view() was really just for completeness to be available to outside modules. However, I think you're right. Nobody will want just one. It can go away.
Watch out for this line, which you put back in:
<?phpif (count(element_children($display_item)))
?>
See #40. I haven't followed your logic completely, but if $display_item is fieldable (or gets fields copied to it from the build), any attached fields will be recognized by element_children(), and you'll get a TRUE when you want a FALSE. This is the problem I had with kits. (Maybe you have some way of avoiding this issue, but I don't see it.)
Something like this might work better:
<?phpif (!empty($display_item['total']))
?>
I like splitting out the buildContent() and uc_taxes_entity_view_alter() code a lot. The current code is really confusing because there is one function handling two completely different situations.
This thing really needs some test cases added. It needs to run some tests without attached fields, attach a field, and repeat the tests. I guess it's time I looked into how to do that, but it will surely take me a while to figure out. Is that something that should go into this issue, or a separate one once this is done?
#53
If we're not sure that this will work in the intended circumstances (ie. being able to attach a field successfully) then we should add tests now to prove whether
if (count(element_children($display_item)))#54
#49 isn't working. With an attached field on uc_order_product and a product kit with two items, it displays the two items separately on checkout instead of grouping them together into the kit. The idea from #52 doesn't fix it. The kit displays just fine in the cart; it's checkout that's wrong. At least there are no crashes.
Going back to #48 does fix it.
It looks like the display elements for the review table aren't being built the right way.
Look at the entity_view() wrapper call in theme_uc_cart_review_table():
<?php$display_items = uc_order_product_view_multiple($items, 'cart');
?>
So, the view mode is cart here, even though this is at checkout.
UcOrderProductController::buildContent() for #49 no longer has the code for 'cart' mode. It just goes with the (former) default mode on every call. So, the #49 buildContent() never invokes a uc_product_kit_uc_cart_display(), so it never talks to uc_product_kit, so it can't be expected to display a kit the way the uc_product_kit module wants, right?
#55
Arguably I would say that this is actually the correct thing to do now; the cart page shows a summary of your cart, whereas the checkout page shows a summary of your order - and a product kit split into its component parts is what will also be shown on your invoice, your order view screen in your account post-checkout, etc.
#56
That's a good argument.
In that case, the entity_view() wrapper shouldn't be asking for a 'cart' display. I guess it should be 'full', 'default', 'checkout', or simply left blank.
Also, this line:
<?phpif (count(element_children($display_item)))
?>
...is probably no longer a concern. That line was originally there to avoid displaying hidden items on their own lines (like all but one of the items of a kit). Since UcOrderProductController::buildContent now always sets some non-'#' values, this will always be true. Now that we're just displaying everything, that line can just be removed.
What happens if two kits each have the same item and both kits go into the cart? Do their quantities get combined in the last steps or do they continue to show up as separate?
#57
I think they show up as separate, but this is also considered by design, for the case where the kits may discount the individual products differently.
#58
This removes the 'cart' view mode and the unnecessary if() noted in #56.
#59
The last submitted patch, 1884512-order-product-entity-58.patch, failed testing.
#60
#58: 1884512-order-product-entity-58.patch queued for re-testing.
#61
Heh, I was working on a patch at the same time to do pretty much the same thing, but then I got sucked into a couple meetings. This gets rid of uc_order_product_view(), so I'll throw it in.
What's the best way to make that nice interdiff? http://drupal.org/node/1488712 Seems kind of hairy....
Working from #49:
Removed uc_order_product_view().
Removed display argument ('cart') in call to uc_order_product_view_multiple().
Removed
if (count(element_children($display_item))) {...}conditional (and always execute the lines it surrounded).#62
The last submitted patch, ubercart_fieldable_ordered_products-1884512-61.patch, failed testing.
#63
#61: ubercart_fieldable_ordered_products-1884512-61.patch queued for re-testing.
#64
While I wait for testbot, possible followup to this issue: Enable the "ordered product display" tab, use it to select and arrange the fields that are displayed in the "Products" table in the admin and customer order view - so computed fields could be displayed here easily. Then, extend that further to allow rearranging and additional fields on the checkout and review pages.
I make interdiffs by working on a local git branch, committing when it's time for a new patch, then using git diff origin/7.x-3.x to make the patch and git diff HEAD^ to make the interdiff.
#65
Yes. This would allow a lot more flexibility beyond what attributes can currently do. For instance, you could use Ubercart as the front end for travel booking, and attach fields for things like flight times.
The ordered product items should probably be individually themable as order products. The current theme_uc_cart_review_table() themes a whole list of them at once into a table. (If they aren't already...I don't know much about themes yet.)
Thinking even further into the future, there could be order product (and cart product) entity bundles that parallel the product type bundles. Different kinds of products have different needs for additional data. You wouldn't really want that 'Coach Class' attached field on your product that represents a hotel stay.
#66
This is looking pretty good. What still remains to be done before committing this?
#67
I'd like to see this in 7.x-3.4, which I expect will be fairly soon. What needs to be done for that?
#68
#61: ubercart_fieldable_ordered_products-1884512-61.patch queued for re-testing.
#69
The last submitted patch, ubercart_fieldable_ordered_products-1884512-61.patch, failed testing.
#70
Well, that's a remarkably unhelpful error message. Does it just mean that the patch no longer applies, due to other changes in the files?
#71
Yeah there must have been some changes in the lines that the patch touches, I haven't tried applying it manually.
#72
It applied manually with only a little fuzz. So, I applied it and remade the patch. Let's see what the testbot thinks of that.
Assuming it passes the tests, what's next?
#73
Nothing, we're done :)
This works well for me, and as nobody else is likely to test it here, I've committed it to 7.x-3.x for further testing and we can deal with any followups separately.
#74
My test system is having an issue with this when I updated to the current -dev version. It still has the "save callback" as uc_order_product_entity_save(), so entity_save() calls that, which results in an infinite loop and an overflowing call stack.
Clearing the caches solves this problem, presumably because it loads the new entity description.
So, just checking: Should we do something to force a cache clear on this update? Does running update.php clear the caches even if there are no new hook_update_N() functions? If so, we don't need to do anything, and this can go back to "fixed".....
...etcetera....
#75
update.php clears cache for you, even if there are no actual updates, see #1049284: Running update.php to flush caches no longer works unless there are updates pending.
If you are running -dev versions of anything and regularly updating code or testing patches, you should always clear cache after doing so to avoid stale info hook implementations causing problems, as they did here.
#76
Automatically closed -- issue fixed for 2 weeks with no activity.