First of all, sorry, this is not really a bug report, but this is neither really a task or feature request, so I just left the default issue category.

I'm doing heavy migration jobs using commerce, I use a custom code base in order to achieve this. This code base uses a generic entity injection layer for loading data into the database, which works with pretty much everything (entity beauty).

The injection process works sequentially, first orders, then line items.

Order save goes well, even if entity metadata wrappers tend to create insane bugs, in most case I can work arround it (by the way, you definitely should get rid of this insane abstraction layer for internals, it tends to over validate data and cause real problems more than it solves).

Line item saving goes well to, I use directly the line item entity controller to save fresh new instances.

Those instances I created all carry the right order_id property: the expected result would be that, when saving the line item the order fields to be updated accordingly without having to load again the order, add the reference manually, then save it again.

I succeeded in working arround this problem by proceeding to arbitrary SQL inserts in fields tables, then clearing the cache, this works and solve many problems (and is a lot faster than loading and saving the order), but numerous problems remains: for example the total order price is not updated.

In most part of your code, each object tree cascade save that needs to update stuff often proceed by loading and saving the updated dependent entity. I explain: for example, removing a line item will check for other entities referencing it, load them (using the EFQ), update them, save them back. This cause a dependency problem when it comes to save and update: if I save a line item and update its order as a dependency (for example using hooks), the order will save its line items while saving itself and create a potential circular dependency (and potential infinite loop).

You probably already have failsafe mecanisms for those potential infinite loops, since you are using a lot rules, I guess you already have thought of that (honestly, I read a lot of commerce code, but not everything yet) but it still afraids me enough so I wont not tempt the devil and try.

You don't have this problem because the update and save operations over line item actually doesn't update the order, but this is a problem: it should because altering the line items does not update the referencing order and create a desynchronization between saved order total and real computed total using the line items.

For your information, I'm actually gonna write update code for orders to update their price without loading and saving back the entity, it will save me a lot of SQL requests and time, but also avoid me the potential circular dependency problem.

This module has a strong API based approach, it's erally nice, but some problems remains, maybe having a more granular approach to entities update would solve some, for example being able to update the total price without loading and saving the full order entity, minor stuff like that. Even for rules integration and processing it would make the overall API to be faster and probably cause less problems.

I guess I cannot judge because I still didn't worked enough with it, but here is what I found out/experienced while working deeply really low level with the API.

Just a note about performances, using my API I can inject 1 million nodes with fields in Drupal 6 in approximatively 1h15 (on my box, figures is not really revelant), but using the same code, entity based in Drupal 7, injecting 10,000 orders take 1h30 (same hardware, same database, more or less same amount of fields into the entity than in my Drupal 6 nodes). It's extremelly slow, even disabling transactions, additional database checks (tuning and stuff): doing some profiling shows me that inserting a single order without additional fields goes up to an average of 30 requests, while inserting a single line item with approximatively 20 fields goes up to 80 requests average (including cache stuff, additional field API and misc core stuff happenning) which explains why this massive data injection is so slow.

Thank you for your time.

Comments

pounard’s picture

Update: commerce order save went down to 13 SQL request average with is way better once I fixed watchdog PHP notices (not Commerce related).

pounard’s picture

New update: using a null object implementation for the cache_field cache bin made a 40% performance boost while injecting order lines on my environment, switching all caches to the null implementation actually makes it worst.

rszrama’s picture

Thanks so much for the in depth report on what it's like to work directly with the APIs. : )

Here's a question for you: do you think it would be worthwhile to pursue a single API function that could save an order, line items, and customer profile data all at once? I ask because I did a Services integration for a client where I ended up writing a single REST resource that could take order header / line item data and save everything at once so they didn't have to submit multiple API requests just to save a new order. Moving something like this closer to core or at least into the Commerce Migrate module may be very helpful for developers with your needs.

Let's keep thinking outside the box, though - I bet there are more things we can do to simplify it. For example, the reason your order totals weren't being recalculated is because that happens on order save based on the current line items on the order. It might be nice to have an API function that directly updates that field in the database without having to do a full order save or something similar. (Though that might be hairy if we get into tracking revisions...)

pounard’s picture

Here's a question for you: do you think it would be worthwhile to pursue a single API function that could save an order, line items, and customer profile data all at once?

I would definitely ensure that the low level atomic API works fine first, ensure that line item save and update really does update the order first. But once you have this working, high level functions are always welcome as long as they actually don't conflict with the smaller ones.

