Implement Migration Paths for Flag 7.x

I just used the patch from #52 and got an unrelated error:

Error: Call to a member function id() on null in Drupal\flag\FlagCountManager->incrementFlagCounts() (line 202 of FlagCountManager.php)

I created a seperate issue for solving this probleme here: 🐛 FlagCountManager: Call to a member function id() on null Needs review .

Just commenting here, for visibility sake.

@mradcliffe what is the foundation you create this MR from? Patch #52? Patch #60? Your own implementation? Would be nice to know, to finally get this issue resolved and implemented.

@Grevil, I based in on patch #60 using commit bd986011de72a6257c51342c2de4fd284516fdc9, "Issue #2409901 by brunodbo, schneidolf, socketwench, dieuwe, marvil07: Implements migration paths for flag 7.x patch 60"

I'm sorry, I started working on it, but then had to set it aside without being able to comment.

OK, thanks for clearing this up! Based on the comments here I was a bit confused, which patch actually works and which doesn't (and which
patch needs further tinkering).

Adding an interdiff here, to have a bit more insight on what actually changed between patches 52 to 56.

I used patch #52 and faced the issue mentioned in #50 too. In my case it was caused by the fact that flag entities were not created for profile flags because "flag_type" remained "entity:profile2" while it should be "entity:profile".

To fix it I have altered my migrate_plus.migration.upgrade_d7_flag.yml my making the flag_type block look as follows:

  flag_type:
    -
      plugin: get
      source: flag_type
    -
      plugin: static_map
      map:
        'entity:profile2': 'entity:profile'
      bypass: true
    -
      plugin: static_map
      map:
        'entity:field_collection_item': 'entity:paragraph'
        'entity:paragraphs_item': 'entity:paragraph'
      bypass: true

I was able to make the migration work by using patch #52 and following a similar approach as described in #59.
Instead of migration_lookup, I used the entity_lookup plugin.

I added the following code to the migrations/d7_flagging.yml file:

entity_id:
    - plugin: entity_lookup
      entity_type: node
      value_key: nid
      bundle: 1
      bundle_key: status
      source: entity_id
    - plugin: skip_on_empty
      method: row

In this case, it looks up all nodes for a matching entity id.

Alright, I'll try to unravel this Patch mess in the current MR, but these are the main corner points:

Patch 52 feels like the "base" implementation of this issue

Patch 56 enforces you to insert your own custom user and content migrations and shouldn't be used without these modifications BUT it also introduced the very importa "skip_on_empty" plugin!

Patch 60 is still a bit unclear to me, since flag_type seems to be the same as entity_type? This is VERY UNCLEAR in the Flag code.

If found this:

'flag_type' => $flag->entity_type

and the flag_types schema definition which maps flag ids to their flaggable bundles? It is called "type" and its type is "varchar", but the description says "bundles, see https://git.drupalcode.org/project/flag/-/blob/7.x-3.x/flag.install#L134.

So I guess one flag only had one flag_type and the changes from patch #60 are just in case for custom preprocessing? Really unsure about this one.

And finally the issue fork "2409901-implement-migration-paths" which is based on patch 60 and brings a bunch of fixture and test changes with it.

I'll try to adjust the issue fork and consider comments 83 and 84 while doing so.

Concerning comment #83, I don't think, we should ship the migration with this mapping, since profile2 is a contrib module. Same with paragraphs. But very helpful for people running into it!

Concerning comment #84: "entity_lookup" isn't a Drupal core migration plugin. But instead, it is provided by "Migrate Plus", meaning if we would use it, we have to somehow add a dependency on that module only for migrations.

Concerning comment #84: "entity_lookup" isn't a Drupal core migration plugin. But instead, it is provided by "Migrate Plus", meaning if we would use it, we have to somehow add a dependency on that module only for migrations.

It probably makes sense to make a deriver for flagging that will make migrations per entity type, and then the migrate_lookup plugin gets the relevant migration for core entity types. Sort of like Node Complete/Classic migrations.

@mradcliffe, good point! Although I think this would take more time implementing then I originally thought. So unfortunately I have to drop it for now 😢.

For anyone curious and willing to take this on! Here is a very good example on how to define a deriver inside the migration yml: https://www.drupal.org/docs/drupal-apis/migrate-api/writing-migrations-f... →

And the Deriver class used in the example above, can be found here: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/...

I don't quite understand yet, how to dynamically list the affected core entity type derivatives in the "entity_id" "migration" yml entry. For example, in my specific case, the "entity_id" migration yml needs to look like this:

  entity_id:
    -
      plugin: migration_lookup
      source: entity_id
      migration:
        - upgrade_d7_node_complete_article
        - upgrade_d7_node_complete_blog
    -
      plugin: skip_on_empty
      method: row

Is it enough, to simply set "d7_node_complete" as a migration entry?

We also need to fix the tests.

I successfully migrated using #52 as-is.

I tried almost everything I found here: patch 52, 56, 60, migration through drupal UI and using drush. Flag migration is perfectly succesful, but the Flagging migrations always failed:

Processed 378 items (0 created, 0 updated, 0 failed, 378 ignored) - done with 'upgrade_d7_flagging

The "flagging" table remains empty. No idea about why.

At the end I decided to migrate all flaggins programmatically: after all they are only 378 (nodes) and I need this once only. I exported data from the old database table to csv; I made a little script to read data, flag automatically nodes from csv. It works (except for flagging creation date, but I solved also this if anyone interested)