Closed (fixed)
Project:
Migrate Extras
Version:
7.x-2.x-dev
Component:
Migrate Extras Features
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
25 May 2011 at 09:09 UTC
Updated:
11 Mar 2012 at 23:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
bojanz commentedThis code comes from Commerce Migrate and is used on several production projects already.
Comment #2
marcusx commentedsubscribe
Comment #3
fagogreat patch, however it don't fee like this suits into the entity API module as this code just generically deals with entities. Ideally, most modules do so self, thus this should go into migrate or a separate add-on.
Else, the entity API would became a dumping-ground for any entity-generic code, which doesn't sound right to me.
Note that the integration we currently have, is just about integrating the entity types that are provided with the entity API itself with other modules - so this is different.
@code:
As migrate-n00b this code seems to only offer entity-properties that are in the main-entity base table as target slots, but then uses entity_save() to save them. But when you do entity_save() anyway, I see no cause to restrict the properties to those that directly match db columns. Else, you could just restrict them to "writeable" properties by looking for an existing "setter callback".
Also there is a helper function, entity_property_values_create_entity() which helps with generically creating entities + setting properties via the wrapper, i.e. how rules creates entities.
Comment #4
mikeryanComment #5
marvil07 commentedJust making the patch apply on migrate, so not moving status by now.
Comment #6
mikeryanI'm a bit torn here - we do have the principle of supporting contrib modules in migrate_extras rather than migrate, and Entity API is less widely used than modules like Date and Pathauto. On the other hand, it represents a path to the future of core as proposed by fago in #1184944: Make entities classed objects, introduce CRUD support (and I'm certainly all for that future).
I'm currently leaning towards moving it into migrate_extras - I expect any project using modules dependent on Entity API will need migration support for at least one of the other modules supported by migrate_extras anyway. But I could be convinced to put it directly into Migrate itself.
Comment #7
bojanz commentedTo be honest, I don't care, as long as it's committed somewhere.
Comment #8
marvil07 commentedActually this started on migrate extras :-) #1133914: EntityAPI destination plugin
So, moving back to migrate_extras(IMHO it's fair to have it there on 7.x, let's move it to migrate on 8.x ;-)) and making the patch apply there.
BTW, just because this is not said here, this integration is actually on Commerce migrate now; so it is actually working. So changing status to NR, and maybe is better to get this in, and open another issue for solving fago comment.
Does it sound right?
Comment #9
marvil07 commentedI just changed the fields() method to show all properties as recommended by fago.
Currently it seems like all values are stored correctly without any other change, so I think this is ready enough :-)
Comment #10
mikeryanForgot to say last time, this is one that I think could really use an example and tests (see migrate_extras_date and date.inc). I can take a stab at adding those myself, but not for a couple of days...
Comment #11
marvil07 commentedThere has been two commits on commerce_migrate that affects the
MigrateDestinationEntityAPIclass:Note: Please notice that this patch is really commerce_migrate version of
MigrateDestinationEntityAPIplus a changed the fields() method which shows all properties as recommended by fago at comment #3(for easy reference I am also attaching a patch just for that change this time).PS: I'm not changing status since a example and tests are needed as mentioned by mikeryan.
Comment #12
rfaySubscribe, of course. I'm actually not sure why this doesn't go into Migrate itself.
Comment #13
drewish commentedSubscribe. Using the version in commerce_migrate quite a bit.
Comment #14
drewish commentedMake sure we wait for #1282562: Allow updating entity records to get addressed before pulling in this code.
Comment #15
pcambraI think this should go to Entity API module, or at least Migrate itself, is generic enough to be there, this is even more generic than Node or User support as you could migrate also those with just this component.
Let's not block this, I think this is functional enough to get in somewhere.
Comment #16
marvil07 commentedTwo more commits on commerce_migrate about
MigrateDestinationEntityAPIclass:And migrate_extras changed its info file, so, here a re-roll.
BTW I
will openhave opened one issue(#1416712: Show all entity properties which have a setter callback on MigrateDestinationEntityAPI::fields()) about the extra change is this patch mentioned in comment 11 to track all changes on commerce_migrate until it get in somewhere.Comment #17
aidanlis commentedIs this NW or NR? If NW, what needs to be done?
Comment #18
marvil07 commented@aidanlis: NW is because of comment 10
Comment #19
mikeryanIn anticipation of a 2.3 RC1 release, I've gone ahead and committed this.