Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It would useful if the og_membership.inc MigrateDestinationOGMembership called the prepare() function right before the og_membership is saved and the complete() function after the roles are created in the import() function. This would be useful and also keep inline with the migrate conventions of commonly implemented functions. See other Migrate destination plugins import() functions for examples.
Comments
Comment #1
svilleintel CreditAttribution: svilleintel commentedComment #2
amitaibuPatch please :)
Comment #3
svilleintel CreditAttribution: svilleintel commentedI just had a brief play with it before and noticed its not a matter of a simple
$this->complete($entity, $row);
The class needs to extend MigrateDestinationEntity and not MigrateDestination. I will try write up and add a patch tomorrow morning.Comment #4
svilleintel CreditAttribution: svilleintel commentedSo i have attached a patch which now makes MigrateDestinationOGMembership extend MigrateDestinationEntity rather than MigrateDestination. Note: MigrateDestinationEntity extends MigrateDestination.
I noticed that some other destinations in Migrate such as menu and menu_links etc extend MigrateDestination directly and have their own implementation of prepare() and complete() very similar to MigrateDestinationEntity. Rather than re-using code i thought it would make more sense just to extend MigrateDestinationEntity and in this way it will use this classes prepare() and complete().
If anyone thinks it would be better to leave the class extend MigrateDestination and include prepare() and complete() functions like menu.inc (MigrateDestinationMenu) and menu_links.inc (MigrateDestinationMenuLinks) please let me know or write a patch. Other wise i think this way is fine.
Comment #5
amitaibuTrailing space.
Just to understand - why hardcode the default value of the bundle?
Comment #6
svilleintel CreditAttribution: svilleintel commentedI was not entirely sure on the bundle to be honest and as I believe the parent class MigrateDestinationEntity requires a bundle to be specified in the constructor. I also thought that if anyone else was instantiating the class in their code with just
new MigrateDestinationOGMembership()
then atleast it will still hopefully continue to work. I must admit though I'm not entirely sure on what the default bundle is or if it should have one. Perhaps someone such as yourself would have a better understanding of the working internals of og and be able to help out.Sorry about the trailing space.
Comment #7
svilleintel CreditAttribution: svilleintel commentedAttached is the MigrateDestinationEntity class. As you can see it already has the complete() and prepare() functions aswell as other helper functions implemented. You can also see here how i was mentioning the constructor requires a bundle.
The other way to do this and avoid specifying any bundle is to leave MigrateDestinationOGMembership extend MigrateDestination, (and like some other destination plugins such as menu and menu_links in the Migrate module) is to just implement the code directly into the MigrateDestinationOGMembership class ourselves. This code was taken from menu.inc and menu_links.inc destinations
As you can see the code is very similar to that of MigrateDestinationEntity and such we could avoid code re-use by inheriting from that class. Im happy either way. If you think its best we dont have a bundle then ill leave MigrateDestinationOGMembership extend MigrateDestination and write a patch including these two functions. Otherwise we could use the patch i uploaded above.
Please let me know your thoughts.
Comment #8
svilleintel CreditAttribution: svilleintel commentedThis is a patch that implements the second way I was mentioning and still lets MigrateDestinationOGMembership extend MigrateDestination without having to specify a bundle. The class now has its own prepare() and complete() functions which prepare handlers before calling any user implemented prepare() and complete() functions. I base this on other destination migrations implementations see attached.
Comment #9
amitaibuCommitted, thanks.
Comment #11
Angry Dan CreditAttribution: Angry Dan commentedSvilleintel, you were closer with your first patch.
MigrateDestinationOGMembership
should extendMigrateDestinationEntity
because og_membership's are entities - just like any node, taxonomy term or user.You were also correct with your bundle choice. It's a bit of an over simplification of the way things are done in OG, but more than sufficient for now. Rather than use the text, I'd suggest the constant
OG_MEMBERSHIP_TYPE_DEFAULT
instead though.+1 for the approach in your original patch to be committed.
Comment #12
Angry Dan CreditAttribution: Angry Dan commentedFollowing my Angry Dan style comment yesterday, I've created a new patch which correctly extends MigrateDestinationEntity, drops the extra methods and includes proper field handling.
Comment #13
amitaibuIF we're already there, shouldn't it extend
MigrateDestinationEntityAPI
?Comment #14
Angry Dan CreditAttribution: Angry Dan commentedThat would put an unnecessary dependency on Migrate Extras, a module by it's very principle which needs to be considered deprecated.
What are the benefits of MigrateDestinationEntityAPI over MigrateDestinationEntity that we would see when migrating OG memberships?
Comment #15
LSU_JBob CreditAttribution: LSU_JBob commentedI tired out the patch in #12 and am still getting an error after trying to import group memberships. I have all the fields set defined in the fields in MigrateDestinationOGMembership
Error I get is "Missing bundle property on entity of type og_membership"
Comment #16
LSU_JBob CreditAttribution: LSU_JBob commentedI updated Angry Dan's patch to include a "type" field. "Type" refers to the entity type for OG Membership entites, whereas "entity_type" refers to the related entity's type (a user) and "group_type" refers to the type of the group entity (node).
Comment #16.0
LSU_JBob CreditAttribution: LSU_JBob commentedforgot to mention the missing complete() function also