Moving something like this closer to core or at least into the Commerce Migrate module may be very helpful for developers with your needs.

It can be helpful yes, I guess. Not for me thought because I'm receiving data from sequential sources, so I still would need the atomic API to work flawlessly.

For example, the reason your order totals weren't being recalculated is because that happens on order save based on the current line items on the order. It might be nice to have an API function that directly updates that field in the database without having to do a full order save or something similar.

I was thinking more generally, each entity update should ensure that all related stuff are updated accordingly. And while we're at it, for example, line item update or save should not save the full order once again (all fields would be updated even if non modified) but changes should be done to the minimal using a less generic and more business oriented approach inside the save handlers. This cause one huge problem though: triggering rules events will become harder to track because you'll have more cases to handle.
Another difficulty is you have to flag the line item update when it's being done while saving the order, because you don't need to re-update once again the order if it's the top level element of the object tree you're saving.

Once upon a time I was actually developing this kind of system entirely using PL/PgSQL which was fully relying on database triggers and procedures. It was safer and easier this way :)

Damien Tournoud’s picture

Cascading updates is definitely one of the pain points of Commerce 1.x. The initial plan was to make the save method of line items and customer profiles private.

Sadly, it's going to have to wait for Commerce 2.x, because we overly rely on the entity metadata wrapper that doesn't support adding reference to an entity that is not saved yet.

In Commerce 1.x, you have to save the line item first, then go back and attach it to the order. The order_id field of the line item must be considered as a denormalization.

pounard’s picture

Cascading updates is definitely one of the pain point of Commerce 1.x. The initial plan was to make the save method of line items and customer profiles private.

How sad it is to hear that, being able to manipulate line items directly is really something that worth it! Especially when doing migration or maintenance jobs.

FYI, I did succeed by overriding the line item controller and making it do arbitrary SQL queries to update the orders, and it works fine. I had to build the component array by hand because the API helpers were not working, which was a pain in the ass since it's not really documented. I read the full commerce order and price code, and I figured out why the component array was a benefit, but IMHO this is a pain to manipulate in the database, maybe this component array should be built at load time and cached, instead of being built at save time and made persistent? It really make it hard for developers to play with the API if those kind of obfuscated serialized data gets to the database, denormalizes data, and takes over the real data being saved at display time (it happens to me, can't remember where exactly).
I'm using a flag system in the order controller that I also overrided to determine if I'm actually saving the order or not when saving a line item, so I can disable my manual arbitrary SQL injection if the top level element being saved is an order.

For DX, being able to manipulate line items outside of the order context is definitely a huge benefit, and this kind of pain worth to be felt if it leads to this kind of more granular API.

fago’s picture

Order save goes well, even if entity metadata wrappers tend to create insane bugs, in most case I can work arround it (by the way, you definitely should get rid of this insane abstraction layer for internals, it tends to over validate data and cause real problems more than it solves).

Oh, could you please elaborate on the problems you ran into? Anything that should be fixed?

Sadly, it's going to have to wait for Commerce 2.x, because we overly rely on the entity metadata wrapper that doesn't support adding reference to an entity that is not saved yet.

Indeed. That would be nice to have, but I can't think of a good way to make this work generically. Probably, it's best to treat it as "out-of-scope" for the wrappers, but solve it directly with lower-level APIs - e.g. I've done so for solving this problem in field-collection:
Creating a new field-collection registers it in its "host entity" while saving the host triggers saving the new field-collection too. So you can use the wrappers to create a field-collection while the low-level APIs care about creating the link.

pounard’s picture

The more low level internals will be, clearer will be the design for external developers to guess! Entity wrappers tends to obfuscate what is really happening under the hood, and make the whole a lot less understandable.

Most of the problems I experienced are due to strong type checking by the entity wrappers, for example:
1. Saving an incomplete entity (relying on database defaults) will fail because the wrapper sometimes intercept NULL values or missing values (can't remember the exact use case, but I got this one more than once).
2. Saving data with loose data typing (int as a string in the object, but still numeric for PHP loose typing) goes to entity wrapper errors even before the SQL layer, which is annoying.
3. Misc stuff like that (got a lot) always related to data typing mainly.

lsolesen’s picture

Status: Active » Postponed (maintainer needs more info)

@pounard and @rszrama This issue is very broad. Could we spawn off some more clear issues of this? Should this be moved to the 2.x-version or will it still be on the radar of 1.x?

bojanz’s picture

Status: Postponed (maintainer needs more info) » Closed (outdated)

Time to close this.