Problem/Motivation

Flag 8.x does not currently supply a migration path from earlier versions.

Proposed resolution

Instead of relying on hook_update_N() to migrate configurations from previous versions, the new Migrate API should be used. This will have the additional benefit of allowing a direct migration from Flag 6.x to 8.x.

Remaining tasks

Write patch.

User interface changes

None.

API changes

None.

Issue fork flag-2409901

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

joachim’s picture

Status: Active » Postponed

I've filed a few issues relating to flag schema which should probably be tackled before this: #2412071: rename flag 'types' to 'subtypes', #2412069: rename flag is_global property to global, #2412063: rename flag id to flag name.

socketwench’s picture

Yep, makes sense.

joachim’s picture

There's just #2504833: rename Flag::types to bundles now I think. Was there an issue to change Flagging properties?

socketwench’s picture

I think there was an issue to rename flagging properties, but I can't seem to find it.

joachim’s picture

Status: Postponed » Active
brunodbo’s picture

StatusFileSize
new3.03 KB

Took a first stab at this. Attached patch successfully migrates part of D7 flags' configuration. I've been following https://drupalize.me/blog/201605/custom-drupal-drupal-migrations-migrate... to run test migrations on a copy of a D7 site with 12 configured flags.

Still very much work in progress:

- For the D7 flag configuration, the following isn't being migrated yet in this patch: bundles, link_type, flagTypeConfig, linkTypeConfig. I think bundles will have to come from the D7 flag_types table, not entirely sure yet how to get the data for those other three.
- Then there's the actual flaggings, flag_counts, and D6.

joachim’s picture

Thanks for making a start on this :)

> I think bundles will have to come from the D7 flag_types table

That's correct.

> link_type, flagTypeConfig, linkTypeConfig

See the options() method, which explains the serialized options property:

In the base class:

    $options = array(
      // The text for the "flag this" link for this flag.
      'flag_short' => '',
      // The description of the "flag this" link.
      'flag_long' => '',
      // Message displayed after flagging an entity.
      'flag_message' => '',
      // Likewise but for unflagged.
      'unflag_short' => '',
      'unflag_long' => '',
      'unflag_message' => '',
      'unflag_denied_text' => '',
      // The link type used by the flag, as defined in
      // hook_flag_link_type_info().
      'link_type' => 'toggle',
      'weight' => 0,
    );

link_type is just link_type.

