I've been trying out the alpha and so far so good, but I have several million rows of "bookmarks" from a non-drupal site that I need to migrate into my D7 rebuild.

I've tried using the migrate_extras MigrateDestinationFlagSimple class with version 2 of Flag and it works fine (with a few teething problems), but obviously it doesn't work at all with 3.x given the changes to the database. I've tried editing MigrateDestinationFlagSimple myself to reflect the 3.x changes, but somehow I'm missing something and it doesn't work.

Are there any plans to provide an updated MigrateDestinationFlagSimple class? migrate_extras is officially unmaintained, but submitting a patch on the issue queue is all I need.

Failing that, has anyone else got a working 3.x-supportive version of the migrate_extra plugin?

Files: 
CommentFileSizeAuthor
#9 flag-migrate-1988976-9.patch6.05 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 448 pass(es).
[ View ]

Comments

joachim’s picture

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

xcession’s picture

Agreed, that would be more convenient.

The migrate_extras issue queue already has a couple of useful patches for the 2.x version of MigrateDestinationFlagSimple - such as adding support for detecting already-imported rows - but it's a bit moot given the overhaul Flag 3.x is getting.

joachim’s picture

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

chr.fritsch’s picture

Here is a patch for migrate_extras to support flag 3

https://drupal.org/node/2029613

chr.fritsch’s picture

Status:Active» Needs review
StatusFileSize
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 254 pass(es).
[ View ]

And now, here is the patch

joachim’s picture

Status:Needs review» Needs work
+++ 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!

hussainweb’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new6.02 KB
PASSED: [[SimpleTest]]: [MySQL] 409 pass(es).
[ View ]

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

hussainweb’s picture

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.

hussainweb’s picture

StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 448 pass(es).
[ View ]

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.

joachim’s picture

Status:Needs review» Needs work

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 :(

Status:Needs work» Needs review
joachim’s picture

Status:Needs review» Needs work

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.