Add migration destination handlers

We should move Migrate support into the Flag project, where it can be better maintained and kept in tandem with branches.

Well we could bring those into the Flag 2.x branch, if they're not getting attention over at migrate_extras.

Patches very gratefully received! :)

Here is a patch for migrate_extras to support flag 3

https://drupal.org/node/2029613

And now, here is the patch

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+class MigrateDestinationFlag extends MigrateDestination {

If this is for importing flaggings, it should be called flagging, not flag.

Also, we should inherit from MigrateDestinationEntityAPI maybe, or MigrateDestinationEntity at least.

I for one would be fine for Flag migrations to depend on Entity API even if Flag module itself does not.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+   * Delete a membership.

Doesn't match function name.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+   * Import a single flag_content.

There is no longer a flag_content.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+
+

Surplus blank lines.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+              array('!id' => $entity->flagging_id, '!destid' => $row->migrate_map_destid1)
+            ));

Surplus indentation.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+    // Otherwise, clear out the fcid, just to be safe.
+    else {
+      unset($entity->flagging_id);

Comment and code don't match.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+    if (!empty($entity->flagging_id)) {
+      $return = drupal_write_record('flagging', $entity, array('flagging_id'));
+    }
+    else {
+      $return = drupal_write_record('flagging', $entity);

The Flag API should be used here. Migrate uses node_save(), for example.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+      ($return == SAVED_NEW) ? $this->numCreated++ : $this->numUpdated++;

Urgh. Please write this out as a proper if-then-else, as this is hard to read.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+      // Update the flag_counts table.
+      $count = db_select('flagging')->fields('flagging')->condition('fid', $this->fid)->condition('entity_type', $entity->entity_type)->condition('entity_id', $entity->entity_type)->countQuery()->execute()->fetchField();
+
+      db_merge('flag_counts')->key(array(
+          'fid' => $this->fid,
+          'entity_id' => $entity->entity_id,
+        ))->fields(array(
+          'entity_type' => $entity->entity_type,
+          'count' => $count,
+          'last_updated' => REQUEST_TIME,
+        ))->execute();

This should not be necessary if going through the API, surely.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+  public function fields() {

Needs docblock.

+++ b/flag.migrate.inc
@@ -0,0 +1,204 @@
+/**
+ * Because we can't identify what kind of entity is passed to complete, we
+ * implement a separate handler for each type.
+ */
+class MigrateNodeFlagHandler extends MigrateFlagHandler {

This doesn't look sustainable now that flags can be on any entity!

I have improved the patch based on the feedback in #6. I tested a migration with this and it works fine.

I have been thinking further and wondering if it will be a good idea to add a new module within flag to provide migration support. Apart from the destination handler, it can also provide a dynamic migration class similar to migrate_d2d.

Does it make sense to have such a module within flag? Or is it a better idea to have it as a separate project? I think it might make sense within flag module as the migration support will always match the flag version used.

I am attaching another patch with two tiny modifications to skip permissions check when flagging content in destination handler. It adds a parameter to skip permission check in complete method in MigrateNodeFlaggingHandler and MigrateUserFlaggingHandler.

Looks like this is coming along nicely. I'm a bit confused over the different kinds of classes in this, but I probably need to go read up on destination handlers in Migrate.

1. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   + * Destination class implementing when you want just an insert into flagging table.
   

   This needs trimming to 80 chars.

2. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   +  public function __construct($flag_name) {
   

   The parameters here need documenting: Migrate destinations have different parameters.

3. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   +    // Update timestamp.
   +    db_update('flagging')
   +      ->fields(array('timestamp' => $timestamp))
   +      ->condition('flagging_id', $flagging->flagging_id)
   +      ->execute();
   +    $flagging->timestamp = $timestamp;
   

   I don't get what this bit is for. Is this because the Flag API always forces the timestamp? We could perhaps consider changing that.

4. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   +  public function fields() {
   +    return array(
   +      'flagging_id' => 'Flag content ID',
   +      'fid' => 'Flag ID',
   +      'entity_type' => 'Entity Type',
   +      'entity_id' => 'Entity ID',
   +      'uid' => 'User ID',
   +      'timestamp' => 'Flagging timestamp',
   +    );
   

   This should perhaps call its parent to get Field API fields too?

5. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   + * Field handler - this will expose flags as fields on the object they're
   + * attached to, and set the flag to the value mapped in addFieldMapping().
   

   This needs to be split into a 1-line summary, and then more detail.

6. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   +    if (module_exists('flag')) {
   

   Presumably this class is only found if flag module is present!

7. +++ b/flag.migrate.inc
   @@ -0,0 +1,217 @@
   + * Because we can't identify what kind of entity is passed to complete, we
   + * implement a separate handler for each type.
   

   Wow! That's a PITA :(

There's no test coverage at all for Migrate integration, so any patch for this is going to pass tests trivially!

This patch needs work based on my previous comment.

I think the work done in #2503815: Flag destination class for migrate module is much more recent and complete. That issue just deals with a destination class. Here, we also have destination handlers which are worth talking about separately.

Let's deal with the destination class in the other issue and talk about introducing handlers here. I already combined the good parts from the patch here to the patch in the other issue.

I am renaming the title to that effect. This does not have anything to do with migrate_extras at all. I am also marking this postponed on #2503815: Flag destination class for migrate module.