#205 | metatag-n2563649-205.patch | 54.45 KB | DamienMcKenna |
|
#205 | metatag-n2563649-205.interdiff.txt | 3.83 KB | DamienMcKenna |
#204 | metatag-n2563649-204.patch | 54.32 KB | DamienMcKenna |
|
#204 | metatag-n2563649-204.interdiff.txt | 6.64 KB | DamienMcKenna |
#202 | metatag-n2563649-202.patch | 53.55 KB | DamienMcKenna |
|
#200 | metatag-n2563649-200.interdiff.txt | 726 bytes | DamienMcKenna |
#200 | metatag-n2563649-200.patch | 50.67 KB | DamienMcKenna |
|
#199 | metatag-n2563649-199.patch | 50.69 KB | DamienMcKenna |
|
#199 | metatag-n2563649-199.interdiff.txt | 785 bytes | DamienMcKenna |
#197 | metatag-n2563649-197.patch | 50.63 KB | DamienMcKenna |
|
#197 | metatag-n2563649-197.interdiff.txt | 39.9 KB | DamienMcKenna |
#196 | metatag-n2563649-196.patch | 50.51 KB | DamienMcKenna |
|
#196 | metatag-n2563649-196.interdiff.txt | 3.36 KB | DamienMcKenna |
#191 | metatag-n2563649-191.interdiff.txt | 3.58 KB | DamienMcKenna |
#191 | metatag-n2563649-191.patch | 49.23 KB | DamienMcKenna |
|
#190 | metatag-n2563649-190.patch | 49.24 KB | DamienMcKenna |
|
#190 | metatag-n2563649-190.interdiff.txt | 6.2 KB | DamienMcKenna |
#188 | metatag-n2563649-188.interdiff.txt | 3.86 KB | DamienMcKenna |
#188 | metatag-n2563649-188.patch | 47.81 KB | DamienMcKenna |
|
#178 | metatag-n2563649-178.interdiff.txt | 1.45 KB | DamienMcKenna |
#178 | metatag-n2563649-178.patch | 46.34 KB | DamienMcKenna |
|
#176 | metatag-n2563649-176.patch | 46.08 KB | DamienMcKenna |
|
#176 | metatag-n2563649-176.interdiff.txt | 521 bytes | DamienMcKenna |
#174 | metatag-n2563649-174.patch | 46.08 KB | DamienMcKenna |
|
#174 | metatag-n2563649-174.interdiff.txt | 390 bytes | DamienMcKenna |
#172 | metatag-n2563649-172.patch | 46.05 KB | DamienMcKenna |
|
#172 | metatag-n2563649-172.interdiff.txt | 36.96 KB | DamienMcKenna |
#167 | metatag-n2563649-166.interdiff.txt | 1.78 KB | DamienMcKenna |
#167 | metatag-n2563649-166.patch | 45.49 KB | DamienMcKenna |
|
#161 | metatag-n2563649-161.interdiff.txt | 2.44 KB | DamienMcKenna |
#161 | metatag-n2563649-161.patch | 45.46 KB | DamienMcKenna |
|
#158 | metatag-n2563649-158.interdiff.txt | 1.99 KB | DamienMcKenna |
#158 | metatag-n2563649-158.patch | 45.22 KB | DamienMcKenna |
|
#156 | metatag-n2563649-156.patch | 45.22 KB | DamienMcKenna |
|
#156 | metatag-n2563649-156.interdiff.txt | 756 bytes | DamienMcKenna |
#154 | metatag-n2563649-154.interdiff.txt | 28.28 KB | DamienMcKenna |
#154 | metatag-n2563649-154.patch | 45.22 KB | DamienMcKenna |
|
#151 | metatag-n2563649-151.patch | 39.57 KB | DamienMcKenna |
|
#151 | metatag-n2563649-151.interdiff.txt | 17.94 KB | DamienMcKenna |
#150 | metatag-n2563649-150.interdiff.txt | 832 bytes | oliverpolden |
#150 | metatag-n2563649-150.patch | 27.07 KB | oliverpolden |
|
#148 | interdiff_142-148.txt | 332 bytes | oliverpolden |
#148 | metatag-n2563649-148.patch | 26.94 KB | oliverpolden |
|
#142 | metatag-n2563649-142.interdiff.txt | 1.21 KB | DamienMcKenna |
#142 | metatag-n2563649-142.patch | 26.82 KB | DamienMcKenna |
|
#141 | metatag-n2563649-141.patch | 26.85 KB | DamienMcKenna |
|
#141 | metatag-n2563649-141.interdiff.txt | 10.9 KB | DamienMcKenna |
#140 | metatag-n2563649-140.patch | 26.93 KB | DamienMcKenna |
|
#140 | metatag-n2563649-140.interdiff.txt | 776 bytes | DamienMcKenna |
#139 | metatag-n2563649-139.patch | 26.93 KB | DamienMcKenna |
|
#139 | metatag-n2563649-139.interdiff.txt | 1.28 KB | DamienMcKenna |
#134 | metatag-n2563649-134.interdiff.txt | 498 bytes | DamienMcKenna |
#134 | metatag-n2563649-134.patch | 26.93 KB | DamienMcKenna |
|
#132 | metatag-n2563649-132.interdiff.txt | 494 bytes | DamienMcKenna |
#132 | metatag-n2563649-132.patch | 26.91 KB | DamienMcKenna |
|
#130 | metatag-n2563649-130.interdiff.txt | 753 bytes | DamienMcKenna |
#130 | metatag-n2563649-130.patch | 26.87 KB | DamienMcKenna |
|
#128 | metatag-n2563649-128.patch | 26.81 KB | DamienMcKenna |
|
#128 | metatag-n2563649-128.interdiff.txt | 2.86 KB | DamienMcKenna |
#123 | metatag-n2563649-123.interdiff.txt | 1.83 KB | marcelovani |
#123 | metatag-n2563649-123.patch | 24.29 KB | marcelovani |
|
#109 | metatag-n2563649-109.interdiff.txt | 1.34 KB | DamienMcKenna |
#109 | metatag-n2563649-109.patch | 22.89 KB | DamienMcKenna |
|
#107 | metatag-n2563649-107.interdiff.txt | 2.12 KB | DamienMcKenna |
#107 | metatag-n2563649-107.patch | 22.85 KB | DamienMcKenna |
|
#104 | metatag-n2563649-104.patch | 21.76 KB | DamienMcKenna |
|
#104 | metatag-n2563649-104.interdiff.txt | 12.87 KB | DamienMcKenna |
#95 | interdiff-2563649-93-migrate.txt | 4.41 KB | marvil07 |
#95 | 2563649-95-8.x-1.4.patch | 22.38 KB | marvil07 |
|
#95 | 2563649-95-8.x-1.x.patch | 21.88 KB | marvil07 |
|
#93 | interdiff.txt | 1.18 KB | marvil07 |
#93 | 2563649-93-migrate.patch | 20.9 KB | marvil07 |
|
#90 | interdiff_2563649_86-90.txt | 3.58 KB | socketwench |
#90 | metatag-n2563649-90.patch | 22.08 KB | socketwench |
|
#86 | metatag-n2563649-86.interdiff.txt | 1.18 KB | DamienMcKenna |
#86 | metatag-n2563649-86.patch | 22.11 KB | DamienMcKenna |
|
#82 | metatag-n2563649-82.interdiff.txt | 1.88 KB | DamienMcKenna |
#82 | metatag-n2563649-82.patch | 22.01 KB | DamienMcKenna |
|
#78 | interdiff-76-78.txt | 3.42 KB | WidgetsBurritos |
#78 | metatag-n2563649-78.patch | 23.58 KB | WidgetsBurritos |
|
#76 | metatag-n2563649-76.patch | 23.37 KB | WidgetsBurritos |
|
#72 | metatag-n2563649-70.patch | 22.17 KB | pobster |
|
#69 | metatag-n2563649-69.patch | 22.16 KB | pobster |
|
#66 | metatag-n2563649-66.patch | 21.94 KB | pobster |
|
#62 | metatag-n2563649-62.patch | 20.32 KB | DamienMcKenna |
|
#52 | migrations_basic-2563649-52.patch | 20.28 KB | jofitz |
|
#52 | interdiff.txt | 2.36 KB | jofitz |
#4 | migrations_basic-2563649-4.patch | 6.92 KB | jofitz |
|
#6 | migrations_basic-2563649-6.patch | 7.63 KB | jofitz |
|
#9 | migrations_basic-2563649-9.patch | 9.75 KB | jofitz |
|
#11 | migrations_basic-2563649-11.patch | 9.73 KB | jofitz |
|
#13 | migrations_basic-2563649-13.patch | 10.1 KB | jofitz |
|
#15 | migrations_basic-2563649-15.patch | 10.08 KB | jofitz |
|
#15 | interdiff.txt | 20.2 KB | jofitz |
#18 | migrations_basic-2563649-18.patch | 10.04 KB | jofitz |
|
#18 | interdiff.txt | 957 bytes | jofitz |
#19 | migrations_basic-2563649-19.patch | 15.41 KB | jofitz |
|
#19 | interdiff.txt | 11.98 KB | jofitz |
#24 | aaa.JPG | 87.19 KB | Benia |
#27 | interdiff.txt | 18.94 KB | jofitz |
#27 | migrations_basic-2563649-27.patch | 15.62 KB | jofitz |
|
#29 | migrations_basic-2563649-29.patch | 15.06 KB | jofitz |
|
#31 | migrations_basic-2563649-31.patch | 15.03 KB | jofitz |
|
#32 | interdiff.txt | 7.06 KB | jofitz |
#32 | migrations_basic-2563649-32.patch | 16.11 KB | jofitz |
|
#34 | migrations_basic-2563649-34.patch | 16.12 KB | jofitz |
|
#37 | interdiff.txt | 6.1 KB | jofitz |
#37 | migrations_basic-2563649-37.patch | 16.12 KB | jofitz |
|
#39 | interdiff.txt | 5.84 KB | jofitz |
#44 | migrations_basic-2563649-44-w_o_user_test.patch | 20.26 KB | jofitz |
|
#39 | migrations_basic-2563649-39.patch | 20.26 KB | jofitz |
|
#44 | interdiff.txt | 759 bytes | jofitz |
#44 | migrations_basic-2563649-44-complete.patch | 20.26 KB | jofitz |
|
Comments
Comment #2
jofitz CreditAttribution: jofitz at ComputerMinds commentedI'm in the process of writing the migration integration for D7.
Comment #3
DamienMcKennaThat would be amazing, thank you!
Comment #4
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded migration integration for D7.
3 migrations:
* d7_metatag_field
* d7_metatag_field_instance
* d7_metatag_field_instance_widget_settings
Metatag data is migrated as part of d7_node/d7_node_revision.
Comment #5
DamienMcKennaIs there any way to make this more generic so it can work with any entity instead of just nodes?
Comment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedAs requested, I have improved the patch to handle any entity rather than only nodes.
Comment #7
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThanks very much for the patch!
From my limited experience of Drupal 8 migration, this seems to be sensible in terms of splitting out the creation of the different pieces of the field and then the migration of the actual data itself.
I've done a visual review of the patch and found a few issues, detailed below.
Do we need this line and associated conditional?
Which migrations have an id that starts with 'd7_metatag_field' and have destinations that extend
EntityContentBase
? I had a quick look but couldn't see any.This code still looks to be node specific. For example, when migrating users, they would not have a
nid
property, but auid
property (I assume).The code might need to handle revisions slightly differently, if the entity type doesn't support revisions, then this will probably return
NULL
which will get fed into the DB query below.Same comments as 1 above.
This probably needs some documentation to explain what we're doing here, either on the class as a whole, or on the method.
We are getting the bundles of the target Drupal 8 site here, not the source Drupal 7 site, we should make that explicit with documentation here too I think.
As you're iterating over an iterator you should be able to simplify this code by using a
foreach
instead of thewhile
.Then you can ditch the
->rewind()
etc.Additionally I suppose this probably needs some tests? I assume that Drupal core has tests for migrations, but have no idea how they work, sorry! I suppose it's up to the maintainer as to whether the lack of tests stops this patch from going in or not.
Comment #8
DamienMcKennaThanks for the review, Steven, I appreciate it.
Comment #9
jofitz CreditAttribution: jofitz at ComputerMinds commentedMade changes suggested in code review.
Made updates for 8.1.1 (i.e. migrations are now plugins rather than entities).
Comment #11
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll patch.
Comment #13
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdd missing module dependency (migrate) to MetatagNodeTranslationTest.
Comment #15
jofitz CreditAttribution: jofitz at ComputerMinds commentedThe migration(s) requires an additional dependency on the "migrate" module. The migration code is now in a separate metatag_migrate sub-module because it would not make sense for the "metatag" module to have a dependency on the "migrate" module.
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #17
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commentedThanks for the updated patch. Thanks for making lots of changes and working to get the tests passing again :)
I've had another look at the code, and here are some minor points that could just do with a quick tidy up I think. Also, needs tests :) So setting back to 'Needs work' because of that.
Very minor nit pick: You don't need this extra blank line here.
Minor nit pick: I think it would be as clear to just use
$source->getDatabase()->select
rather than introduce a variable$db
that only gets used once.Migrate only cares if you
return FALSE
from the prepare hooks, so you could probably just ditch the return statement here.Comment #18
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll in response to code review.
Comment #19
jofitz CreditAttribution: jofitz at ComputerMinds commented* Added functional tests for metatag migration.
* Transferred migration tweaks from Subscriber to hook_migration_plugins_alter() because the change to migration dependencies must happen before PreImport.
And a few minor changes to the migration templates:
* obtain entity type from source rather than hard-coded in d7_metatag_field_instance_widget_settings migration.
* set all metatag migration to ignore_map because joining to the map tables creates errors.
* added taxonomy vocabulary as a migration dependency of metatag field instance.
Comment #20
mikeryanThere is no need for a separate submodule (and that makes things harder for upgraders, needing to temporarily enable an extra module). The main module does not need to have a dependency on migrate - the migration-relevant plugins and classes will only come into play when migrate/migrate_drupal are enabled.
Comment #21
DamienMcKennaThanks for the info, mikeryan.
Putting this back to "needs work" so that the changes can be merged back into the primary module instead of being a submodule.
Comment #22
Benia CreditAttribution: Benia commentedDeleted.
Comment #23
Benia CreditAttribution: Benia commentedDeleted.
Comment #24
Benia CreditAttribution: Benia commentedI putted the patch file in my Metatag module directory in the 8.1.3 Drupal test site, navigated with terminal and did git apply patch-file-name.
I then did flushed all caches, and clicked "review upgrade";
Even though, the "Metatag" migration path was still marked as "Missing".
Comment #25
Steven Jones CreditAttribution: Steven Jones at ComputerMinds commented@Benia with the current patch in #19 you'll need to enable the metatag_migrate sub module to get the migration code loaded.
Comment #26
Benia CreditAttribution: Benia commentedThat's so amazing, it worked ! (in the Migrate UI, the Metatag migration path still appeared as "Missing" but I had the feeling I should try anyway and after the upgrade).
when I checked the nodes --- All I checked appeared with Metadata.
I am so happy from this, thank all of you for this contribution !
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedTransferred code back out of metatag_migrate sub-module.
Comment #29
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected the patch.
Comment #31
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #32
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdd Unit test. Move Kernel test.
Comment #34
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #37
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #39
jofitz CreditAttribution: jofitz at ComputerMinds commentedUpload patch that should have been in #37.
Comment #41
jofitz CreditAttribution: jofitz at ComputerMinds commentedUnable to replicate the failure in local tests.
Lacking information in the error message:
fail: [Other] Line 69 of modules/metatag/tests/src/Kernel/Migrate/d7/MigrateMetatagTest.php:
Comment #42
DamienMcKennaJo: Are you running the latest 8.2.x code locally?
Comment #43
jofitz CreditAttribution: jofitz at ComputerMinds commentedI am not. I've been testing this ticket against 8.1.1. I'll give that a go, thanks.
Comment #44
jofitz CreditAttribution: jofitz at ComputerMinds commentedI've now managed to replicate the test failure locally (and find the cause) - this test will continue to fail until #2671312: No default value for User langcode when migrating D7 users with no language is resolved (or if the test user language is set).
I have uploaded a version of the patch with the failing line commented out to show that the test is close to passing.
Comment #46
jofitz CreditAttribution: jofitz at ComputerMinds commentedPostponing the ticket until #2671312: No default value for User langcode when migrating D7 users with no language is resolved.
Comment #47
Benia CreditAttribution: Benia commentedShould I use the current version of the patch to move only the basic Metadata (i.e Title, description, Abstract & Keywords) from my Drupal 7.44 site to my Drupal 8.1.3 Site ? Or it's better for me to use the version from comment #19 that seemed to work just fine for me in a test site ?
Comment #48
jofitz CreditAttribution: jofitz at ComputerMinds commented@Benia use the latest version (#44) - it has the same functionality as #19, but no longer requires you to enable a sub-module ("Metatag migrate") it is now all within Metatag.
Comment #49
csedax90 CreditAttribution: csedax90 commentedIt's possibile to use this patch with a custom migration and not only with a D7 -> D8 migration?
Comment #50
DamienMcKennaCan metatag_migrate_prepare_row() be moved into a separate file, as was possible with D7?
Comment #51
jofitz CreditAttribution: jofitz at ComputerMinds commentedNow awaiting resolution of #2751223: D6 & D7 users are migrated into D8 with incorrect langcode, should be able to get this finished soon.
Comment #52
jofitz CreditAttribution: jofitz at ComputerMinds commentedShould work since the release of #2751223: D6 & D7 users are migrated into D8 with incorrect langcode.
Minor tweaks to testing nids to avoid interference from core test fixture.
Comment #53
jofitz CreditAttribution: jofitz at ComputerMinds commented@DamienMcKenna RE Your question in #50 - why would you want
metatag_migrate_prepare_row()
in a separate file?Comment #54
DamienMcKenna@Jo: Thanks. Ideally anything that could be moved into a separate file should be, presuming you don't have to do any include_once() nonsense.
Comment #55
jofitz CreditAttribution: jofitz at ComputerMinds commented@Damien No, I don't think
metatag_migrate_prepare_row()
can be moved into a separate file because there is nomigrate_hook_info()
.Comment #56
DamienMcKennaAh, ok, in that case it's fine.
Thanks for your work on this, I'll try to test it soon.
Comment #57
tedfordgif CreditAttribution: tedfordgif at WebFirst, Inc. commentedFor those that are doing custom migrations (not from D7) to D8, you can just serialize an array to store the meta_tag data, like this:
Comment #58
DamienMcKennaThe 1.0 release is being bumped up, so the everything else is being put on the back burner for now.
Comment #59
DamienMcKennaPostponing until after 1.0.
Comment #60
DamienMcKenna8.x-1.0 is out, so this is fair game again.
Comment #61
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and at Appnovation for Appnovation commentedHi everyone, thank you for this initiative.
I would like to help but I cannot apply the git patch #52 whether with git or composer patches.
It just says "Skip patch".
Would you kindly tell me if it's my bad or if the patch is just too old to be working?
Comment #62
DamienMcKennaRerolled.
Comment #64
mglamanLooks like this needs to also add menu_ui
Comment #65
cruno CreditAttribution: cruno commentedI haven't tried anything posted in this thread yet, but I did manage to make a simplified method to grab metatag data during a node migration.
In MyNodeMigration.php's prepareRow() implementation:
In NodeMigrations.php, which MyNodeMigration.php extends:
I didn't test if every single key is the same from the D7 to D8 version, but I have seen it grabs the basics just fine (title, description, keywords). What it does not do is take revisions into account.
Comment #66
pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commentedRe-rolled (as the first hunk fails to apply with latest dev) and added the menu_ui dependency.
Comment #67
pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commentedComment #69
pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commentedMaybe I'm just reading this wrong, but I can't really see how this ever worked? Nothing actually wrote the term to the database? But ... there was a time when it was passing, so ... confused?
Comment #70
pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commentedComment #72
pobster CreditAttribution: pobster at ArcadeGeek LTD for Rackspace commentedMeh ... Tired ...
Comment #74
Pobtastic CreditAttribution: Pobtastic at ArcadeGeek LTD commented...then fine, it's coming from somewhere else, and it's likely that the defaults aren't working for terms in the same way that they weren't working for users before. I'll dig into this further when I have more time, at the moment I'm just puzzled as to why a lot of my module tests fail locally (this migration test won't even run), hence I'm pushing code in the hope that it works!
Meh, never again...
Comment #75
pcranston CreditAttribution: pcranston commentedI did this in a way pretty similar to @cruno described in #65, but re this comment:
we're pretty heavy users of the OpenGraph and Twitter card tags, so the name of the array key just needs to be updated for these, as the syntax is different. When looping over the array rows and bumping the value up a level, I updated the $key values accordingly:
Comment #76
WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commentedThis is simply a reroll of #70 against the latest commit.
Comment #78
WidgetsBurritos CreditAttribution: WidgetsBurritos at Rackspace commentedThis patch should resolve the test failures.
Comment #79
a.milkovskyIn addition to #65. I am processing drupal 7 metatags like this:
Comment #80
DamienMcKennaWith 8.6.x this throws the following error:
Incidentally, the patch for ECK (#2815453: Add migrations for Drupal 7 ECK to Drupal 8/9) gives the same error, so it must be a recent API change.Comment #81
DamienMcKennaFYI even 8.5.x throws this message:
Comment #82
DamienMcKennaDoes this help? It seems to at least get past that one blocker when I try it locally.
Comment #83
DamienMcKennaFYI that helps with both 8.5.x and 8.6.x to at least be able to get to the upgrade review page.
Comment #85
DamienMcKenna.. though, with the patch the 8.5.x upgrader shows that Metatag will be included in the list of upgraded modules, but 8.6.x shows it will not be upgraded.
This upgrade system is a wee bit complicated.
Comment #86
DamienMcKennaMight this help?
Comment #88
benjifisherI think the last line should have
source_module
rather thansource.module
. Or you could leave it out since that line is only needed if you want to override the value specified in the plugin annotation.Same comment for
migration_templates/d7_metatag_field_instance_widget_settings.yml
.See the change record: Use 'source_module' and 'destination_module' annotation to indicate module responsible for migration.
Also, the directory
migration_templates
has been deprecated. The CR is Renamed migrations_templates directory to migrations.Comment #89
heddnLinking meta #2906878: [Meta] Support for D7 -> D9 contrib migrate
Comment #90
socketwench CreditAttribution: socketwench at TEN7 commentedsource.module
tosource_module
.Comment #92
heddnNit: should be plural Implements
Consider doing what we are doing in commerce_migrate where we are checking the source and/or destination classes.
As a specific example, see commerce_migrate_commerce_migration_plugins_alter
You should be able to use the 'Content' tag. Not all content extends ECB we found. See https://www.drupal.org/node/2944527. (only useful on 8.5+)
Just because metatag is enabled on the destination doesn't mean it has a source in d6/d7. Unless I misunderstand, do a table exists check first. And actually, is this even necessary at all? Won't the normal field discovery discover this field already? Perhaps a comment explaining the need could be added.
This should be able to use Class::class syntax.
A comment why this flag is important would be useful. The docs for it were just updated in 8.6.x, so that could be a first place review if we are unsure its reason.
This should be a full db dump. It will be a pain to maintain over time. Just bite the bullet and add a full fixture. See https://www.drupal.org/docs/8/api/migrate-api/generating-database-fixtur...
Comment #93
marvil07 CreditAttribution: marvil07 at Isovera commentedI could not apply the patch at #90 to the current 8-x.1-x, e1d016c1f8f2dd85a8985a728f75cb96d1e9d22e at the time I checked, hence the interdiff is not highly accurate.
done: 1, 5
todo: 2, 3, 4, 6, 7
Needs Review only for testbot.
Test failure seems to be related with the fixture, so 7 could probably help.
Comment #95
marvil07 CreditAttribution: marvil07 at Isovera commentedI was about to create a new ticket for the case when no d7 configuration is migrated, but I think it should be handled here too, since most of the work is already there.
What is new:
hook_entity_base_field_info()
.d7_metatag_field_instance
migration is problematic, especially for people not migrating configuration, I have removed that line, but we should find a way to re-add it somewhere.Update on #92:
Two patches posted for convenience: one for 8.x-1.x, and one for 8.x-1.4, to be able to actually use the patch using current stable.
Comment #97
heddnCan we add a link to the issue that is creating a more general version?
Support a known subset.
This query will fail if source isn't in the conditionals of user, term, or node. Let's check that $entity_type isset.
!empty() should be enough here.
Can we get a comment from a module maintainer on this?
This says metatag, but then also lists comment. Can we clarify what we want?
The destination plugin manager lets you createInstance as a stub. That's how this is typically done.
If we only support node, user, terms, then we cannot just check on ECB. We need to be more specific.
How come metatag fields don't get discovered by normal field discovery? Do we need to add a field plugin instead of this here? See https://www.drupal.org/docs/8/api/migrate-api/writing-migrations-for-con...
Sorry, I didn't notice this in my past review. But an answer to this fundamental question might be useful before pressing on with the current approach.
Comment #98
heddnComment #99
heddnA typical thing a field plugin does it insert a process section that then references new metatag process plugin. This other plugin would accept the entity id and be able to query and retrieve the actual value for the metatag via its transform function. The tricky thing is discovering the entity id. But I think we can do that because we have the full migration passed in as an argument. From that, we can glean the destination. And then from the destination and using the entity type manager, get the name of the id and revision_id keys in the destination. Then pass in that value into this metatag process would do its transform.
Example of what that would look like for nodes:
Retrieve the keys in the field plugin like this:
Which will result in the following yaml coming out of
processFieldValues()
:And our new FieldPlugin/field_metatag process plugin would then get all the existing logic that we currently have in the prepare_row hook.
Comment #100
PierreRicci CreditAttribution: PierreRicci commentedHi,
Great I need to migrate data from a old site. But is this patch can be use for non drupal data ?
I’ve try to make it myself for my field_metatag with no chance ( my array is not saved).
Is this patch reveal the next release of the module without the migration in a src/plugin... directory ?
Thanks
Comment #101
benjifisher@PierreRicci:
I do not see any code in this patch for a destination plugin, so I do not think it will help unless you are migrating from another Drupal site.
Until this issue is marked Fixed, the only way to get the update is by downloading and applying a patch from this page.
Once this issue is marked Fixed, the updated code should be available ...
Comment #102
ccarrascal CreditAttribution: ccarrascal commentedHi all, I am importing metatags data using a combination of #65 and #79 and it's working fine.
Comment #103
DamienMcKennaComment #104
DamienMcKennaMinor tweaks.
Comment #106
DamienMcKennaComment #107
DamienMcKennaI copied fileMigrationSetup() from FileMigrationSetupTrait in 8.4 because I didn't want to deal with extending getFileMigrationInfo().
Comment #109
DamienMcKennaThere are two main errors:
The entity type is actually called "metatag_defaults".
This happens because the Migrate APIs changed; see: https://www.drupal.org/node/2795403
Lets comment out that part of the migrate tests, maybe deal with it later.
Comment #111
DamienMcKennaThe outstanding failure is:
Comment #112
heddnI think this will be less brittle if we create a deriver and derive 3 migrations for just the field_metatag to node, terms, users. Then in the derivative code, check if the source has a metatag field set. If so, then map just that field. We can depend on d7_node, d7_user, etc to make sure the node, user, etc migrations have finished first. Use migration_lookup for the nid, uid, tid, etc.
Use a field plugin. This shouldn't be necessary.
Is there any langcode we need to account for here?
Would a generator work better than a foreach loop?
I don't see where we are inserting any user metatag data. It might effect anything, but I've found that testing a bundlable and a non-bundlable entity helps catch bugs.
This assumes a certain folder structure. That could be fragile on the testbots.
Comment #113
heddnOr rather than 112.1, use a field plugin.
Comment #114
DamienMcKenna@heddn: thanks for the review.
Completely agreed on 1, 2 and 5.
For 3, I don't know where the language support should go. Lets leave it as a todo item for now
I cannot respond on #4, but I'll take your word for it :)
For 6, that's working off the current file structure of the module, any reason why that wouldn't be safe to do?
Comment #115
heddn#6: if the testbot moves where it puts contrib modules was my thought. I found
CreateMigrationsTrait
in core that seems to be a decent means to the same end. Not quite what we want, but closer and we can copy/paste from it.Comment #116
heddn#112.3: we should figure out langcode support before this lands. Adding language support after commit is problematic as the mapping table gets created with all the columns in the getIds. So if we have anything to put in there from the source or destination, then we should now.
Comment #117
DamienMcKennachx provided an alternative approach to this:
https://gist.github.com/chx/232ba2d2bb32ceb2eef68045d54bb53b
Comment #118
chefigueroa CreditAttribution: chefigueroa commentedI am new to Drupal. I installed this patch and ran the migration and it didn't work. To be able to migrate fields from metatags from Drupal 7 to Drupal 8, all I have to do is install this patch and go ahead and migrate? Or is there something I am missing?
Thanks!
Comment #119
DamienMcKenna@che607: The issue's status says "needs status" and the last few comments indicate work needs to be done on it, including proper language handling. So unfortunately this isn't ready for you to use yet.
Comment #120
DamienMcKennaThis blog post might help: https://thinktandem.io/blog/2018/07/24/writing-a-custom-drupal-8-module-...
Comment #121
DamienMcKennaComment #122
DamienMcKennaComment #123
marcelovaniI have a migration to do where it only has one language.
Patch #109 is fine for my case.
Apart from one small problem:
The code assumes that the legacy site is running the latest version of Metatag Module.
In this case, the site is running on version 7.x-1.0-beta7, which doesn't support metatag revisions yet.
I know some might say we should upgrade that site first but that is totally out of the scope, and due to some complicated dependencies it won't be that simple to upgrade the legacy site.
So the easy solution is to add a check for the field
I am putting a path together and interdiff. Also updated the Readme to help people that are new to migrations.
Comment #124
DamienMcKenna@marcelovani: Migrations from old versions of Metatag will not be supported, sites will need to be running the latest stable version of Metatag for D7; while I appreciate the documentation improvements, the code change won't included, but I thank your thoroughness.
Comment #125
floown CreditAttribution: floown commentedHello,
Sorry, I’m french, I try to learn what I should do to migrate a Drupal 7 to Drupal 8. The previous time I have use Migrate UI, the metatags have not imported to my D8.
Should I redo the process with Metatag in Development version: 8.x-1.x-dev updated 15 Apr 2019 at 13:53 UTC? And use Migrate instead Migrate UI?
Regards
Comment #126
DamienMcKenna@floown: no, we haven't finished the migration part yet for Metatag, so using the dev version of Metatag won't make any difference.
Comment #127
DamienMcKennaFrom marvil07 in Slack:
Comment #128
DamienMcKennaOk, let's see if this fixes the tests.
Comment #130
DamienMcKennaLet's see if this fixes the problem with the StreamWrapperInterface namespace.
Comment #132
DamienMcKennaThis time maybe?
Comment #134
DamienMcKennaThe current error is:
Anyone have any thoughts? I couldn't find "d7_node:blog" in the entire core codebase. Does it need to be a new item passed to executeMigrations()?
Comment #136
DamienMcKennaOk, now we're getting to the actual tests for this change:
Comment #137
DamienMcKennaSo somehow the array is being changed from:
to:
Comment #138
PapaGrande@damienmckenna Thanks a lot for the hard work. I was able to get the migration working with the #134 patch except that the metatag array keys are subtly different between D7 & D8. In my case, I had to translate “og:image” to “og_image” and “twitter:image” to “twitter_card_image”. Does the migration code need to use the metatag plugin id instead of the name?
Here's what I did in my hook_migrate_prepare_row(), which runs last:
Also, for others' reference, the migration YML is simple:
Comment #139
DamienMcKennaOn this week's Contrib Half Hour we looked at this and think we have a solution. The problem was in metatag_migrate_prepare_row() where it was only copying the "value" portion of the array whereas it needed to keep the entire thing.
Comment #140
DamienMcKennaWhoops, accidentally forgot to uncomment a line I removed while testing.
Comment #141
DamienMcKennaSome coding standards improvements.
Comment #142
DamienMcKennaAnother few minor tweaks.
Comment #143
DamienMcKennaSo, the last part is working out how to convert the D7 meta tag names to D8's.
Comment #144
drupalninja99 CreditAttribution: drupalninja99 at Mediacurrent commentedI am running the patch from #142 and the field instances are carrying over correctly but my custom meta tag values on a node are not coming through. I have a custom title and description for one node but the data does not get imported.
Comment #145
DamienMcKennaOne additional request I believe we should add is to rename the pseudo field to "pseudo_field_metatag" to avoid people getting confused when they review the codebase, so they understand it's not a real field.
Comment #146
DamienMcKennaComment #147
DamienMcKennaComment #148
oliverpolden CreditAttribution: oliverpolden commentedUpdated patch to handle canonical renamed to canonical_url.
Comment #149
DamienMcKenna@oliverpolden: Thank you for giving that a try. I think it needs to be more generic, though, so probably a source:destination array and a foreach loop.
Comment #150
oliverpolden CreditAttribution: oliverpolden commentedI've updated this to make it more generic but there may be a better place to put the array.
Comment #151
DamienMcKennaOk, this adds a new helper class for storing all that messy mapping. Right now it only works with the meta tags bundled in the main module, it needs to be expanded to handle the rest, and to add a hook to allow the mapping to be modified to support other tags.
Comment #152
DamienMcKennaComment #153
oliverpolden CreditAttribution: oliverpolden commentedThanks @damienmckenna. Interdiff looks good to me so far.
Comment #154
DamienMcKennaThis should cover all meta tags currently present in D8, any tag not available yet is listed as a @todo item.
Comment #156
DamienMcKennaDoh. Lets try this again.
Comment #158
DamienMcKennaA second doh - variable name typoo.
Comment #159
DamienMcKennaWoohoo! Tests are green!
Comment #160
heddnThis is really fantastic work. I've read through the patch enough to know we've got a lot of really great things going on here. However, one thing that really bothers me (a lot) is that our prepare_row knows about the source and the destination. This really breaks one of the cardinal rules of ETL. These should be decoupled.
Rather than doing that, could we build out N migrations in a migration deriver. It would have one migration for each of the entity types. At time of constructions of the migration, the migration can filter out what migrations need to be created or not. And at the migration level, we have full access to source and destination. Once we dive into the source or the process plugins or destination, those shouldn't know about anything else.
So the deriver can generate migrations. Great. I think the helper class could easily then be used in that generated migration and we wouldn't need any prepare_row any more.
Could we use a plugin deriver and build out 1-N migrations depending on if the sources exist? Then use field plugins for
field_metatag
?Can't this use
Class:class
syntax?Comment #161
DamienMcKennaSome minor improvements to the README.txt file and the pseudo field has been renamed to "pseudo_field_metatag".
Comment #162
DamienMcKenna@heddn: Thank you for the details response. While I don't understand all of the words you used ;-) I do appreciate that this current solution might not be as flexible as we need. I also suspect the upgrade would fail if the destination field name was not exactly "field_metatag".
Do you have examples of other modules that use the deriver functionality? I might have some time this week to put on a snorkel and go swimming. Thanks.
I wonder if the field data conversion should be put into a custom plugin instead of using the hooks? That would make it more reusable for custom migrations.
Comment #163
heddnHere's a migration deriver: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/node/s.... ERR uses one too: https://git.drupalcode.org/project/entity_reference_revisions/blob/8.x-1... They all extend DeriverBase, so looking for other implementations of that class in core will give you additional examples.
Edit: fixed link to ERR deriver.
Comment #164
DamienMcKennaSomething was accidentally broken with the last patch(es) and in a manual test none of the meta data was migrated.
Comment #165
oliverpolden CreditAttribution: oliverpolden commentedI'm checking the latest patch, 161 with config: field_metatags: pseudo_field_metatag (I hacked the code locally to handle field_metatags instead of field_metatag).
And I get this error:
which refers to:
Also, perhaps it would be good for the readme to refer to field_metatag instead of field_metatags for the time being.
Comment #166
oliverpolden CreditAttribution: oliverpolden commentedThere has been talk about the "value" element issue:
In D7 the serialized array is:
a:3:{s:5:"title";a:1:{s:5:"value";s:152:"Some string...
This gets migrated as is to D8, i.e. D8 has the "value" element in it which means the title metatag doesn't display when viewing the node.
Upon saving the node, the array is updated to:
a:2:{s:5:"title";s:152:"Some string...
Now the title metatag is visible.
So, it looks like we think the array should be migrated as is, but the metatag will not display until the node is saved and the "value" element is removed.
It's the following line where the metatag seems to get lost if there is the "value" element, MetatagManager::generateRawElements():
Comment #167
DamienMcKennaOk, this is really confusing. It works via "drush migrate-upgrade" if you do --configure-only first and then run the full migration, but doing the full migration doesn't save the records even though they're correctly processed by metatag_migrate_prepare_row().
Comment #168
DamienMcKennaI've noted the two remaining tasks.
Comment #169
DamienMcKennaThree remaining tasks.
Comment #170
DamienMcKennaAnother change that we need to do is move the custom data shuffling to a process plugin.
Comment #171
DamienMcKennaI'm working on the process plugin, will finish it later today.
Comment #172
DamienMcKennaLet's see how this works - I moved the custom conversion logic into a generic process plugin.
Comment #173
DamienMcKennaComment #174
DamienMcKennaUpdated the README.txt file accordingly.
Comment #176
DamienMcKennaTypoo, "process" instead of "plugin".
Comment #178
DamienMcKennaThe tests run locally, but no data is migrated when I test it manually. Not sure what's going on here.
Comment #179
DamienMcKennaSo... the tests pass but it fails locally. Not sure what's going on anymore.
That said, the restructuring as a plugin seems to work ok.
Comment #180
oliverpolden CreditAttribution: oliverpolden commented@damienmckenna Have you checked what you get in the database when you migrate vs save a node in D8?
Comment #181
DamienMcKennaI hadn't yet.
This is what gets stored in D8:
a:2:{s:11:"description";s:4:"Test";s:6:"robots";s:20:"noarchive, nosnippet";}
Instead of storing the data as nested arrays, it's more of a key:value pair.
Anyone want to change the structure? It should be more of a novice task for someone.
Comment #182
oliverpolden CreditAttribution: oliverpolden commentedDo we know what the structure of the serialized array should be in D8?
#137 seems to suggest that there should be a "value" element. But currently if you save a node in D8 then there is no "value" element.
The migration currently preserves the "value" element and the migrated metatags won't display until the migrated node is saved again.
So which of these is the correct solution:
It seems to me that the "value" element is redundant and it's correct that it's removed from D8, plus we probably don't want to change that now. So I would say that the migration should remove the "value" element which it currently does not.
Comment #183
DamienMcKennaSo #137 was my mistake, sorry about that X-)
The migration needs to be updated to remove the nested aspect of the array. It might be worth adding the "robots" tag to the test so we can make sure multi-value items are also stored correctly.
Comment #184
heddnso, after doing a more thorough review of metatag in d7, we can't really use field plugins. So we will have to use exactly what this patch is doing. Which is
hook_migrate_prepare_row
andhook_migration_plugins_alter
. It is a little ugly, but given the limitations of the source data one of the few options for getting it to work.I really like seeing the metatag process plugin created. That's a really nice addition. The only funny thing is the destination field mapping. This patch assumes that the destination is `field_metatags`. That's not a bad assumption, considering there is an early migration that creates the field with that name. But we don't have any optional migration dependencies added to any of the node or term migrations. That should happen so we can be certain the field is created before the data is dumped into it. That might solve some of the mysteries of things working sometimes.
Comment #185
DamienMcKennaThanks heddn, that's good news on the architectural recommendation.
I'll look into the dependencies.
Comment #186
DamienMcKennaWould something like this work?
Comment #187
heddn#186: that's about what I was thinking. You'll have to test, but that's the general idea. Optionals (should) only influence order of running, not require 100% successful completion of the previous migration(s).
Comment #188
DamienMcKennaI tried adding 'migration_dependencies' items but it didn't make any difference.
Comment #189
heddnYou could ignore based on 'Content' or 'Configuration' migration_tags on the migrations. That would help keep this to a more managable list. You'd have to add a few content entity destinations, but a lot less.
Comment #190
DamienMcKennaUpdated the migration to no longer nest values, so migrated values should be correct again.
Comment #191
DamienMcKennaAnd now support for array-based tags, like "robots".
Comment #192
DamienMcKennaI nuked my local D8 testbed and the migration works again via migrate_upgrade.
Comment #193
DamienMcKennaOk, this seems to work ok in my manual testing, both with migrate_upgrade and migrate_drupal_ui.
Anyone want to second it?
Any further suggestions for refinements?
Comment #194
oliverpolden CreditAttribution: oliverpolden commentedJust picking up on #184 that mentions we're assuming the field in D8 is "field_metatags" and not "field_metatag". I believe the patch was assuming the field in D8 is actually "field_metatag".
Is there a default field name in D8? As #184 says, it would be good to ensure the patch/update assumes the field is whatever D8 creates by default (if it creates/names it by default).
I'm using "field_metatags" on my project and should be able to confirm back in the next couple of days how it goes with the latest patch at the time.
Thank you very much with your continued effort on this, it's a subtly important module for Drupal in terms of ensuring sites perform well on search engines etc.
Comment #195
DamienMcKennaThere are two different issues here.
If you're customizing your migration instead of doing a guided migration then you'll need to follow the second option and it'll work.
Reminder: Metatag on D8 doesn't create any entity fields for you by default, you have to do that manually.
Comment #196
DamienMcKennaImproved documentation in the README.txt and at the top of metatag_migration_plugins_alter().
Comment #197
DamienMcKennaI renamed pieces of the migration to make room to also migrate the configuration later, the entity migration pieces now have "entities" in their names.
Comment #198
heddnI had to really stretch to find anything. The following 2 items could be fixed on commit or ignored if there is still good reason to leave as is. I'm going to assume the bot will come back green. So onward to RTBC.
This todo seems completed w/ the latest readme doc updates.
I don't see that
unserialize
throws an exception.Comment #199
DamienMcKennaThanks @heddn.
For the first item, I'd like to document how someone could modify the destination when performing a guided migration.
I've replaced the try/catch block with a check to ensure the value from unserializing the old value is actually an array.
Comment #200
DamienMcKennaForgot to remove $e->getMessage().
Comment #201
heddnGreat! Let's wait on the bot now.
Comment #202
DamienMcKennaI renamed the fixtures and tests to separate them from other tests we'll add for other migrations.
Comment #203
DamienMcKennaComment #204
DamienMcKennaI created a new issue for handling the dynamic hreflang meta tags (#3077778: Migrate: per-language hreflang meta tags) and updated the tag mapping with comments indicating which issues handle adding the missing tags.
Comment #205
DamienMcKennaSorry for the noise, folks. I renamed the hook and made some minor coding standards improvements.
Comment #207
DamienMcKennaAnd it's committed! Woohoo, thanks everyone!
Comment #208
floown CreditAttribution: floown commentedOMG, we can now import metatags from D7 to D8? All it's ok?
Comment #209
DamienMcKennaThe global defaults aren't migrated over yet, that'll be in #2563651: Migrations: Metatag-D7 default configurations. But the per-entity values should be a-ok now.
Note: some meta tags haven't been added to Metatag-D8 yet, please see this list:
Comment #210
oliverpolden CreditAttribution: oliverpolden commentedCan confirm my custom migration of metatags is now working after updating the plugin and source as in the README. Thank you so much for your work on this @damienmckenna
Comment #211
DamienMcKenna@oliverpolden: That's excellent, thanks for letting us know.
Comment #213
John_B CreditAttribution: John_B commentedDocumenting that the import of entity-specific metatags works with both migrate_tools and core migrate commands. However, it only works if the metatag migrations have been run before the node migrations.
A dependency would be useful, to save time puzzling why the metatag data has failed to migrate, where it is imported before metatag has been properly set up on the target site. I am not opening a ticket because the maintainers probably have enough work, but if anyone else agrees this is an issue, they might want to open a ticket.
Comment #214
DamienMcKenna@John_B: That's a reasonable idea, please open a new issue so we can look into it and so that the idea isn't lost. Thank you.
Comment #215
John_B CreditAttribution: John_B commentedOpened #3281110: Entity metatag migrations fail unless metatag migrations are run before entity migrations and made it a child of this issue.