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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

svilleintel’s picture

Title: MigrateDestinationOGMembership prepare() call missing » MigrateDestinationOGMembership prepare() and complete() calls missing
amitaibu’s picture

Patch please :)

svilleintel’s picture

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

svilleintel’s picture

Status: Active » Needs review
FileSize
1.26 KB

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

amitaibu’s picture

+++ b/includes/migrate/plugins/destinations/og_membership.incundefined
@@ -8,7 +8,11 @@
+    ¶

Trailing space.

+++ b/includes/migrate/plugins/destinations/og_membership.incundefined
@@ -8,7 +8,11 @@
+  public function __construct($bundle = 'og_membership_type_default', array $options = array()) {

Just to understand - why hardcode the default value of the bundle?

svilleintel’s picture

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

svilleintel’s picture

FileSize
5.57 KB
3.35 KB

Attached 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

  /**
   * Implementation of MigrateDestination::prepare().
   */
  public function prepare($menu, stdClass $row) {
    // We do nothing here but allow child classes to act.
    $migration = Migration::currentMigration();
    $menu->migrate = array(
      'machineName' => $migration->getMachineName(),
    );

    // Call any general handlers.
    migrate_handler_invoke_all('menu', 'prepare', $menu, $row);
    // Then call any prepare handler for this specific Migration.
    if (method_exists($migration, 'prepare')) {
      $migration->prepare($menu, $row);
    }
  }

  public function complete($menu, stdClass $row) {
    // We do nothing here but allow child classes to act.
    $migration = Migration::currentMigration();
    $menu->migrate = array(
      'machineName' => $migration->getMachineName(),
    );
    // Call any general handlers.
    migrate_handler_invoke_all('menu', 'complete', $menu, $row);
    // Then call any complete handler for this specific Migration.
    if (method_exists($migration, 'complete')) {
      $migration->complete($menu, $row);
    }
  }

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.

svilleintel’s picture

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

amitaibu’s picture

Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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

Angry Dan’s picture

Title: MigrateDestinationOGMembership prepare() and complete() calls missing » MigrateDestinationOGMembership should extend MigrateDestinationEntity
Status: Closed (fixed) » Needs work

Svilleintel, you were closer with your first patch. MigrateDestinationOGMembership should extend MigrateDestinationEntity 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.

Angry Dan’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

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

amitaibu’s picture

IF we're already there, shouldn't it extend MigrateDestinationEntityAPI?

Angry Dan’s picture

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

LSU_JBob’s picture

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

LSU_JBob’s picture

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

LSU_JBob’s picture

Issue summary: View changes

forgot to mention the missing complete() function also

  • amitaibu committed 4543291 on 8.x-1.x authored by svilleintel
    Issue #1884874 by svilleintel: Added MigrateDestinationOGMembership...