Problem/Motivation

An upgrade path is required for upgrading from Drupal 7.

Proposed resolution

Provide integration with the Migrate system to allow upgrading from Drupal 7.

Remaining tasks

  • Add a system to map D7 meta tag names to D8 meta tag names.
  • Rename the pseudo field to "pseudo_metatag_d7".
  • Improve documentation in the README.txt file about the migration setup.
  • Move meta tag conversion logic to a process plugin (#170).
  • Fix the migration so it works correctly during manual testing (#167, #179).
  • Fix the converted data so it is fully compatible with Drupal 8 (#166, #181/182/183).
  • Rewrite to use a migration deriver (#160). Won't do per #184.

User interface changes

n/a

API changes

TBD.

Data model changes

n/a

Release notes snippet

(Major and critical issues should have a snippet that can be pulled into the release notes when a release is created that includes the fix)

CommentFileSizeAuthor
#205 metatag-n2563649-205.patch54.45 KBDamienMcKenna
#205 metatag-n2563649-205.interdiff.txt3.83 KBDamienMcKenna
#204 metatag-n2563649-204.patch54.32 KBDamienMcKenna
#204 metatag-n2563649-204.interdiff.txt6.64 KBDamienMcKenna
#202 metatag-n2563649-202.patch53.55 KBDamienMcKenna
#200 metatag-n2563649-200.interdiff.txt726 bytesDamienMcKenna
#200 metatag-n2563649-200.patch50.67 KBDamienMcKenna
#199 metatag-n2563649-199.patch50.69 KBDamienMcKenna
#199 metatag-n2563649-199.interdiff.txt785 bytesDamienMcKenna
#197 metatag-n2563649-197.patch50.63 KBDamienMcKenna
#197 metatag-n2563649-197.interdiff.txt39.9 KBDamienMcKenna
#196 metatag-n2563649-196.patch50.51 KBDamienMcKenna
#196 metatag-n2563649-196.interdiff.txt3.36 KBDamienMcKenna
#191 metatag-n2563649-191.interdiff.txt3.58 KBDamienMcKenna
#191 metatag-n2563649-191.patch49.23 KBDamienMcKenna
#190 metatag-n2563649-190.patch49.24 KBDamienMcKenna
#190 metatag-n2563649-190.interdiff.txt6.2 KBDamienMcKenna
#188 metatag-n2563649-188.interdiff.txt3.86 KBDamienMcKenna
#188 metatag-n2563649-188.patch47.81 KBDamienMcKenna
#178 metatag-n2563649-178.interdiff.txt1.45 KBDamienMcKenna
#178 metatag-n2563649-178.patch46.34 KBDamienMcKenna
#176 metatag-n2563649-176.patch46.08 KBDamienMcKenna
#176 metatag-n2563649-176.interdiff.txt521 bytesDamienMcKenna
#174 metatag-n2563649-174.patch46.08 KBDamienMcKenna
#174 metatag-n2563649-174.interdiff.txt390 bytesDamienMcKenna
#172 metatag-n2563649-172.patch46.05 KBDamienMcKenna
#172 metatag-n2563649-172.interdiff.txt36.96 KBDamienMcKenna
#167 metatag-n2563649-166.interdiff.txt1.78 KBDamienMcKenna
#167 metatag-n2563649-166.patch45.49 KBDamienMcKenna
#161 metatag-n2563649-161.interdiff.txt2.44 KBDamienMcKenna
#161 metatag-n2563649-161.patch45.46 KBDamienMcKenna
#158 metatag-n2563649-158.interdiff.txt1.99 KBDamienMcKenna
#158 metatag-n2563649-158.patch45.22 KBDamienMcKenna
#156 metatag-n2563649-156.patch45.22 KBDamienMcKenna
#156 metatag-n2563649-156.interdiff.txt756 bytesDamienMcKenna
#154 metatag-n2563649-154.interdiff.txt28.28 KBDamienMcKenna
#154 metatag-n2563649-154.patch45.22 KBDamienMcKenna
#151 metatag-n2563649-151.patch39.57 KBDamienMcKenna
#151 metatag-n2563649-151.interdiff.txt17.94 KBDamienMcKenna
#150 metatag-n2563649-150.interdiff.txt832 bytesoliverpolden
#150 metatag-n2563649-150.patch27.07 KBoliverpolden
#148 interdiff_142-148.txt332 bytesoliverpolden
#148 metatag-n2563649-148.patch26.94 KBoliverpolden
#142 metatag-n2563649-142.interdiff.txt1.21 KBDamienMcKenna
#142 metatag-n2563649-142.patch26.82 KBDamienMcKenna
#141 metatag-n2563649-141.patch26.85 KBDamienMcKenna
#141 metatag-n2563649-141.interdiff.txt10.9 KBDamienMcKenna
#140 metatag-n2563649-140.patch26.93 KBDamienMcKenna
#140 metatag-n2563649-140.interdiff.txt776 bytesDamienMcKenna
#139 metatag-n2563649-139.patch26.93 KBDamienMcKenna
#139 metatag-n2563649-139.interdiff.txt1.28 KBDamienMcKenna
#134 metatag-n2563649-134.interdiff.txt498 bytesDamienMcKenna
#134 metatag-n2563649-134.patch26.93 KBDamienMcKenna
#132 metatag-n2563649-132.interdiff.txt494 bytesDamienMcKenna
#132 metatag-n2563649-132.patch26.91 KBDamienMcKenna
#130 metatag-n2563649-130.interdiff.txt753 bytesDamienMcKenna
#130 metatag-n2563649-130.patch26.87 KBDamienMcKenna
#128 metatag-n2563649-128.patch26.81 KBDamienMcKenna
#128 metatag-n2563649-128.interdiff.txt2.86 KBDamienMcKenna
#123 metatag-n2563649-123.interdiff.txt1.83 KBmarcelovani
#123 metatag-n2563649-123.patch24.29 KBmarcelovani
#109 metatag-n2563649-109.interdiff.txt1.34 KBDamienMcKenna
#109 metatag-n2563649-109.patch22.89 KBDamienMcKenna
#107 metatag-n2563649-107.interdiff.txt2.12 KBDamienMcKenna
#107 metatag-n2563649-107.patch22.85 KBDamienMcKenna
#104 metatag-n2563649-104.patch21.76 KBDamienMcKenna
#104 metatag-n2563649-104.interdiff.txt12.87 KBDamienMcKenna
#95 interdiff-2563649-93-migrate.txt4.41 KBmarvil07
#95 2563649-95-8.x-1.4.patch22.38 KBmarvil07
#95 2563649-95-8.x-1.x.patch21.88 KBmarvil07
#93 interdiff.txt1.18 KBmarvil07
#93 2563649-93-migrate.patch20.9 KBmarvil07
#90 interdiff_2563649_86-90.txt3.58 KBsocketwench
#90 metatag-n2563649-90.patch22.08 KBsocketwench
#86 metatag-n2563649-86.interdiff.txt1.18 KBDamienMcKenna
#86 metatag-n2563649-86.patch22.11 KBDamienMcKenna
#82 metatag-n2563649-82.interdiff.txt1.88 KBDamienMcKenna
#82 metatag-n2563649-82.patch22.01 KBDamienMcKenna
#78 interdiff-76-78.txt3.42 KBWidgetsBurritos
#78 metatag-n2563649-78.patch23.58 KBWidgetsBurritos
#76 metatag-n2563649-76.patch23.37 KBWidgetsBurritos
#72 metatag-n2563649-70.patch22.17 KBpobster
#69 metatag-n2563649-69.patch22.16 KBpobster
#66 metatag-n2563649-66.patch21.94 KBpobster
#62 metatag-n2563649-62.patch20.32 KBDamienMcKenna
#52 migrations_basic-2563649-52.patch20.28 KBjofitz
#52 interdiff.txt2.36 KBjofitz
#4 migrations_basic-2563649-4.patch6.92 KBjofitz
#6 migrations_basic-2563649-6.patch7.63 KBjofitz
#9 migrations_basic-2563649-9.patch9.75 KBjofitz
#11 migrations_basic-2563649-11.patch9.73 KBjofitz
#13 migrations_basic-2563649-13.patch10.1 KBjofitz
#15 migrations_basic-2563649-15.patch10.08 KBjofitz
#15 interdiff.txt20.2 KBjofitz
#18 migrations_basic-2563649-18.patch10.04 KBjofitz
#18 interdiff.txt957 bytesjofitz
#19 migrations_basic-2563649-19.patch15.41 KBjofitz
#19 interdiff.txt11.98 KBjofitz
#24 aaa.JPG87.19 KBBenia
#27 interdiff.txt18.94 KBjofitz
#27 migrations_basic-2563649-27.patch15.62 KBjofitz
#29 migrations_basic-2563649-29.patch15.06 KBjofitz
#31 migrations_basic-2563649-31.patch15.03 KBjofitz
#32 interdiff.txt7.06 KBjofitz
#32 migrations_basic-2563649-32.patch16.11 KBjofitz
#34 migrations_basic-2563649-34.patch16.12 KBjofitz
#37 interdiff.txt6.1 KBjofitz
#37 migrations_basic-2563649-37.patch16.12 KBjofitz
#39 interdiff.txt5.84 KBjofitz
#44 migrations_basic-2563649-44-w_o_user_test.patch20.26 KBjofitz
#39 migrations_basic-2563649-39.patch20.26 KBjofitz
#44 interdiff.txt759 bytesjofitz
#44 migrations_basic-2563649-44-complete.patch20.26 KBjofitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

jofitz’s picture

I'm in the process of writing the migration integration for D7.

DamienMcKenna’s picture

That would be amazing, thank you!

jofitz’s picture

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

DamienMcKenna’s picture

Is there any way to make this more generic so it can work with any entity instead of just nodes?

jofitz’s picture

As requested, I have improved the patch to handle any entity rather than only nodes.

Steven Jones’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

  1. +++ b/metatag.module
    @@ -262,3 +269,55 @@ function metatag_get_default_tags() {
    +  $is_not_metatag_migration = strpos($migration->id(), 'd7_metatag_field') !== 0;
    

    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.

  2. +++ b/metatag.module
    @@ -262,3 +269,55 @@ function metatag_get_default_tags() {
    +    $entity_id = $row->getSourceProperty('nid');
    

    This code still looks to be node specific. For example, when migrating users, they would not have a nid property, but a uid property (I assume).

  3. +++ b/metatag.module
    @@ -262,3 +269,55 @@ function metatag_get_default_tags() {
    +    $revision_id = $row->getSourceProperty('vid');
    

    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.

  4. +++ b/metatag.module
    @@ -262,3 +269,55 @@ function metatag_get_default_tags() {
    +  $is_not_metatag_migration = strpos($migration->id(), 'd7_metatag_field') !== 0;
    

    Same comments as 1 above.

  5. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
    @@ -0,0 +1,73 @@
    +  public function initializeIterator() {
    

    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.

  6. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
    @@ -0,0 +1,73 @@
    +    while ($entity_types->valid()) {
    

    As you're iterating over an iterator you should be able to simplify this code by using a foreach instead of the while.
    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.

DamienMcKenna’s picture

Thanks for the review, Steven, I appreciate it.

jofitz’s picture

Status: Needs work » Needs review
FileSize
9.75 KB

Made changes suggested in code review.

Made updates for 8.1.1 (i.e. migrations are now plugins rather than entities).

Status: Needs review » Needs work

The last submitted patch, 9: migrations_basic-2563649-9.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
9.73 KB

Re-roll patch.

Status: Needs review » Needs work

The last submitted patch, 11: migrations_basic-2563649-11.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
10.1 KB

Add missing module dependency (migrate) to MetatagNodeTranslationTest.

Status: Needs review » Needs work

The last submitted patch, 13: migrations_basic-2563649-13.patch, failed testing.

jofitz’s picture

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

jofitz’s picture

Status: Needs work » Needs review
Steven Jones’s picture

Status: Needs review » Needs work

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

  1. +++ b/metatag_migrate/metatag_migrate.module
    @@ -0,0 +1,75 @@
    +
    

    Very minor nit pick: You don't need this extra blank line here.

  2. +++ b/metatag_migrate/metatag_migrate.module
    @@ -0,0 +1,75 @@
    +    /** @var SqlBase $source */
    +    $db = $source->getDatabase();
    +    /** @var SelectInterface $query */
    +    $query = $db->select('metatag', 'm')
    

    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.

  3. +++ b/metatag_migrate/metatag_migrate.module
    @@ -0,0 +1,75 @@
    +  return TRUE;
    

    Migrate only cares if you return FALSE from the prepare hooks, so you could probably just ditch the return statement here.

jofitz’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
957 bytes

Re-roll in response to code review.

jofitz’s picture

FileSize
15.41 KB
11.98 KB

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

mikeryan’s picture

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.

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

DamienMcKenna’s picture

Status: Needs review » Needs work

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

Benia’s picture

Deleted.

Benia’s picture

Deleted.

Benia’s picture

FileSize
87.19 KB

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

Steven Jones’s picture

@Benia with the current patch in #19 you'll need to enable the metatag_migrate sub module to get the migration code loaded.

Benia’s picture

That'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 !

jofitz’s picture

Status: Needs work » Needs review
FileSize
18.94 KB
15.62 KB

Transferred code back out of metatag_migrate sub-module.

Status: Needs review » Needs work

The last submitted patch, 27: migrations_basic-2563649-27.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
15.06 KB

Corrected the patch.

Status: Needs review » Needs work

The last submitted patch, 29: migrations_basic-2563649-29.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
15.03 KB
jofitz’s picture

Issue tags: -Needs tests
FileSize
7.06 KB
16.11 KB

Add Unit test. Move Kernel test.

Status: Needs review » Needs work

The last submitted patch, 32: migrations_basic-2563649-32.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
16.12 KB

Status: Needs review » Needs work

The last submitted patch, 34: migrations_basic-2563649-34.patch, failed testing.

The last submitted patch, 34: migrations_basic-2563649-34.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
16.12 KB
  • Added dependency injection to MetatagFieldInstance.
  • Added unit test for MetatagFieldInstance.
  • Tweaked relative path in MigrateMetatagTest in case that passes the test (patch is passing all tests locally, just not testbot).

Status: Needs review » Needs work

The last submitted patch, 37: migrations_basic-2563649-37.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
5.84 KB
20.26 KB

Upload patch that should have been in #37.

Status: Needs review » Needs work

The last submitted patch, 39: migrations_basic-2563649-39.patch, failed testing.

jofitz’s picture

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

DamienMcKenna’s picture

Jo: Are you running the latest 8.2.x code locally?

jofitz’s picture

I am not. I've been testing this ticket against 8.1.1. I'll give that a go, thanks.

jofitz’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 44: migrations_basic-2563649-44-complete.patch, failed testing.

jofitz’s picture

Status: Needs work » Postponed
Benia’s picture

Should 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 ?

jofitz’s picture

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

csedax90’s picture

It's possibile to use this patch with a custom migration and not only with a D7 -> D8 migration?

DamienMcKenna’s picture

Can metatag_migrate_prepare_row() be moved into a separate file, as was possible with D7?

jofitz’s picture

jofitz’s picture

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

jofitz’s picture

@DamienMcKenna RE Your question in #50 - why would you want metatag_migrate_prepare_row() in a separate file?

DamienMcKenna’s picture

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

jofitz’s picture

@Damien No, I don't think metatag_migrate_prepare_row() can be moved into a separate file because there is no migrate_hook_info().

DamienMcKenna’s picture

Ah, ok, in that case it's fine.

Thanks for your work on this, I'll try to test it soon.

tedfordgif’s picture

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

# migrate_plus.migration.migration_name.yml
id: migration_name
# ...
destination:
  plugin: entity:node
process:
  # ...
  field_meta_tags: meta_tags
/**
 * Migration source example.
 *
 * @MigrateSource(
 *   id = "migration_name"
 * )
 */
class MigrationSourceExample {

  /**
   * {@inheritdoc}
   */
  public function prepareRow(Row $row) {
    // ...
    $metatags = [
      'title' => $row->getSourceProperty('HEAD_TITLE') . ' | [site:name]',
      'description' => $row->getSourceProperty('DESCRIPTION'),
      'keywords' => $row->getSourceProperty('KEYWORDS'),
    ];
    $row->setSourceProperty('meta_tags', serialize($metatags));

    return parent::prepareRow($row);
  }

}
DamienMcKenna’s picture

The 1.0 release is being bumped up, so the everything else is being put on the back burner for now.

DamienMcKenna’s picture

Status: Needs review » Postponed

Postponing until after 1.0.

DamienMcKenna’s picture

Status: Postponed » Needs review

8.x-1.0 is out, so this is fair game again.

matthieuscarset’s picture

Hi 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?

DamienMcKenna’s picture

FileSize
20.32 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 62: metatag-n2563649-62.patch, failed testing. View results

mglaman’s picture

Schema errors for node.type.article with the following errors: node.type.article:third_party_settings.menu_ui.available_menus missing schema, node.type.article:third_party_settings.menu_ui.parent missing schema (/var/www/html/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95)
Failed asserting that false is true.
+++ b/tests/src/Kernel/Migrate/d7/MigrateMetatagTest.php
@@ -0,0 +1,93 @@
+class MigrateMetatagTest extends MigrateDrupal7TestBase {
+
+  static $modules = [

Looks like this needs to also add menu_ui

cruno’s picture

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

$row->setSourceProperty('metatags', $this->getNodeMetatags($nid));

In NodeMigrations.php, which MyNodeMigration.php extends:

**
   * Gets the Drupal 7 meta tags for a given node and translates into Drupal 8.
   *
   * @param int $host_id
   *   Node Id.
   *
   * @return string
   *   Serialized array of metatag key value pairs.
   */
  protected function getNodeMetatags($host_id) {
    $data = $this->select('metatag', 'm')
      ->condition('entity_id', $host_id)
      ->fields('m', ['data'])
      ->execute()
      ->fetchField();

    if (!empty($data)) {
      $metatags = [];

      foreach (unserialize($data) as $key => $value) {
        $metatags[$key] = array_shift($value);
      }

      return serialize($metatags);
    }
  }

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.

pobster’s picture

Re-rolled (as the first hunk fails to apply with latest dev) and added the menu_ui dependency.

pobster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: metatag-n2563649-66.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pobster’s picture

Maybe 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?

pobster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 69: metatag-n2563649-69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pobster’s picture

Status: Needs work » Needs review
FileSize
22.17 KB

Meh ... Tired ...

Status: Needs review » Needs work

The last submitted patch, 72: metatag-n2563649-70.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Pobtastic’s picture

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

pcranston’s picture

I did this in a way pretty similar to @cruno described in #65, but re this comment:

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

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:

if($key == 'twitter:card'){ $key ='twitter_cards_type'; }
if($key == 'twitter:url'){ $key ='twitter_cards_page_url'; }
if($key == 'twitter:description'){ $key ='twitter_cards_description'; }
if($key == 'twitter:site'){ $key ='twitter_cards_site'; }
if($key == 'twitter:site:id'){ $key ='twitter_cards_site_id'; }
if($key == 'twitter:creator'){ $key ='twitter_cards_creator'; }
if($key == 'twitter:creator:id'){ $key ='twitter_cards_creator_id'; }
if($key == 'twitter:title'){ $key ='twitter_cards_title'; }
if($key == 'canonical'){ $key ='canonical_url'; }
if($key == 'og:url'){ $key ='og_url'; }
if($key == 'og:description'){ $key ='og_description'; }
if($key == 'og:image'){ $key ='og_image'; }
if($key == 'og:image:url'){ $key ='og_image_url'; }
if($key == 'og:image:secure_url'){ $key ='og_image_secure_url'; }
if($key == 'og:image:type'){ $key ='og_image_type'; }
WidgetsBurritos’s picture

Status: Needs work » Needs review
FileSize
23.37 KB

This is simply a reroll of #70 against the latest commit.

Status: Needs review » Needs work

The last submitted patch, 76: metatag-n2563649-76.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

WidgetsBurritos’s picture

Status: Needs work » Needs review
FileSize
23.58 KB
3.42 KB

This patch should resolve the test failures.

a.milkovsky’s picture

In addition to #65. I am processing drupal 7 metatags like this:

  /**
   * Processes metatags data.
   *
   * @param Row $row
   *   The row.
   *
   * @return string|null
   *   The serialized processed metatags, null if empty.
   */
  protected function processMetaTags(Row $row) {
    $data = $row->getSourceProperty('metatags');
    if (!$data) {
      return NULL;
    }
    $metatags = [];
    foreach (unserialize($data) as $metatag_name => $value) {
      $value = reset($value);
      if (is_array($value)) {
        $value = array_filter($value);
        if ($value) {
          $value = implode(', ', array_keys($value));
        }
      }
      if ($value) {
        $metatags[$metatag_name] = $value;
      }
    }

     if (!$metatags) {
      return NULL;
    }

    return serialize($metatags);
  }
DamienMcKenna’s picture

With 8.6.x this throws the following error:

The website encountered an unexpected error. Please try again later.Drupal\migrate\Plugin\Exception\BadPluginDefinitionException: The d7_metatag_field_instance plugin must define the source_module property. in Drupal\migrate_drupal\MigrationPluginManager->processDefinition() (line 104 of core/modules/migrate_drupal/src/MigrationPluginManager.php).

Drupal\migrate\Plugin\MigrationPluginManager->findDefinitions() (Line: 175)
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.
DamienMcKenna’s picture

Status: Needs review » Needs work

FYI even 8.5.x throws this message:

The d7_metatag_field_instance plugin must define the source_module property.

DamienMcKenna’s picture

Does this help? It seems to at least get past that one blocker when I try it locally.

DamienMcKenna’s picture

FYI that helps with both 8.5.x and 8.6.x to at least be able to get to the upgrade review page.

Status: Needs review » Needs work

The last submitted patch, 82: metatag-n2563649-82.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

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

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
22.11 KB
1.18 KB

Might this help?

Status: Needs review » Needs work

The last submitted patch, 86: metatag-n2563649-86.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

benjifisher’s picture

+++ b/migration_templates/d7_metatag_field_instance.yml
@@ -0,0 +1,23 @@
+id: d7_metatag_field_instance
+label: Metatag field instance
+migration_tags:
+  - Drupal 7
+source:
+  plugin: d7_metatag_field_instance
+  source.module: metatag

I think the last line should have source_module rather than source.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.

heddn’s picture

socketwench’s picture

Status: Needs work » Needs review
FileSize
22.08 KB
3.58 KB
  • Renamed source.module to source_module.
  • Moved migration_templates/ to migrations/.

Status: Needs review » Needs work

The last submitted patch, 90: metatag-n2563649-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

  1. +++ b/metatag.module
    @@ -491,40 +498,81 @@ function metatag_entity_diff_options($entity_type) {
    + * Implement hook_migrate_prepare_row().
    

    Nit: should be plural Implements

  2. +++ b/metatag.module
    @@ -491,40 +498,81 @@ function metatag_entity_diff_options($entity_type) {
    +function metatag_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
    +  // @todo Write a more general version rather than hard-coded.
    +  $source_can_have_metatags = in_array($source->getPluginId(), [
    +    'd7_node',
    +    'd7_node_revision',
    +    'd7_taxonomy_term',
    +    'd7_user'
    +  ]);
    

    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

  3. +++ b/metatag.module
    @@ -491,40 +498,81 @@ function metatag_entity_diff_options($entity_type) {
    +  if ($source_can_have_metatags && $migration->getDestinationPlugin() instanceof EntityContentBase) {
    ...
    +      if (is_subclass_of($destination_plugin, $entity_content_base) || $destination_plugin == $entity_content_base) {
    

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

  4. +++ b/metatag.module
    @@ -491,40 +498,81 @@ function metatag_entity_diff_options($entity_type) {
    +    $query = $source->getDatabase()->select('metatag', 'm')
    

    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.

  5. +++ b/metatag.module
    @@ -491,40 +498,81 @@ function metatag_entity_diff_options($entity_type) {
    +      $entity_content_base = 'Drupal\migrate\Plugin\migrate\destination\EntityContentBase';
    

    This should be able to use Class::class syntax.

  6. +++ b/migrations/d7_metatag_field.yml
    @@ -0,0 +1,20 @@
    +  ignore_map: true
    
    +++ b/migrations/d7_metatag_field_instance.yml
    @@ -0,0 +1,23 @@
    +  ignore_map: true
    
    +++ b/migrations/d7_metatag_field_instance_widget_settings.yml
    @@ -0,0 +1,21 @@
    +  ignore_map: true
    

    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.

  7. +++ b/tests/fixtures/drupal7.php
    @@ -0,0 +1,189 @@
    +$connection->schema()->createTable('metatag', array(
    
    +++ b/tests/src/Kernel/Migrate/d7/MigrateMetatagTest.php
    @@ -0,0 +1,106 @@
    +    $this->loadFixture(__DIR__ . '/../../../../fixtures/drupal7.php');
    

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

marvil07’s picture

Status: Needs work » Needs review
FileSize
20.9 KB
1.18 KB

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

Status: Needs review » Needs work

The last submitted patch, 93: 2563649-93-migrate.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marvil07’s picture

Status: Needs work » Needs review
FileSize
21.88 KB
22.38 KB
4.41 KB

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

  • I have added a check on to exclude both 'entity:comment', and 'entity:metatag' from getting the metatag source field, see hook_entity_base_field_info().
  • Also, adding the requirement on 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.
  • I started using $source->getSourceModule(), but I think using class hierarchy can help a bit more. This excludes sources not extending base classes, but that sounds less usual, so I have chosen class hierarchy as suggested on #92
  • Finally, I found some extra preprocessing for d7 entries on prepare row needed, as some already have mentioned on the issue, so I have added that to prepare row, but that can live on a migrate process plugin too.

Update on #92:

  • done: 1, 2, 5
  • todo: 3, 4, 6, 7

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.

The last submitted patch, 95: 2563649-95-8.x-1.x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

  1. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +  // @todo Write a more general version rather than hard-coded.
    

    Can we add a link to the issue that is creating a more general version?

  2. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +  // Support a know subset of d7 sources.
    

    Support a known subset.

  3. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +    $query = $source->getDatabase()->select('metatag', 'm')
    

    This query will fail if source isn't in the conditionals of user, term, or node. Let's check that $entity_type isset.

  4. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +    if (!is_null($revision_id)) {
    

    !empty() should be enough here.

  5. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +          // @fixme Skip these values for now, maybe an some version supported
    +          // these?
    

    Can we get a comment from a module maintainer on this?

  6. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +      if (in_array($migration['destination']['plugin'], ['entity:comment', 'entity:metatag'])) {
    

    This says metatag, but then also lists comment. Can we clarify what we want?

  7. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +      $destination_plugin = DefaultFactory::getPluginClass($migration['destination']['plugin'], $plugin_definition);
    

    The destination plugin manager lets you createInstance as a stub. That's how this is typically done.

  8. +++ b/metatag.module
    @@ -579,3 +589,107 @@ function metatag_generate_entity_metatags($entity) {
    +      if (is_subclass_of($destination_plugin, EntityContentBase::class) || $destination_plugin == EntityContentBase::class) {
    +        $migration['process']['field_metatag'] = 'field_metatag';
    

    If we only support node, user, terms, then we cannot just check on ECB. We need to be more specific.

  9. +++ b/migrations/d7_metatag_field.yml
    @@ -0,0 +1,20 @@
    +id: d7_metatag_field
    
    +++ b/migrations/d7_metatag_field_instance.yml
    @@ -0,0 +1,23 @@
    +id: d7_metatag_field_instance
    
    +++ b/migrations/d7_metatag_field_instance_widget_settings.yml
    @@ -0,0 +1,21 @@
    +id: d7_metatag_field_instance_widget_settings
    

    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.

heddn’s picture

Status: Needs review » Needs work
heddn’s picture

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

$entity_id_key = $this->getKey('id'); // nid
$entity_revision_key = $this->getKey('revision'); // vid

Which will result in the following yaml coming out of processFieldValues():

field_metatag:
  plugin: field_metatag
  source:
    - @nid
    - @vid

And our new FieldPlugin/field_metatag process plugin would then get all the existing logic that we currently have in the prepare_row hook.

PierreRicci’s picture

Hi,

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

benjifisher’s picture

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

  • with git: immediately. See the "Version control" tab on the module's project page.
  • in a dev release: within 24 hours
  • in a regular release: as soon as the module maintainers decide to make the next release.
ccarrascal’s picture

Hi all, I am importing metatags data using a combination of #65 and #79 and it's working fine.

DamienMcKenna’s picture

Title: Migrations: basic entities » Migrations: Metatag basic entities
DamienMcKenna’s picture

Status: Needs review » Needs work

The last submitted patch, 104: metatag-n2563649-104.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

I copied fileMigrationSetup() from FileMigrationSetupTrait in 8.4 because I didn't want to deal with extending getFileMigrationInfo().

Status: Needs review » Needs work

The last submitted patch, 107: metatag-n2563649-107.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

There are two main errors:

1) Drupal\Tests\metatag\Kernel\Migrate\d7\MigrateMetatagTest::testMetatag
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "metatag" entity type does not exist.

The entity type is actually called "metatag_defaults".

Trying to configure method "getHighWater" which cannot be configured because it does not exist, has not been specified, is final, or is static

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.

Status: Needs review » Needs work

The last submitted patch, 109: metatag-n2563649-109.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

The outstanding failure is:

1) Drupal\Tests\metatag\Kernel\Migrate\d7\MigrateMetatagTest::testMetatag
Migration d7_metatag_field did not meet the requirements. The module metatag is not enabled in the source site. source_module: metatag.
Failed asserting that false is true.

heddn’s picture

  1. +++ b/metatag.module
    @@ -603,3 +611,103 @@ function metatag_generate_entity_metatags($entity) {
    +function metatag_migrate_prepare_row(Row $row, MigrateSourceInterface $source, MigrationInterface $migration) {
    

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

  2. +++ b/metatag.module
    @@ -603,3 +611,103 @@ function metatag_generate_entity_metatags($entity) {
    +      if (is_subclass_of($destination_plugin, EntityContentBase::class) || $destination_plugin == EntityContentBase::class) {
    +        $migration['process']['field_metatag'] = 'field_metatag';
    

    Use a field plugin. This shouldn't be necessary.

  3. +++ b/src/Plugin/migrate/source/d7/MetatagField.php
    @@ -0,0 +1,42 @@
    +  public function getIds() {
    +    $ids['entity_type']['type'] = 'string';
    +    return $ids;
    +  }
    
    +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
    @@ -0,0 +1,95 @@
    +  public function getIds() {
    +    $ids['entity_type']['type'] = 'string';
    +    $ids['bundle']['type'] = 'string';
    +    return $ids;
    +  }
    

    Is there any langcode we need to account for here?

  4. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
    @@ -0,0 +1,95 @@
    +      foreach (array_keys($bundle_info) as $bundle) {
    +        $bundles[] = [
    +          'entity_type' => $instance['entity_type'],
    +          'bundle' => $bundle
    +        ];
    +      }
    

    Would a generator work better than a foreach loop?

  5. +++ b/src/Plugin/migrate/source/d7/MetatagFieldInstance.php
    @@ -0,0 +1,95 @@
    diff --git a/tests/fixtures/drupal7.php b/tests/fixtures/drupal7.php
    

    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.

  6. +++ b/tests/src/Kernel/Migrate/d7/MigrateMetatagTest.php
    @@ -0,0 +1,131 @@
    +    $this->loadFixture(__DIR__ . '/../../../../fixtures/drupal7.php');
    

    This assumes a certain folder structure. That could be fragile on the testbots.

heddn’s picture

Or rather than 112.1, use a field plugin.

DamienMcKenna’s picture

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

heddn’s picture

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

heddn’s picture

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

DamienMcKenna’s picture

chx provided an alternative approach to this:

https://gist.github.com/chx/232ba2d2bb32ceb2eef68045d54bb53b

chefigueroa’s picture

I 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!

DamienMcKenna’s picture

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

DamienMcKenna’s picture

DamienMcKenna’s picture

Issue tags: +migrate-d7-d8
DamienMcKenna’s picture

Title: Migrations: Metatag basic entities » Migrations: Metatag-D7 basic entities
marcelovani’s picture

I 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

      if ($source->getDatabase()->schema()->fieldExists('metatag', 'revision_id')) {
        $query->condition('revision_id', $revision_id);
      }

I am putting a path together and interdiff. Also updated the Readme to help people that are new to migrations.

DamienMcKenna’s picture

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

floown’s picture

Hello,

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

DamienMcKenna’s picture

@floown: no, we haven't finished the migration part yet for Metatag, so using the dev version of Metatag won't make any difference.

DamienMcKenna’s picture

From marvil07 in Slack:

Something like `field_meta_tags: field_metatag` should work.
That new field comes from
+ $row->setSourceProperty('field_metatag', serialize($metatag));

Status: Needs review » Needs work

The last submitted patch, 128: metatag-n2563649-128.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
26.87 KB
753 bytes

Let's see if this fixes the problem with the StreamWrapperInterface namespace.

Status: Needs review » Needs work

The last submitted patch, 130: metatag-n2563649-130.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
26.91 KB
494 bytes

This time maybe?

Status: Needs review » Needs work

The last submitted patch, 132: metatag-n2563649-132.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
26.93 KB
498 bytes

The current error is:

Migration d7_node_revision:test_content_type did not meet the requirements. Missing migrations d7_node:blog. requirements: d7_node:blog.

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()?

Status: Needs review » Needs work

The last submitted patch, 134: metatag-n2563649-134.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Ok, now we're getting to the actual tests for this change:

1) Drupal\Tests\metatag\Kernel\Migrate\d7\MigrateMetatagTest::testMetatag
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'a:1:{s:8:"keywords";a:1:{s:5:"value";s:7:"keynode";}}'
+'a:1:{s:8:"keywords";s:7:"keynode";}'

/var/www/html/modules/contrib/metatag/tests/src/Kernel/Migrate/d7/MigrateMetatagTest.php:114
DamienMcKenna’s picture

So somehow the array is being changed from:

[
    'keywords' => [
        'value' => 'keynode',
    ]
]

to:

[
    'keywords' => 'keynode',
]
PapaGrande’s picture

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

      $metatags = $row->getSourceProperty('field_metatag');
      if ($metatags) {
        $metatags = unserialize($metatags);
        if (isset($metatags['og:image'])) {
          $metatags['og_image'] = $metatags['og:image'];
        }
        if (isset($metatags['twitter:image'])) {
          $metatags['twitter_cards_image'] = $metatags['twitter:image'];
        }
        $row->setSourceProperty('field_metatag', serialize($metatags));
      }

Also, for others' reference, the migration YML is simple:

  field_meta_tags: field_metatag
DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
26.93 KB

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

DamienMcKenna’s picture

DamienMcKenna’s picture

Some coding standards improvements.

DamienMcKenna’s picture

Another few minor tweaks.

DamienMcKenna’s picture

Status: Needs review » Needs work

So, the last part is working out how to convert the D7 meta tag names to D8's.

drupalninja99’s picture

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

DamienMcKenna’s picture

Issue summary: View changes

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

DamienMcKenna’s picture

DamienMcKenna’s picture

Issue summary: View changes
oliverpolden’s picture

Updated patch to handle canonical renamed to canonical_url.

DamienMcKenna’s picture

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

oliverpolden’s picture

I've updated this to make it more generic but there may be a better place to put the array.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
17.94 KB
39.57 KB

Ok, 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.

DamienMcKenna’s picture

Status: Needs review » Needs work
oliverpolden’s picture

Thanks @damienmckenna. Interdiff looks good to me so far.

DamienMcKenna’s picture

This should cover all meta tags currently present in D8, any tag not available yet is listed as a @todo item.

Status: Needs review » Needs work

The last submitted patch, 154: metatag-n2563649-154.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
756 bytes
45.22 KB

Doh. Lets try this again.

Status: Needs review » Needs work

The last submitted patch, 156: metatag-n2563649-156.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
45.22 KB
1.99 KB

A second doh - variable name typoo.

DamienMcKenna’s picture

Woohoo! Tests are green!

heddn’s picture

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

  1. +++ b/metatag.module
    @@ -614,3 +623,125 @@ function metatag_generate_entity_metatags($entity) {
    +  if (is_a($source, Node::class)) {
    ...
    +  if ($migration->getDestinationPlugin() instanceof EntityContentBase) {
    ...
    +      case 'node':
    ...
    +      case 'taxonomy':
    ...
    +      case 'user':
    ...
    +    $tags_map = MigrateHelper::mapForMetatagD7();
    

    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?

  2. +++ b/tests/src/Unit/Migrate/d7/MetatagFieldInstanceTest.php
    @@ -0,0 +1,87 @@
    +  const PLUGIN_CLASS = 'Drupal\metatag\Plugin\migrate\source\d7\MetatagFieldInstance';
    ...
    +    $module_handler = $this->getMock('Drupal\Core\Extension\ModuleHandlerInterface');
    +    $state = $this->getMock('Drupal\Core\State\StateInterface');
    +    $entity_manager = $this->getMock('Drupal\Core\Entity\EntityManagerInterface');
    +    $entity_type_bundle_info = $this->getMockBuilder('Drupal\Core\Entity\EntityTypeBundleInfo')
    

    Can't this use Class:class syntax?

DamienMcKenna’s picture

Some minor improvements to the README.txt file and the pseudo field has been renamed to "pseudo_field_metatag".

DamienMcKenna’s picture

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

heddn’s picture

Here'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.

DamienMcKenna’s picture

Assigned: Unassigned » DamienMcKenna
Status: Needs review » Needs work

Something was accidentally broken with the last patch(es) and in a manual test none of the meta data was migrated.

oliverpolden’s picture

Assigned: DamienMcKenna » Unassigned
Status: Needs work » Needs review

I'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:

Error: Call to undefined method Drupal\migrate_plus\Plugin\migrate\source\Url::getDatabase() in metatag_migrate_prepare_row() (line 628 of /var/www/html/****/web/modules/contrib/metatag/metatag.module).

which refers to:

  if (!$source->getDatabase()->schema()->tableExists('metatag')) {
    return;
  }

Also, perhaps it would be good for the readme to refer to field_metatag instead of field_metatags for the time being.

oliverpolden’s picture

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

$processed_value = PlainTextOutput::renderFromHtml(htmlspecialchars_decode($this->tokenService->replace($tag->value(), $token_replacements, ['langcode' => $langcode])));
DamienMcKenna’s picture

Ok, 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().

DamienMcKenna’s picture

Issue summary: View changes
Status: Needs review » Needs work

I've noted the two remaining tasks.

DamienMcKenna’s picture

Issue summary: View changes

Three remaining tasks.

DamienMcKenna’s picture

Issue summary: View changes

Another change that we need to do is move the custom data shuffling to a process plugin.

DamienMcKenna’s picture

Assigned: Unassigned » DamienMcKenna

I'm working on the process plugin, will finish it later today.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
36.96 KB
46.05 KB

Let's see how this works - I moved the custom conversion logic into a generic process plugin.

DamienMcKenna’s picture

Assigned: DamienMcKenna » Unassigned

Status: Needs review » Needs work

The last submitted patch, 174: metatag-n2563649-174.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
521 bytes
46.08 KB

Typoo, "process" instead of "plugin".

Status: Needs review » Needs work

The last submitted patch, 176: metatag-n2563649-176.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

The tests run locally, but no data is migrated when I test it manually. Not sure what's going on here.

DamienMcKenna’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

oliverpolden’s picture

@damienmckenna Have you checked what you get in the database when you migrate vs save a node in D8?

DamienMcKenna’s picture

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

oliverpolden’s picture

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

  1. The migration needs to be updated to remove the "value" element
  2. D8 needs to be updated to have a "value" element

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.

DamienMcKenna’s picture

Issue summary: View changes

So #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.

heddn’s picture

so, 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 and hook_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.

DamienMcKenna’s picture

Issue summary: View changes

Thanks heddn, that's good news on the architectural recommendation.

I'll look into the dependencies.

DamienMcKenna’s picture

Would something like this work?

      if (is_subclass_of($destination_plugin, EntityContentBase::class) || $destination_plugin == EntityContentBase::class) {
        ..
        $migration['migration_dependencies']['optional'][] = 'd7_metatag_field_instance';
      }
heddn’s picture

#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).

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
47.81 KB
3.86 KB

I tried adding 'migration_dependencies' items but it didn't make any difference.

heddn’s picture

+++ b/metatag.module
@@ -704,20 +704,61 @@ function metatag_migrate_prepare_row(Row $row, MigrateSourceInterface $source, M
+      if (in_array($migration['destination']['plugin'], $destinations_to_ignore)) {

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

DamienMcKenna’s picture

Updated the migration to no longer nest values, so migrated values should be correct again.

DamienMcKenna’s picture

And now support for array-based tags, like "robots".

DamienMcKenna’s picture

I nuked my local D8 testbed and the migration works again via migrate_upgrade.

DamienMcKenna’s picture

Issue summary: View changes

Ok, 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?

oliverpolden’s picture

Status: Needs review » Needs work

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

DamienMcKenna’s picture

Status: Needs work » Needs review

There are two different issues here.

  1. If you're doing a guided migration using either migrate_upgrade or migrate_drupal_ui the code in metatag_migration_plugins_alter() will create a field named "field_metatag" for you.
  2. If you're doing a custom migration you follow the details from the README.txt file and it should work because of the code in metatag_migrate_prepare_row().

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.

DamienMcKenna’s picture

Improved documentation in the README.txt and at the top of metatag_migration_plugins_alter().

DamienMcKenna’s picture

I renamed pieces of the migration to make room to also migrate the configuration later, the entity migration pieces now have "entities" in their names.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

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

  1. +++ b/metatag.module
    @@ -614,3 +623,154 @@ function metatag_generate_entity_metatags($entity) {
    +  // @todo Document how to change the field name.
    

    This todo seems completed w/ the latest readme doc updates.

  2. +++ b/src/Plugin/migrate/process/d7/MetatagD7Entities.php
    @@ -0,0 +1,425 @@
    +    try {
    +      $old_tags = unserialize($value);
    +    }
    +    catch (\Exception $e) {
    

    I don't see that unserialize throws an exception.

DamienMcKenna’s picture

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

DamienMcKenna’s picture

Forgot to remove $e->getMessage().

heddn’s picture

Great! Let's wait on the bot now.

DamienMcKenna’s picture

I renamed the fixtures and tests to separate them from other tests we'll add for other migrations.

DamienMcKenna’s picture

DamienMcKenna’s picture

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

DamienMcKenna’s picture

Sorry for the noise, folks. I renamed the hook and made some minor coding standards improvements.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

And it's committed! Woohoo, thanks everyone!

floown’s picture

OMG, we can now import metatags from D7 to D8? All it's ok?

oliverpolden’s picture

Can 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

DamienMcKenna’s picture

@oliverpolden: That's excellent, thanks for letting us know.

Status: Fixed » Closed (fixed)

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

John_B’s picture

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

DamienMcKenna’s picture

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

John_B’s picture