Comments

bojanz’s picture

Status: Active » Needs review
StatusFileSize
new6.99 KB

This code comes from Commerce Migrate and is used on several production projects already.

marcusx’s picture

subscribe

fago’s picture

Title: Add Migrate module integration » Add generic-entity integration
Project: Entity API » Migrate
Version: 7.x-1.x-dev » 7.x-2.x-dev
Component: Code - misc » Code

great 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.

mikeryan’s picture

Status: Needs review » Needs work
marvil07’s picture

Just making the patch apply on migrate, so not moving status by now.

mikeryan’s picture

I'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.

bojanz’s picture

To be honest, I don't care, as long as it's committed somewhere.

marvil07’s picture

Project: Migrate » Migrate Extras
Component: Code » Migrate Extras Features
Status: Needs work » Needs review
StatusFileSize
new6.95 KB

Actually 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?

marvil07’s picture

I 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 :-)

mikeryan’s picture

Status: Needs review » Needs work

Forgot 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...

marvil07’s picture

There has been two commits on commerce_migrate that affects the MigrateDestinationEntityAPI class:

commit 8df8c70cc913c7d8925bc9aa69217a6f8f7451db
Author: Randy Fay <randy@commerceguys.com>
Date:   Tue Jul 26 13:02:52 2011 -0600

    followup to #1230228 by rfay | bojanz: Improve order import performance; I missed one file and missed the same thing in line item

commit 945480d9d25a3cf683ffac3468efe02514b6c5f9
Author: Randy Fay <randy@commerceguys.com>
Date:   Fri Jul 29 13:21:13 2011 -0600

    - #1233856 by rfay: Fixed entity_load() needs to pass an array in MigrateDestinationEntityAPI.

Note: Please notice that this patch is really commerce_migrate version of MigrateDestinationEntityAPI plus 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.

rfay’s picture

Subscribe, of course. I'm actually not sure why this doesn't go into Migrate itself.

drewish’s picture

Subscribe. Using the version in commerce_migrate quite a bit.

drewish’s picture

Make sure we wait for #1282562: Allow updating entity records to get addressed before pulling in this code.

pcambra’s picture

I 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.

marvil07’s picture

Two more commits on commerce_migrate about MigrateDestinationEntityAPI class:

commit 8f04d647f428e391cc9bd361c0b824b0e679c7fb
Author: drewish <drewish@34869.no-reply.drupal.org>
Date:   Sat Sep 17 11:39:19 2011 +0200

    Issue #1282096 - Report counts of records created and updated

commit 16cc1edbf11444fbc81eafdd34a06e342b19bf93
Author: drewish <drewish@34869.no-reply.drupal.org>
Date:   Thu Nov 10 12:43:32 2011 +0100

    Issue #1282562: Fix problems when dealing with revisions.

And migrate_extras changed its info file, so, here a re-roll.

BTW I will open have 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.

aidanlis’s picture

Is this NW or NR? If NW, what needs to be done?

marvil07’s picture

@aidanlis: NW is because of comment 10

mikeryan’s picture

Status: Needs work » Fixed

In anticipation of a 2.3 RC1 release, I've gone ahead and committed this.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.