Flag destination class for migrate module

That sounds great!

(Though AFAIK, Migrate D8 and Migrate D7 are very different, so I don't know how much it will be possible to apply the D7 work to D8. But we'll see!)

Since Migrate Extras recommends implementation within contributed modules, I agree with this initiative. I've included a destination class from the Migrate Extras module into Flag, and implemented a patch from https://www.drupal.org/node/2029613#comment-9805289 to handle version 3 and below of the Flag module. Shout out to @chr.fritsch, @ultimike, @DrupalChimp, and @berenddeboer for their contributions there.

shoot, missed testing a rollback and migrate. getting a "duplicate key value violates unique constraint". Will try to resolve today.

EDIT: Never mind, this doesn't seem to be an issue this morning after a clean `migrate-deregister` and `migrate-register`;

Thanks for getting this going again!

> Since Migrate Extras recommends implementation within contributed modules

I agree with that as well. It will also allow this code to be simplified quite a bit.

A few things need more work though:

1. +++ b/flag.migrate.inc
   @@ -0,0 +1,197 @@
   +    if (FLAG_API_VERSION == 3) {
   

   You don't need this if we're in Flag module, as you know we're on the 3.x branch.

   This code will all be much easier to read once you replace $this->foo with hardcoded table field names.

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

   Copy-pasted docs? ;)

3. +++ b/flag.migrate.inc
   @@ -0,0 +1,197 @@
   +    db_delete($this->tableName)
   +      ->condition($this->flaggingId, $ids, 'IN')
   +      ->execute();
   

   You're deleting from the flagging table directly, which is going to leave flag counts messed up.

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

   flag_content is 2.x table name.

5. +++ b/flag.migrate.inc
   @@ -0,0 +1,197 @@
   +   *   Object object to build. Pre-filled with any fields mapped in the
   

   'object object'

6. +++ b/flag.migrate.inc
   @@ -0,0 +1,197 @@
   +    if (!empty($entity->{$this->flaggingId})) {
   +      $return = drupal_write_record($this->tableName, $entity, $this->flaggingId);
   +    }
   +    else {
   +      $return = drupal_write_record($this->tableName, $entity);
   +    }
   

   I really think this should be going through the API (just as importing entities runs entity_save()).

I was able to address most of the points made, but am not sure what route to take for points 3 and 6 in #5 above.

> am not sure what route to take for points 3 and 6 in #5 above.

What I suggest is this: to create a flagging, you must have a flag ID, an entity ID, and (if non-global) a user ID. Rather than create the DB row yourself, call $flag->flag() and let the API handle it.

This change tackles point 3 in #5 above.

> Rather than create the DB row yourself, call $flag->flag() and let the API handle it.

I've now implemented this change.

I think there's something wrong with your last 2 patches!

whoops! yeah, i committed the previous patches, and included that in my patch export. my bad. fixed.

Found a bug when a UID is not passed from source, user_load($uid) was encountering an error. wrapped in condition to pass null account to flag() to fix.

Thanks @jcandan! #12 is working, but here are two notes:
1. When creating a new flag using $this->flag->flag('flag', $entity->entity_id, $account, TRUE);, you can't retrieve new $entity->flagging_id. So when you return it return array($entity->flagging_id);, there will be an error.
2. If fid field is auto populated here: $entity->fid = $this->flag->fid;, it might as well be removed from field list.

Glad you offered that feedback @Elaman. I'm not sure what you meant by number 2 in #13, but i noticed i very poorly constructed the conditionals around update vs. new. the flag->flag() method can handle updates and inserts. I'll also remove my flag_counts table update query, which i'm sure is unnecessary if i stick to using the flag() api call. i'll revisit this today which should also take care of these concerns you mentioned.

As @Elaman pointed out, the flagging entity id isn't returned, so I'm forced to use flag's getter. It's certainly not the best scenario. should we create a separate ticket to have Flag return the flagging entity? Other than that, this implementation is working. Let me know if you can commit it.

@jcandan, is it a good thing to exclude sid column? Won't it produce whole bunch of duplicates where uid is 0?

1. +++ b/flag.migrate.inc
   @@ -0,0 +1,173 @@
   + * Destination class to implement when you just want to insert into flag table.
   + *
   + * @see https://www.drupal.org/node/2661352 for example.
   

   The comment doesn't really make sense. It doesn't just insert data in the table.

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

   We could consider using MigrateDestinationEntity. Fields support for free.

I am making the above and more changes. I removed the hook_migrate_api (we don't need it to just define a destination class). I then changed direct deletes on tables and used ->flag('unflag') to rollback (so that fields are removed). It also now supports session id as a destination field.

There is an interdiff but I doubt if that will be useful. I am using the patch right now in my migration project and it works well.

Postponing #1988976: Add migration destination handlers on this. We can just continue to discuss field handlers in that issue. I think it's a good idea to keep the issues for destination class separate from handlers. The destination class is necessary whereas the field handlers are a nice to have but not of much use (especially since they don't have sub-fields support yet).

I've had a read of this, and it looks pretty good to me!

Just one thing I've spotted:

+++ b/flag.migrate.inc
@@ -0,0 +1,176 @@
+//    $uid = (!empty($account)) ? $account->uid : NULL;
+//    $flagging = $this->flag->get_flagging($entity->entity_id, $uid);

Should these lines be uncommented or removed?

Oh, I missed that. I wanted to test it without that block as I believe it's not necessary. It worked.

The flag method calls flagging_insert method which calls drupal_write_record, which sets the ID's after insert. This means calling a getter is not necessary.

Great, thanks!

Ideally I'd like a review by someone who's actually using Migrate before committing it. Hopefully one of the previous commenters can help :)

I've tested this on my recent migration. Everything (import/rollback) works fine, but here are some notes:
1. On rollback you will get PHP notices, because `flag` method requires user object, not user id.
2. Timestamps on import are not preserved, because they are forced to be REQUEST_TIME in `flagging_insert` method. Had to make minor change to `flag_flag` class.
3. `fid` must be hidden, since it is automatically defined when `$flag` object is loaded by machine name on `__constuct` method.
4. `flag_count` numbers are different from source.
5. Couldn't get `sid` column working.

I've attached the patch with 1, 2 and 3 fixed.

I've successfully used patch #25 to migrate flags from a D6 site to D7.
7.x-3.9 / PHP 7.1.1 / MariaDB 10.1.21

> 2. Timestamps on import are not preserved, because they are forced to be REQUEST_TIME in `flagging_insert` method. Had to make minor change to `flag_flag` class.

Sounds like a shortcoming of our CRUD code. IIRC, with nodes you can specify a 'created' timestamp to override the default behaviour of using REQUEST_TIME.

> 5. Couldn't get `sid` column working.

There's currently no sid column. There's an issue open to fix that...

I applied patch #25 and was able to successfully migrate flags. Thanks!

Hi to all.
I need to migrate from flag D7 to a brand new D7.
I already migrated nodes and comments, and flags are only associated to nodes.
I already read this: https://www.drupal.org/node/2661352 but it's for Flag2 migration.

Can someone help me in pointing out where's the difference and how to change that code to work for flags3 -> flags3 ?

Thanks a lot!

Patch #25 worked fine for me.

Drupal 7 is EOL. No more new features will be added. Issue will be closed, but patches are still here