I am migrating from a CSV file which has no product IDs in it, they are autogenerated on insert. So, the system-of-record is SOURCE. Due to this, at an update MigrateDestinationEntityAPI uses entity_create and copies the various properties from the source row. This results in create => '' returned from CommerceProductEntityController::create staying in the product and drupal_write_record casts this to a zero killing the existing create. The fix is easy and should break nothing because this situation is extremely rare; typically an entity update is precluded by an entity_load which puts an the already existing created number in the product entity. We detect created-returned-create by strict checking for an empty string which is surely this rare situation: an entity_load or any code would put some integer/numeric in place.
The same is true for uid.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 1943602.entity_create_to_update.patch | 5.26 KB | rszrama |
| #5 | commerce_product_create_1943602.patch | 849 bytes | chx |
| #1 | commerce_product_create_1943602.patch | 989 bytes | chx |
| commerce_product_create.patch | 993 bytes | chx |
Comments
Comment #1
chx commentedComment #2
chx commentedComment #2.0
chx commentedUpdated issue summary.
Comment #4
chx commentedRemoving these from create() might lead to notices. Better to remove them during save. Do we want to restore them to an empty string after save perhaps?
Comment #5
chx commentedComment #6
rszrama commentedI suppose this would affect migrated customer profiles, orders, and payment transactions (also have created / uid) and line items (has created)? I can copy the fix over if so. And do we not need to consider any of the other properties, like changed or revision_uid?
Comment #7
chx commentedchanged, revision_timestap revision_uid are set in save() unconditionally. created (and product_id) are autogenerated only on create and thus need protection. uid is only relevant because it might be different to the original.
Comment #8
rszrama commentedAlrighty, I didn't extend the coverage to those properties but did attempt to translate your patch to the other entity controllers. The Order controller had a similar property in hostname that needed protection, though to actually wipe a hostname value I suppose it's harder now... you'd just have to use NULL instead of '', but honestly, I don't think this will be an issue.
As much as we tried to be consistent across controllers, there's actually a fair amount of inconsistency with respect to these similar properties. Apparently the product controller is the only one initializing is_new on ::create(), for example, and the if statement to determine whether or not created should be initialized is inconsistent. Additionally, the initial value for uid is all over the place...
global $user->uidfor payment transactions,0for customer profiles,''for products and orders. I don't wanna touch the payment transaction atm, but I did change the customer profile to''to see if that's a safe change. This all just kinda reeks of "magic" - we definitely need to have a more explicit consistency in the update to 2.x.I wonder, too, if we should seek an upstream accommodation in the Migrate contrib you're using. Or are we just unique among entity defining modules in preparing a full "empty" entity object on create?
Letting test bot have a crack at this before committing to make sure I didn't screw it up royally. : )
Comment #9
rszrama commentedCommitted.
Comment #10
damien tournoud commentedWhy are we setting those to
''instead ofnullin the first place in the create methods?Comment #11
rszrama commentedI honestly couldn't tell you other than that's what we did and I'm not sure it's feasible to change it now. I was even concerned about changing the customer profile to initialize to '' instead of 0. I think at the time I simply assumed we needed to initialize everything, and I think I for a while assumed that
'' === NULL.One more thing to fix in 2.x. Do we even need to initialize everything? Was that a relic of unstable Entity API perhaps? Or was it just over-aggressive initialization for no good reason?
Comment #12
damien tournoud commentedUntil we have base classes, we do need to initialize everything. Having "slots" for data is what classes are for :)
Comment #13.0
(not verified) commentedUpdated issue summary.