In the flag entity class:

  function options() {
    $options = parent::options();
    $options += array(
      // Output the flag in the entity links.
      // This is empty for now and will get overriden for different
      // entities.
      // @see hook_entity_view().
      'show_in_links' => array(),
      // Output the flag as individual pseudofields.
      'show_as_field' => FALSE,
      // Add a checkbox for the flag in the entity form.
      // @see hook_field_attach_form().
      'show_on_form' => FALSE,
      'access_author' => '',
      'show_contextual_link' => FALSE,
    );

That gives you some of what's in flagTypeConfig.

For linkTypeConfig, see flag_flag_link_type_info().

brunodbo’s picture

StatusFileSize
new3.4 KB

Thanks! New patch, with bundles.

brunodbo’s picture

StatusFileSize
new4.31 KB

Adding link_type and flagTypeConfig

brunodbo’s picture

StatusFileSize
new4.63 KB

And linkTypeConfig

brunodbo’s picture

StatusFileSize
new14.31 KB

I'm a bit stuck: I added migration templates for flagging and flag_counts, along with source and destination plugins for both, but the plugins aren't being discovered. Meaning that when I run drush migrate-upgrade --configure-only, no migration configuration is being created for flagging and flag_counts. Not sure why that's happening (the flag plugins are still being discovered). Uploading the patch with that code, in case someone knows.

(Note that I wasn't able to test the flagging and flag_counts plugins yet, they most certainly need to be changed before they will work. Also wondering if we need migration templates at all, or if we should just supply migration configuration.)

joachim’s picture

brunodbo’s picture

StatusFileSize
new14.19 KB

The flag count/flagging migration plugins were being discovered, but no migration configuration was being created due to using the wrong source class. Flag counts are now being migrated, but I'm getting an error after migrating a few flaggings. Trying to figure out why that's happening.

Error: Call to a member function id() on null in Drupal\flag\FlagCountManager->incrementFlagCounts() (line 159 of /home/bruno/workspace/textillia_d8/drupal_composer/web/modules/contrib/flag/src/FlagCountManager.php)

brunodbo’s picture

Also, we'll want to disable incrementFlagCounts() during migration somehow. This might be relevant for that: #2684753: Add capability of disabling hooks (and events?) during migration.

brunodbo’s picture

StatusFileSize
new14.49 KB

Passing the flag_name instead of the D7 fid fixes the error in #13. Next up: passing the bundle in the flagging destination plugin, to fix this one (getting one of these for every flagging that's being migrated):

Missing bundle for entity type flagging (/home/bruno/workspace/textillia_d8/drupal_composer/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:83)

brunodbo’s picture

StatusFileSize
new15 KB

Cleaned up some things in the flagging migration, that seems to be working now. Still working on properly saving the flag_counts. Trying to find examples in core, especially for what to do in the destination's plugin import() method.

brunodbo’s picture

Not sure how to best save flag counts in the destination plugin's import() method. Is this a candidate for #2769979: Add migrate destination for sql table?

brunodbo’s picture

Status: Active » Needs review

Setting this to 'Needs review' to see if anyone has ideas on how to migrate the flag_counts table.

The last submitted patch, 8: 2409901_flag_migration_paths_8.patch, failed testing.

The last submitted patch, 6: 2409901_flag_migration_paths_6.patch, failed testing.

The last submitted patch, 6: 2409901_flag_migration_paths_6.patch, failed testing.

The last submitted patch, 8: 2409901_flag_migration_paths_8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2409901_flag_migration_paths_16.patch, failed testing.

joachim’s picture

> Not sure how to best save flag counts in the destination plugin's import() method. Is this a candidate for #2769979: Add migrate destination for table?

Indeed, that does sound like a candidate for that.

Alternatively, we could have our destination plugin for the Flagging entity update the counts table each time the flagging is saved.

schneidolf’s picture

StatusFileSize
new6.08 KB

In my opinion there is no need to migrate the flag counts. It's just calculated data and will recalculate on flagging migration. I cleaned up the patch from #16.

schneidolf’s picture

Status: Needs work » Needs review
schneidolf’s picture

StatusFileSize
new6.18 KB

Removed coding standard errors from #25.

socketwench’s picture

Status: Needs review » Needs work
  1. --- /dev/null
    +++ b/migrations/d7_flag.yml
    

    Move this to migration_templates/flag/d7. That's more consistent with core.

  2. +++ b/migrations/d7_flag.yml
    @@ -0,0 +1,33 @@
    +  bundles: bundles
    

    I'm a bit concerned here. It'd be a good place for a static_map to change the target bundle during the migration. Would that still work here?

  3. +++ b/migrations/d7_flag.yml
    @@ -0,0 +1,33 @@
    +  weight: 'options/weight'
    +  flag_short: 'options/flag_short'
    +  flag_long: 'options/flag_long'
    +  flag_message: 'options/flag_message'
    +  unflag_short: 'options/unflag_short'
    +  unflag_long: 'options/unflag_long'
    +  unflag_message: 'options/unflag_message'
    +  unflag_denied_text: 'options/unflag_denied_text'
    

    Can we use an Iterator here instead? The / syntax is a bit frowned upon now. Alternatively, just expose all the options fields as fields in their own right. That way, no /, no Iterator. Wordier, but easier to manipulate for the migrator.

  4. +++ b/migrations/d7_flag.yml
    @@ -0,0 +1,33 @@
    diff --git a/migrations/d7_flagging.yml b/migrations/d7_flagging.yml
    

    Same as with Flag. Let's move this to migration_templates/flagging/d7.

  5. +++ b/migrations/d7_flagging.yml
    @@ -0,0 +1,19 @@
    \ No newline at end of file
    

    [nit]Missing newline.[/nit]

socketwench’s picture

Title: Implement Migration Paths for Flag 7.x, 6.x » Implement Migration Paths for Flag 7.x
Related issues: +#2924209: Implement Migration Paths for Flag 6.x

Let's also split off 6.x into it's own issue. #2924209: Implement Migration Paths for Flag 6.x

socketwench’s picture

Component: Flag core » Migration
schneidolf’s picture

Component: Migration » Flag core
Status: Needs work » Needs review
StatusFileSize
new6.15 KB

For 1. and 4. :
If you look for examples from other contributed modules, you may notice that the migrations are placed in the MODULE/migration_templates directory. This directory still works for backwards compatibility but the correct place since Drupal 8.1 is the MODULE/migrations directory.
Source: https://www.drupal.org/docs/8/api/migrate-api/writing-migrations-for-con...

For 2.
It's not possible to do this with a static map. We don't know the bundle names of the source system.

For 3.
There is no need for an iterator. Syntax is given by the Migrations API. Save code here and leave it as it is.

For 5.
Patched.

schneidolf’s picture

Component: Flag core » Migration
tr’s picture

Just to confirm what @schneidolf said in #31: Since Drupal 8.1, the correct place for the migration configuration is in MODULE/migrations.

And now, 18 months after the fact, core migrations have been moved to the proper place. Here is the change record: Renamed migrations_templates directory to migrations

Note that MODULE/migration_templates is now @deprecated, so with the recent changes to the testbot, tests will start failing if you have anything in the old MODULE/migration_templates directory.

socketwench’s picture

Issue tags: +Needs tests

It's not possible to do this with a static map. We don't know the bundle names of the source system.

You would in a custom migration, and that's my point. There'd probably be a way to make that work even if there was an array there anyways.

Note that MODULE/migration_templates is now @deprecated, so with the recent changes to the testbot, tests will start failing if you have anything in the old MODULE/migration_templates directory.

Yep, you're right. I've obviously had my nose buried in a 8.4 migration for too long...

What about tests?

tr’s picture

@socketwench: Yes, definitely need migration tests here. I'm willing to write the tests. But can you create D6 and D7 database fixtures with Flag data to use for the tests? That would give me something to use - and since reviewing a fixture that someone else made is a huge job I'm guessing you'd feel more comfortable creating your own anyway ...

socketwench’s picture

Let me see if I can find the last backup of my D7 site... There might be a few flags on there.

socketwench’s picture

Will that work?

Bleh. D.O isn't letting me upload a *.sql file...

socketwench’s picture

StatusFileSize
new6.76 KB
tr’s picture

Awesome, thanks! I'll put together a migrate test using that fixture and post it here. It may take me a day or two ...

tr’s picture

Assigned: Unassigned » tr

Assigning it to myself.

I now have working tests, but some changes to the patch in #31 need to be made since there have been changes to Flag and Migrate over the past year or so that impact the code.

The table dumps from #38 are a good start, but don't include a lot of data (only one flag, for example) and don't include {flag_types}. What I did was use the core drupal7.php fixture then import these flag table dumps and write the tests against them. What I plan to do is add more flag data so I can improve the test coverage.

I'll post a patch in a few days with the update to #31, the new migration fixture, and the new migration tests. Then we can talk about what is still left to be done.

brunodbo’s picture

StatusFileSize
new6.6 KB

Extended patch in #31 to add support for Field API fields in the flagging source plugin (based on how Node module does it).

For now, this leaves it up to the migrator to map the fields in the flagging migration. We could add mappings like Node module does (when you run drush migrate-upgrade --configure-only, though in the case of Node, the bundles are split up in separate migrations, which might be overkill for Flag.

socketwench’s picture

Status: Needs review » Needs work

So far so good! I've tested it locally and it's working fine.

I would like to figure out if there's a way to do tests before committing it though.

socketwench’s picture

Status: Needs work » Needs review
StatusFileSize
new6.8 KB
new506 bytes

Added the ability to select a flag_type in the flagging source plugin. This makes it easier to create custom migrations by flag type with different field mappings.

tr’s picture

Status: Needs review » Needs work

Actually no, #41 is not good because it doesn't fix some of the things that are wrong with #31. And there's no interdiff, so it's harder to tell what was added. I think the only thing added/changed by #41 was the addition of Flagging::prepareRow(). Correct me if I'm wrong ...

As I mentioned in #41, I found and fixed some problems in #31 via the process of writing the tests - there have been changes to both Flag and Migrate since the original patch was written, and it needed to be updated to work properly.

I assigned this to myself because I have already done a lot of work to fix the migration templates and write the tests. The tests work, but IMO they are not sufficient because the existing database fixture based on the table dumps from #38 is insufficient to provide testing of all the migration. As a point of procedure, I would appreciate it if others would refrain from working on this while it is assigned to me, or at a minimum coordinate with me, because it annoys me when I put in a lot of work that potentially gets ignored/goes to waste when my efforts get bypassed.

The only thing missing right now is a more complete database fixture so the tests can cover everything that needs to be tested. I have been busy with other things recently, but I will return to this in the next week or two. If someone else can provide the data that would be great, but if not I need a free day to create that before I can finish up the tests.

I can post my intermediate work with my current minimal database fixture based on the table dumps in #38, but I'd rather wait until I get a good fixture - that way it only has to be reviewed once, which will make it easier on everyone.

I can add the Flagging::prepareRow() from #41 to what I already have, but that will mean the database fixture now has to declare fields on some of the flaggings, and the tests need to check that those fields were migrated properly. That will take additional work.

Christopher Riley’s picture

It has been a while since there was any movement on getting this added. Has the issue gone dead or is it still being looked into?

brunodbo’s picture

@Christopher Riley I used the patch in #43 to migrate flags/flaggings from D7 to D8 (August 2018), and experienced no issues doing so.

Migrate support is still something that should be added to the Flag module, so this issue is still alive :). My guess is that people just haven't had the time to work on it further.

demon326’s picture

@brunodbo the patch does not work anymore.. The migrate API changed since then and the php files need some new information regarding "@MigrateSource(...)". When adding that information, the migrate module pulls the data from the old site, but does not insert it into the flag tables. Flag migrate tables do have data.

Christopher Riley’s picture

In Flag.php and Flagging.php, source_module = "flag" needs to be added instead of source_provider = "flag". Once this is changed it should work just fine and dandy but it would be nice for this to get officially addressed.

joelpittet’s picture

@Christopher Riley could you roll that into the latest patch?

Christopher Riley’s picture

If I ever get something working I will post a patch right now fighting with a:

Error: Call to a member function id() on null in /home/sites/sitename/public_html/modules/contrib/flag/src/FlagCountManager.php on line 202 #0 [internal function]: Drupal\flag\FlagCountManager->incrementFlagCounts(Object(Drupal\flag\Event\FlaggingEvent), 'flag.entity_fla...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))

demon326’s picture

@Christopher Riley i dont know if its an issue due working with migrate UI, but it does not transfer the flags over to D8. Should i first create them in D8, then start migrate? Patched against Alpha 3.

dieuwe’s picture

StatusFileSize
new6.8 KB

This is just a re-roll of the patch in 43 with "source_provider" changed to "source_module" so that it doesn't fail validation. I haven't addressed any of the other issues raised since comment 43 since I haven't run into them yet.

EDIT: Can confirm the error from 50, I'll see if I can get another updated patch tomorrow.

demon326’s picture

@dieuwe can comfirm the error to... would be great if you found a way to fix it... Flag migrate is the only path that i still need and i would love it if atleast that would work.

johan.gant’s picture

I can confirm that the patch at #52 applies cleanly on 8.x-4.0-alpha3 on core 8.7.1. Once set up on a custom migration I was able to successfully extract D7 flags config and import to a clean D8 site. NB: this does not import the contents of the 'flagging' table - the data/content - that needs a separate approach.

dieuwe’s picture

I've managed to figure out that the error occurs when you attempt to migrate a flag for a node that does not exist:

Error: Call to a member function id() on null in /home/sites/sitename/public_html/modules/contrib/flag/src/FlagCountManager.php on line 202

What you need to do is alter the entity_id property from the example to skip on empty (and you probably want to run a migration_lookup against that in 99% of cases, here is the example:

process:
  ...
  entity_id:
    -
      plugin: migration_lookup
      source: entity_id
      migration: { }
    -
      plugin: skip_on_empty
      method: row
  ...

You'll need to reference your node migration(s), left blank above.

This is because as part creating the flagging, the node it references is loaded. It is not necessary to skip on the uid, also that might be a good idea to make sure you don't bring across any hanging flaggings.

I am not providing a patch at this stage, but might recommend that the example YAML files are altered with the migration_lookup and skip_on_empty included by default so that this is properly documented.

Once that documentation change has been made I would say this is ready for final review.

dieuwe’s picture

Assigned: tr » Unassigned
Status: Needs work » Needs review
StatusFileSize
new7.21 KB

Decided to add a patch anyway so that it can be reviewed. All this does is change the sample migration/d7_flagging.yml to avoid errors on missing nodes by making it pretty clear that nodes need to exist before flags can be migrated.

demon326’s picture

Strange: The patch worked on a older 8.7 test site without editing the yml files. When i test it with a new migration, it gives the error.. same content and migrate strategie used.

demon326’s picture

New update:

Tested patch using d7_node and d7_user, the migrate starts working and no fatal errors appear. BUT, when finishedn it just says ignored < number of flags >.

The question i got is, what is the correct syntax in the yml file to use:

is it;

.... 
 plugin: migration_lookup
      source: entity_id
      migration: { d7_node } # Your content migration(s).
.....

or:

.... 
 plugin: migration_lookup
      source: entity_id
      migration:  d7_node  # Your content migration(s).
.....

or:

.... 
 plugin: migration_lookup
      source: entity_id
      migration: # Your content migration(s).
          - d7_node
.....
dieuwe’s picture

Yes, so if you are doing an in-place upgrade and preserving all IDs on your Drupal site, you want the YML files from #52 and previous.

The YML files for #56 intentionally don't work out of the box, you need to edit them based on your other content and user migrations.

So you'd be looking at a syntax like the following if you have only a single content migration from which flagging could possibly be coming from:

 plugin: migration_lookup
      source: entity_id
      migration:  my_article_content_migration

or if you have multiple content migrations then you would need the following:

  plugin: migration_lookup
      source: entity_id
      migration:
          - my article_content_migration
          - my page_content_migration
          - my event_content_migration

The same goes for the UID code block as per the EID one. (And if you have other flaggable entity types then you obviously need lookups against the migrations for those other too, not just node ones.)

This is why perhaps my patch from #56 is not the perfect solution as it attempts to provide some extra documentation about how you should be handling the migration in most circumstances, but perhaps it should simply keep the simplified syntax and add a single comment with a warning that that will only work in limited circumstances and provide a link to more detail.

Regardless of the approach taken, it's clearly enough of a problem that better documentation will be required.

marvil07’s picture

Thanks for the patches in this issue!
It seems only the last one would be the relevant for now, so I am reflecting that on the file metadata.

I have only tested the new migrate source plugins and they are working great.

In the patch attached here, I am making a small change that allows passing both one flag type or multiple flags as an array from the migration yaml file.

@socketwench, you may want to add the migrate source plugins separately first, so some migration support get in, until the final migration yaml files are ready.

pipicom’s picture

#52 works for me. Thank you!

iancawthorne’s picture

I have tried the patches at #52 and #60. In both cases I get the flag types, but no flaggings migrated. It says they are processed, but "ignored" when running "drush migrate:import upgrade_d7_flagging". Users and relevant nodes have already been migrated.

#52. I see the flaggings in the migrate table of the db. #60 they don't get that far. Neither get into the flaggings table.

Is this expected? Is there any documentation on manual steps to modify migration yml files to add missing bits specific to a site? Apologies, I tried to follow the post at #59 and got quite confused on what should be done, if anything.

pipicom’s picture

@iancawthorne

You have to first migrate users, nodes and relevant fields and then first import d7_flag & aftewards d7_flagging.
In my case patch #52 was what i needed - applied the patch without any modification.

Hope this helps.

iancawthorne’s picture

Thanks @pipicom, but I can confirm I have tried various order combinations of running the imports, but always made sure users and nodes are imported before the flag and then the flaggings are run. If you are successful in migrating the flaggings, I must be missing something, but I'm not sure what.

demon326’s picture

@ian:
I have used the following order succesfully:

#user 

- text_settings
- d7_user
- d7_user_flood
- d7_user_mail
- d7_user_role

#taxonomy 

- d7_taxonomy_term
- d7_taxonomy_vocabulary
- taxonomy_settings

#nodes 
- d7_filter_format
- d7_node_type
- d7_field
- d7_field_formatter_settings
- d7_field_instance
- d7_field_instance_widget_settings
- d7_node
- d7_node_settings
- d7_node_title_label
- d7_flag
- d7_flagging
- d7_filter_settings
- d7_image_settings
- d7_image_styles
- user_picture_entity_display
- user_picture_entity_form_display
- user_picture_field
- user_picture_field_instance
- d7_file_settings

You could try this version of the module, its an old alpha with the migrate patch attached to it:
https://we.tl/t-iOcaHqrdZ7

If the migrate should succeed, you should overwrite it with new files.

iancawthorne’s picture

Thanks @demon326. I've tried your alpha version of the module and it gives the same result.

When I migrate:import d7_flagging the migrate_map_upgrade_d7_flagging table in the db is populated, but the flagging table is not.
The migrate_import when finished reports:
[notice] Processed 44 items (0 created, 0 updated, 0 failed, 44 ignored) - done with 'upgrade_d7_flagging'.
In that table the "destid1" column has NULL for every row. I am wondering if this is the problem.
migrate:rollback then clears the migrate_map_upgrade_d7_flagging table

I'm fairly sure I've done all of the migration:imports (plus a lot more) you have listed above before running the flag migrations.

I noticed you have listed "d7_node". I don't have that as a migration option on its own, but I do have ones for each content type to migrate, eg "d7_node:page" and "d7_node:article". I'm guessing that is what you mean by d7_node?

demon326’s picture

@iancawthorne strange that it won't work in your case.

I did the whole migration process using the migrate_manifest module, as that makes doing the migration over and over again way much easier and that worked many times over to migrate the (flag) content. That is why you see d7_node and not the subtypes, as it takes them all in one go.

dkmishra’s picture

Thanks @marvil07, I have applied the #60 patch with version 8.x-4.0-beta1. It was showing NIL, NIL then I have update yml as below

In d7_flag.yml

id: d7_flag
label: Flag configuration
migration_group: migrate_drupal_7 # Added this

In d7_flagging.yml

id: d7_flagging
label: Flagging
migration_group: migrate_drupal_7 # Added this

Also I have updated flag module to latest version in Drupal 7 setup. After doing this, all the flags have migrated successfully. Thank you.

fredonia_webteam’s picture

We are having issues with the upgrading of d7_flagging using #60 patch, same issues as @iancawthorne (Comment#66). Our migrate_map_upgrade_d7_flagging table shows NULL for all destid1.

The result from migrate:import is:
Processed 3124 items (0 created, 0 updated, 0 failed, 3124 ignored) - done with 'upgrade_d7_flagging'

We ran the upgrade_d7_flagging import last after all other imports. We are on Drupal 8.8.4 using Flag 8.x-4.0-beta1.

@Devendra Mishra did you do anything else to get the flagging migration to work? What version of Drupal were you on when you migrated?

jas1988’s picture

We have already migrated our nodes etc from 7 to 8 and migration mappings are already present in site which were created using :

drush migrate-upgrade --legacy-db-url=mysql://root:passport@localhost/drupal7 --legacy-root=/Sites/drupa8 --configure-only

Now I have tried different patches from above and tried editing files as suggested above for patch 60 etc

Can anyone please suggest how exactly these migrations will be listed ie: how mapping table for flags will be crated, I tried running drush migrate-upgrade again after clearing cache but it does not create any migration which we can import using drush migrate-import d7_flag etc ..


EDIT:
When tried exporting configurations such as below :

drush8 config-import --partial --source=/Users/Sites/drupal8/migrations

In migration folder there are d7_flag.yml and d7_flagging.yml files it give below error:

Import the listed configuration changes? (y/n): y
Drupal\Core\Config\ConfigImporterException: There were errors validating the config synchronization. [error]

The config name d7_flag is invalid.
The config name d7_flagging is invalid.

Configuration d7_flag depends on the d7_flag extension that will not be installed after import.
Configuration d7_flagging depends on the d7_flagging extension that will not be installed after import. in

iancawthorne’s picture

@pipicom Over a year since my last looking at this issue I know, but I've just tried it again using patch #52 and it looks to be working. I think I made the mistake of applying the patch after I'd already run the drush migrate-upgrade --configure-only command, so the mapping was based on the patch at #60.

I do have another question for this thread, but it may warrant it's own ticket....

The flags I'm migrating have fields on them. I can see these fields failing to migrate during drush migrate:import upgrade_d7_field_instance. It reports the error: Attempt to create a field field_name that does not exist on entity type flagging. Has anyone done migration of fields (and their data) on flags? Should it work, or has it not been included? Is it a flag migration thing or is it a core field thing or a bit of both?

pipicom’s picture

@iancawthorne happy that it worked for you even after a year :)
But my flags didn't have any extra fields so I wouldn't know the answer to your question. Perhaps you should create a field with the same name on your new flags and then try to import the old fields. I mean, you cannot import fields if they dont exist already on your new website.

berenddeboer’s picture

@jas1988, don't do config import but do drush migrate:update --configure-only, then it showed up for me.

berenddeboer’s picture

I've noticed a big problem with this update: it imports entity node instead of comment. I have a customer who has both node and comment flags. Comment flags are imported with the "node" entity_type.

berenddeboer’s picture

False alarm, must have some bad data in the table already. Once I add the comment id to the entity_id migration lookup, it works.

However this seems scary: what if a node and a comment share the same id, the migration_lookup will simply find the first one. This lookup should look at entity_type as well.

dolcaer’s picture

This issue seems rather inactive, but I just performed a migration from Drupal 7 which seems to have been successful. I'm starting the migrated site as a clean Drupal 9 site, migrating all content to this site, with mostly the same modules. It's an install using Composer to manage dependencies.

What I did to get this working on flag version 4.0.0-beta3:

- require drupal/flag using composer, enabling the module using drush
- including the patch of #52 using cweagans/composer-patches
- composer reinstall drupal/flag to make sure it picks up this patch
- clean install of the site (drush site:install, drush config:import)
- prepare the migrations (drush migrate:upgrade --configure-only)
- perform the migrations flag seems to rely on (drush migrate:import <migration-id> for user, my flagged node type)
- perform the flag migrations (drush migrate:import <migration-id> for flag, flagging)

I have also tried using patch #60, but this resulted in all flags being ignored. I have not been able to find out what caused this.

mradcliffe made their first commit to this issue’s fork.

grevil’s picture

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 separate issue for solving this problem here: #3336839: FlagCountManager: Call to a member function id() on null.

Just commenting here, for visibility’s sake.

EDIT: Just saw patch 56 which apparently solves this issue, using the skip on empty process plugin. The empty check still makes sense, so I won't close the separate issue.

grevil’s picture

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

mradcliffe’s picture

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

grevil’s picture

StatusFileSize
new774 bytes

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.

alex.bukach’s picture

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
d_lav’s picture

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 to fix the Error: Call to a member function id() on null issue.

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 published nodes for a matching entity id.

grevil’s picture

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 important "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.

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.

EDIT: Patch 60 introduces the possibility to restrict migrating flags to specific entity types only, before only one entity type could be used for this restriction. This was unclear at first as "flagging" = flag type and "flag_type" = entity type to flag.

grevil’s picture

Status: Needs review » Needs work
grevil’s picture

Concerning comment #83, I don't think, we should ship the migration with this mapping, since profile2 is a contrib module. 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.

mradcliffe’s picture

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.

grevil’s picture

@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
grevil’s picture

We also need to adjust the tests, after specifying and implementing the deriver.

solideogloria’s picture

I successfully migrated using #52 as-is.

francoud’s picture

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 flaggings 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)

ivnish made their first commit to this issue’s fork.

ivnish’s picture

I ran pipeline. Tests needs some updates

ivnish’s picture

ivnish’s picture

Assigned: Unassigned » ivnish

  • ivnish committed 66a50582 on 8.x-4.x
    Issue #2409901 by mradcliffe, brunodbo, grevil, ivnish, schneidolf,...

ivnish’s picture

Assigned: ivnish » Unassigned
Status: Needs work » Fixed
Issue tags: -Needs tests
Related issues: -#2924209: Implement Migration Paths for Flag 6.x
ivnish’s picture

Thanks all! Committed!

joelpittet’s picture

Oh wow, thanks @ivnish for wrapping this up!

mradcliffe’s picture

A follow-up would be to make a deriver class because as-is this will have some problems without manual YAML modification (migration_lookup, etc...) if using migrate_upgrade / migrate_plus entities.

Status: Fixed » Closed (fixed)

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