Problem/Motivation

Drupal 8 link fields store route name and parameters and options. Drupal 6 link fields store paths. Drupal 8 link fields, at this moment, does not have a working conversion between paths and route name/parameters.

Proposed resolution

There are proposed solutions in core #2235457: Use link field for shortcut entity but this issue circumvents the complexities of that issue to allow users to begin testing migrations.

The menu link migration added the Route process plugin and an example of using it. We need to use this same circuitous way because the link field can store that. The challenge is to coax the entity load plugin to use the new CckLink process plugin which is based off the current Route process plugin.

Remaining tasks

User interface changes

API changes

Original report by @ultimike

Based on my not-so-good results of larger and more complex D6 databases, I figured that I'd start with something really simple - a fresh D6 site with limited content types and fields.

It seems like there is an issue with link fields, as while the data appears to be migrated (I can see it in the D8 database), there is still something odd going on as the view and edit pages for the nodes return WSOD. Could be related to https://drupal.org/node/2233901, as multiple migrations didn't run due to requirement issues.

I'm planning on re-running this test once https://drupal.org/node/2233901 is resolved, or after I add some new revisions to my D6 nodes.

Here's what I did:

  1. Enabled the "Content" and "Link" modules in D6.
  2. Added a new link field to the "story" content type in D6, added content.
  3. Enabled the "Link" module in D8 (disabled by default).
  4. Ran the migration (using https://drupal.org/comment/8615887#comment-8615887): nodes migrated, link field created, link content not migrated, with errors:
    ~/Sites/imp $  drush migrate-manifest mysql://imp@localhost/drupal6 manifest.yml</li>
      <li>Migration d6_cck_field_revision:page did not meet the requirements                                                                                                              [error]</li>
      <li>Migration d6_cck_field_revision:story did not meet the requirements                                                                                                             [error]</li>
      <li>Migration d6_cck_field_values:page did not meet the requirements                                                                                                                [error]</li>
      <li>Migration d6_cck_field_values:story did not meet the requirements 
  5. Based on migration_dependencies listed in the the .yml files for d6_cck_field_revision and d6_cck_field_values, I added the following migrations to the manifest:
    d6_node_revision
    d6_field_formatter_settings
  6. Ran the migration: nodes migrated, link field created, link content migrated (I can see it in the DB), but with errors:
    ~/Sites/imp $  drush migrate-manifest mysql://imp@localhost/drupal6 manifest.yml
    Migration d6_cck_field_revision:page did not meet the requirements                                                                                                              [error]</li>
    Migration d6_cck_field_revision:story did not meet the requirements 
  7. Then, when I go to the D8 site in a browser, both the "view" and "edit" pages for the newly migrated nodes have fatal errors (http://note.io/1lwAAUH and http://note.io/1dZePNj).

-mike

Files: 
CommentFileSizeAuthor
#65 interdiff.txt605 bytesbenjy
#65 2233883-65.patch14.52 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,805 pass(es). View
#63 interdiff.txt547 bytesbenjy
#63 2233883-63.patch14.46 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,755 pass(es). View
#62 interdiff-58-62.txt996 byteshussainweb
#62 2233883-62.patch14.5 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,755 pass(es). View
#58 interdiff-54-58.txt1.8 KBhussainweb
#58 2233883-58.patch14.49 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,752 pass(es), 2 fail(s), and 0 exception(s). View
#54 interdiff.txt1.44 KBbenjy
#54 2233883-54.patch14.53 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,547 pass(es). View
#51 interdiff.txt2.03 KBbenjy
#51 2233883-51.patch14.34 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,552 pass(es). View
#48 interdiff.txt1.73 KBbenjy
#48 2233883-48.patch14.11 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,598 pass(es), 2 fail(s), and 0 exception(s). View
#44 interdiff.txt831 bytesbenjy
#44 2233883-44.patch14.4 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,597 pass(es), 2 fail(s), and 0 exception(s). View
#39 interdiff.txt1.58 KBbenjy
#39 2233883-39-FAIL.patch817 bytesbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,586 pass(es), 2 fail(s), and 0 exception(s). View
#39 2233883-39.patch13.59 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,604 pass(es), 2 fail(s), and 0 exception(s). View
#36 interdiff.txt4.43 KBbenjy
#36 2233883-36.patch12.01 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,545 pass(es). View
#31 interdiff.txt779 bytesbenjy
#31 2233883-31.patch12.13 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,498 pass(es). View
#29 interdiff.txt4.95 KBbenjy
#29 2233883-28.patch12.89 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,500 pass(es), 2 fail(s), and 0 exception(s). View
#27 interdiff.txt5.91 KBbenjy
#27 2233883-27.patch10.44 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,510 pass(es), 16 fail(s), and 0 exception(s). View
#23 interdiff.txt2.68 KBbenjy
#23 2233883-23.patch8.33 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,360 pass(es), 1 fail(s), and 0 exception(s). View
#22 2233883-22.patch8.55 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,345 pass(es), 1 fail(s), and 0 exception(s). View
#20 2233883-20.patch7.12 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,351 pass(es), 2 fail(s), and 1 exception(s). View
#2 localhost_drupal6.sql_.gz127.47 KBultimike

Comments

ultimike’s picture

Title: Simple D6->D8 Node Migration test » Simple D6->D8 Node Migration test - Link field issue?
Issue summary: View changes
ultimike’s picture

FileSize
127.47 KB

Attaching source (D6) database. UID 1 user/pass: admin/admin

-mike

ultimike’s picture

I just pulled a fresh copy of the imp sandbox and retried the same test - the situation has taken one step forward and one step back.

First, the forward step - the "did not meet the requirements" errors no longer appear when running the manifest, so that's good.

But, after I run the manifest, the /node and migrated node pages are WSOD. Screenshot of the error dump (they're all similar): http://note.io/1lOrfdy

Debugging the issue, I found that in the D8 (destination) database, in the node__field_url (this is my field of type "Link") table, the field_url_options was null for imported links. When I created a link through the UI on D8, the field_url_options was populated with a serialized array:

a:3:{s:5:"query";a:0:{}s:8:"fragment";s:0:"";s:10:"attributes";a:0:{}}

Manually populating the migrated node__field_url record with this data and rebuilding the cache allows the node to be rendered in D8. This proves (at least to me) that this is the issue.

It appears to me that perhaps the D6 link migration should be initializing this value as such? I'm happy to work on a patch for this, but where do I start? Is this type of thing handled in the Destination class? Maybe migrate/lib/Drupal/migrate/Plugin/migrate/destination/???

-mike

ultimike’s picture

Pulled latest code, re-ran test, same results.

Digging in a little deeper into this issue, it looks like that when a link is created in D8, the /Drupal/Link/Plugin/Field/FieldWidget/LinkWidget massageFormValues() function creates the options and adds them to the url structure.

When we're migrating link fields in, the massageFormValues() function never runs (obviously), so the options never get set, and therefore the field is set to null, leading to the WSOD.

If all this is correct (big if), then I would think that we need a mechanism to look at the incoming link data and create options, if necessary. Unfortunately, I'm not sure the best way to do this. It seems like this would be more of a Migrate thing rather than a Migrate Drupal thing? I'm happy to do the leg work on this one, I just need some direction...

Thanks,
-mike

chx’s picture

Title: Simple D6->D8 Node Migration test - Link field issue? » Field values sometimes need per field type processing
Priority: Normal » Critical

Currently

process:
  nid:
    plugin: migration
    migration: d6_node
    source: nid
  field_foo: field_foo
  field_bar: field_bar

But it probably needs to look like:

process:
  nid:
    plugin: migration
    migration: d6_node
    source: nid
  field_foo:
    plugin: d6_cck_link
    source: field_foo

And so change LoadEntity to get the process plugin manager injected ( implement ContainerFactoryPluginInterface ) and then use it to check for the existence of d6_cck_$field_type and if so then use it. Also, write the plugin for link field.

There seems to be a core bug lurking here as well: if $node->some_link_field->options is not set then $node->save() fails.

ultimike’s picture

Following up on chx's previous comment,

  1. There could potentially be a core bug that causes the WSOD when a link field's option value is null. I'm going to write a test to demonstrate this.
  2. As chx explained, It looks like we're going to have to write some per-field-type migration process plugins to fix this for this and other field types.

-mike

ultimike’s picture

chx,

In IRC you mentioned:

09:38 chx: ultimike: if you can verify that (please create a node type / add a link field / write a 3 line script to save a node like that and run with drush php-script) then it's a core bug yes
09:38 chx: ultimike: and the example script does double duty as test :)

Should I be creating the node type and link field manually then writing some PHP that calls $node->save()? Your whole "3 line script" is throwing me off - give me a hint?

Thanks,
-mike

chx’s picture

Please see NodeLastChangedTest for a DrupalUnitTestc creating nodes.

ultimike’s picture

I'm working on modifying the \Drupal\link\Tests\LinkItemTest to test specifically for the case when a link doesn't have any query/attribute/fragment options. In my initial go at it, the test still passed - the entity was loaded.

chx gave me some guidance and I modified the initial test as such:

public function testLinkItem() {
    // Create entity.
    $entity = entity_create('entity_test');
    $url = 'http://www.drupal.org?test_param=test_value';
    $parsed_url = UrlHelper::parse($url);
    $title = $this->randomName();
    $class = $this->randomName();
    $entity->field_test->url = $parsed_url['path'];
    $entity->field_test->title = $title;
    //$entity->field_test->first()->get('options')->set('query', $parsed_url['query']);
    //$entity->field_test->first()->get('options')->set('attributes', array('class' => $class));
    $entity->name->value = $this->randomName();
    $entity->save();
    $id = $entity->id();
    $entity = entity_load('entity_test', $id);
    $this->assertTrue($entity, 'Saved entity loads successfully.');

At this point, I'm thinking that the WSOD I saw (see comment 3 above) happens after the entity is loaded (maybe during rendering?), but I want to debug and inspect the $entity variable in the above code before I do anything else.

Unfortunately, I've never tried to debug a Simpletest before. Debugging from the browser doesn't seem doable (I think Batch API gets in the way?) and I'm still trying to figure out how to set up phpStorm to debug core/scripts/run-tests.sh

I found this article about setting up a Run/Debug configuration in phpStorm, but I haven't been able to get it to work yet...

-mike

ultimike’s picture

Okey dokey, some progress...

I figured out what the issue was with being able to debug Simpletests using PhpStorm and xdebug. Turns out the version of xdebug (2.2.0) that came installed with my stack (MAMP Pro 2.1.1) was buggy, after I updated xdebug to 2.2.3, all was well.

Based on chx's suggestion, I created a new Link module test: LinkFieldItemNoOptions - it is virtually identical to the LinkFieldItem test, but with no link options (fragment, attributes, query) set or tested for. Running the test results in no fails (bummer?!) I think I need to figure out how to write a test that renders an entity with no options to see if I can get it to fail (like my comment 3 above).

As chx mentions above (comment 5), we may have to add new process plugins for fieldtypes to handle these types of situations.

Thanks,
-mike

ultimike’s picture

chx’s picture

Thinking more of it: there is already a per field type mechanism already -- it's the entity_field plugins. They aren't used yet but I think the simplest would be to use those. See ContentEntityBase::import .

ultimike’s picture

I just tested a core patch that appears to solve this issue: https://drupal.org/comment/8703857#comment-8703857

Here's what I did:

1. Fresh D8 site.
2. Applied patch: https://drupal.org/comment/8698261#comment-8698261
3. Ran the migration (as outlined in the issue summary).
4. Received no "did not meet the requirements" errors.
5. Content imported cleanly.
6. Was able to view rendered links on node pages with no issues.

At this point, I suppose that this issue can be closed as soon as https://drupal.org/node/2235457 is committed.

-mike

chx’s picture

I commented on the issue that seems to solve this which is #2235457: Use link field for shortcut entity. Thanks!

ultimike’s picture

Project: IMP » Drupal core
Version: » 8.x-dev
Component: Code » migration system
Priority: Critical » Normal
Related issues: +#2235457: Use link field for shortcut entity
chx’s picture

Title: Field values sometimes need per field type processing » Link migration is broken
Anonymous’s picture

This remains broken. The two possible patches cannot be rerolled to PSR-4. There is currently no pathway to get this working using the existing patches.

chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
benjy’s picture

Status: Active » Needs review
FileSize
7.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,351 pass(es), 2 fail(s), and 1 exception(s). View

Lets try get this started.

Status: Needs review » Needs work

The last submitted patch, 20: 2233883-20.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,345 pass(es), 1 fail(s), and 0 exception(s). View

Forgot to add the new file.

benjy’s picture

FileSize
8.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,360 pass(es), 1 fail(s), and 0 exception(s). View
2.68 KB

Better, yet. Use FieldStorageConfig

The last submitted patch, 22: 2233883-22.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2233883-23.patch, failed testing.

chx’s picture

I guess the fail is the field storage / field config entity itself not migrating.

That said, I am OK with the approach. It's migrate_drupal not migrate so we can restrict the super generic to something to be usable. That's why we moved over the load entity plugin too.

benjy’s picture

Status: Needs work » Needs review
FileSize
10.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,510 pass(es), 16 fail(s), and 0 exception(s). View
5.91 KB

The problem is that the loading of the migrations happens before any of the migrations actually run. Current patch fixes that issue but I think there still could be a problem in the dumps related to nodes and node revisions. See what the bot says.

Status: Needs review » Needs work

The last submitted patch, 27: 2233883-27.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
12.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,500 pass(es), 2 fail(s), and 0 exception(s). View
4.95 KB

Should fix the individual tests, let see what MigrateDrupal6Test says.

Status: Needs review » Needs work

The last submitted patch, 29: 2233883-28.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
12.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,498 pass(es). View
779 bytes

Reverted the dump code I uncommented by accident in #29.

chx’s picture

Status: Needs review » Reviewed & tested by the community

And there was much rejoicing.

benjy’s picture

Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes
benjy’s picture

Title: Link migration is broken » Link migration needs to convert source url into the appropriate route format for storage
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal/src/Plugin/CckFieldMigrateSourceInterface.php
    @@ -0,0 +1,16 @@
    +<?php
    +
    +namespace Drupal\migrate_drupal\Plugin;
    

    Missing @file

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/load/LoadEntity.php
    @@ -15,6 +15,8 @@
    +use Drupal\field\Entity\FieldStorageConfig;
    

    Not used.

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/CckLink.php
    @@ -0,0 +1,36 @@
    +    // @TODO, WTF, D6 double serializes the attributes?
    

    @todo what? this should describe something to do and not just vent.

  4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/CckFieldValues.php
    @@ -264,6 +265,16 @@ public function fields() {
    diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    
    diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    index 807ee43..0155674 100644
    
    index 807ee43..0155674 100644
    --- a/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    
    --- a/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ProfileFieldValues.php
    @@ -12,7 +12,6 @@
    
    @@ -12,7 +12,6 @@
     use Drupal\migrate\Plugin\SourceEntityInterface;
     use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
     
    -
     /**
      * Drupal 6 profile fields values source.
      *
    

    Unrelated change

  5. +++ b/core/modules/migrate_drupal/src/Tests/Dump/Drupal6Node.php
    @@ -535,6 +555,45 @@ public function load() {
    +        'field_test_link_attributes' => 's:6:"a:0:{}";',
    ...
    +        'field_test_link_attributes' => 's:6:"a:0:{}";',
    ...
    +        'field_test_link_attributes' => 's:6:"a:0:{}";',
    

    Shouldn't we test migrating a field with link attributes?

benjy’s picture

Status: Needs work » Needs review
FileSize
12.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,545 pass(es). View
4.43 KB

Thanks for the review, all feedback addressed. Plus fixed a bug with link options/attributes and added support for "external".

Sorry about the TODO, I was genuinely meant to go see why D6 does that. I'm still not entirely sure why but I did confirm, they're stored in the database in D6 double serialized.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Round #2.

benjy’s picture

Status: Reviewed & tested by the community » Needs work

I tested this tonight with a real migration and it still doesn't seem to work. Not sure what is going on at this point. Setting back to NW until I figure it out.

benjy’s picture

Status: Needs work » Needs review
FileSize
13.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,604 pass(es), 2 fail(s), and 0 exception(s). View
817 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,586 pass(es), 2 fail(s), and 0 exception(s). View
1.58 KB

OK, I tracked the issue down to link fields with default values. They were being creating during the node migration and we needed to initialise $link['options']['attributes'] in FieldInstanceDefaults.

Attached is a patch that shows the problem with a test only patch, the interdiff and the final patch with the fix. The default settings don't seem to get stored in the field instance table in D6 so right now they're just initialised empty. Minor follow-up IMO.

The last submitted patch, 39: 2233883-39.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 39: 2233883-39-FAIL.patch, failed testing.

Gábor Hojtsy’s picture

The patch fails with Schema errors for field.field.node.story.field_test_link with the following errors: field.field.node.story.field_test_link:default_value.0.options missing schema If you look at how field.field is defined in schema in field.schema.yml:

field.field.*.*.*:
  type: field_config_base
  label: 'Field'

Then you'll look at field_config_base (in core.data_types.schema.yml):

field_config_base:
  type: config_entity
  mapping:
    [....]
    default_value:
      type: sequence
      label: 'Default values'
      sequence:
        - type: field.value.[%parent.%parent.field_type]
          label: 'Default value'
    [....]

I quoted the default value only because you have problems with that. So the default value is a sequence of options defined by the field_type on the top of the file (two levels up). So the fail may be one of a few:

1. the field_type is missing at the top of the file
2. the field_type is not what it should be (not the type that has an options key)
3. the options key should not be there because it is not supported by the (correctly specified) field type
4. the field type default value schema may be incorrect

Looking at link.schema.yml, field.value.link only supports a title and a url key, no options key. Unless this is incorrect, then 3. seems to be the problem.

Hope this helps.

Gábor Hojtsy’s picture

Reviewed this with @benjy in IRC. Looks like the schema is incorrect because there is an options key supported on the defualt items:

  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {

    $default_url_value = NULL;
    if (isset($items[$delta]->url)) {
      if ($url = \Drupal::pathValidator()->getUrlIfValid($items[$delta]->url)) {
        $url->setOptions($items[$delta]->options);
        $default_url_value = ltrim($url->toString(), '/');
      }
    }

So the default value options key should support all options supported by Url::setOptions() (documented on the constructor).

benjy’s picture

Status: Needs work » Needs review
FileSize
14.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,597 pass(es), 2 fail(s), and 0 exception(s). View
831 bytes

Added schema.

Gábor Hojtsy’s picture

+++ b/core/modules/link/config/schema/link.schema.yml
@@ -60,3 +60,22 @@ field.value.link:
+        language:
+          type: string
+          label: 'The language'

Erm that is certainly not a string at least in PHP, its an object. Is it persisted as a string in config?!

benjy’s picture

Status: Needs review » Needs work

No it is an object, i'll take a look later and try to figure out what the schema should look like.

The last submitted patch, 44: 2233883-44.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
14.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,598 pass(es), 2 fail(s), and 0 exception(s). View
1.73 KB

Removed attributes and external.

Gábor Hojtsy’s picture

As for attributes, LinkWidget has this:

    $element['attributes'] = array(
      '#type' => 'value',
      '#tree' => TRUE,
      '#value' => !empty($items[$delta]->options['attributes']) ? $items[$delta]->options['attributes'] : array(),
      '#attributes' => array('class' => array('link-field-widget-attributes')),
    );

I think that it ALSO passes in 'attributes' as an option to Url is incorrect, right? Not that the default value may not have an attributes key, it may have. The typing of that is undefined though and left for others? How is this supposed to work?

As for the external flag, Url uses that internally in setUnrouted() so I guess it is not supposed to be set anymore? That means a link field will not be able to set an external URL anymore? Is that not what the link field is supposed to be about?

Status: Needs review » Needs work

The last submitted patch, 48: 2233883-48.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
14.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,552 pass(es). View
2.03 KB

OK, brought attributes back and added schema. Interdiff is based on #44.

chx’s picture

Assigned: Unassigned » Gábor Hojtsy

I like it but I'll let Gabor RTBC it.

dawehner’s picture

  1. +++ b/core/modules/migrate_drupal/src/Plugin/CckFieldMigrateSourceInterface.php
    @@ -0,0 +1,21 @@
    +
    +interface CckFieldMigrateSourceInterface extends MigrateSourceInterface {
    

    At least have a oneliner would be nice.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/load/LoadEntity.php
    @@ -82,8 +83,31 @@ public function loadMultiple(EntityStorageInterface $storage, array $sub_ids = N
    +        $source_plugin = $migration->getSourcePlugin();
    +        if ($source_plugin instanceof CckFieldMigrateSourceInterface) {
    +          foreach ($source_plugin->fieldData() as $field_name => $data) {
    +            // Specifically process the link field until core is fixed.
    +            // @see https://www.drupal.org/node/2235457
    +            if ($data['type'] == 'link') {
    +              $migration->process[$field_name] = [
    +                'plugin' => 'd6_cck_link',
    +                'source' => [
    +                  $field_name,
    +                  $field_name . '_title',
    +                  $field_name . '_attributes',
    +                ],
    +              ];
    +            }
    +            else {
    +              $migration->process[$field_name] = $field_name;
    +            }
    

    @benjy ensured that on the longrun we don't have to hardcode that in the LoadEntity class

benjy’s picture

FileSize
14.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,547 pass(es). View
1.44 KB

Thanks for the review, feedback addressed. I've created #2395993: Clean up Loadentity a little for the per field type processing discussion.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/link/config/schema/link.schema.yml
@@ -60,3 +60,27 @@ field.value.link:
+        language:
+          type: string
+          label: 'The language'

Still not true. Same as I said in #45.

dawehner’s picture

@Gábor Hojtsy
Ah interesting ... well I guess it used to be a string in D6. For D8 we have #2363459: Make it possible to provide just $options['langcode'] for URL generation instead of full language object to make it at least possible to just specify the string.
I don't even have a clue how you could insert the language in the UI?

hussainweb’s picture

FileSize
14.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,752 pass(es), 2 fail(s), and 0 exception(s). View
1.8 KB

I am trying out two things from my review of the patch.

  1. +++ b/core/modules/link/config/schema/link.schema.yml
    @@ -60,3 +60,27 @@ field.value.link:
    +        query:
    +          type: string
    +          label: 'Url query string'
    

    This is marked as string, but the documentation says this should be an array. We are not really testing this either. I have modified one of the test cases to check for this. If it passes, we should probably duplicate the test.

  2. +++ b/core/modules/link/config/schema/link.schema.yml
    @@ -60,3 +60,27 @@ field.value.link:
    +        language:
    +          type: string
    +          label: 'The language'
    

    I checked the link-6.x module and different code paths in migrate but found no reference to language. I know it could be used later on in path processor. But I am curious as to what happens if we remove this.

hussainweb’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

I think its sort of fine to remove the language element for now, if we assume nobody will set it. Its much better than specifying an invalid value. Once somebody tries to set it at least in a test, they will get fails. Ideally we would figure out which keys are stored there and only allow those to be persisted (ie. filter based on the keys expected). Otherwise this may end up in hard to debug type mismatches in PHP for the options keys due to invalid config. That is not really the task of this issue though.

Status: Needs review » Needs work

The last submitted patch, 58: 2233883-58.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
14.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,755 pass(es). View
996 bytes

Forgot to update the test.

benjy’s picture

FileSize
14.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,755 pass(es). View
547 bytes

@hussainweb, I don't see how adding that query string is offering any extra coverage? The only way query is used as part of the migration is for internal url's in the Route process plugin but this is an external url.

New patch just removes the language schema stuff as per #60, interdiff is based on my last patch in #54.

hussainweb’s picture

@benjy: The reason I am testing for query strings is because query is an array. UrlHelper::parse is the function responsible for parsing URL's (both external and internal). You can see from the code (as well as documentation) that $options['query'] is an array. This function is called from PathValidator::getUrl() and you can see that $options['query'] does get assigned here.

However, in our new config, we are setting query as type: string. I wanted to test if actually testing a query part makes it fail. Apparently, it doesn't. It might be serialized before storage but I am not sure about that.

benjy’s picture

FileSize
14.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,805 pass(es). View
605 bytes

PathValidator::getUrl() does parse the query out but it's never used because of isExternal().

    elseif (UrlHelper::isExternal($path) && UrlHelper::isValid($path)) {
      if (empty($parsed_url['path'])) {
        return FALSE;
      }
      return Url::fromUri($path);
    }

Adding a query to the data and testing for that here really makes no difference. The full url is saved when the link field is saved, the validator ignores the options when loading in LinkWidget because the url is external. If we really want to test how $options['query'] works, we'll need to add a relative url to the dumps and then a new test.

That said, the schema should as you said, indicate that query is an array not a string, new patch attached. Thanks for the review.

Status: Needs review » Needs work

The last submitted patch, 65: 2233883-65.patch, failed testing.

benjy queued 65: 2233883-65.patch for re-testing.

benjy’s picture

Status: Needs work » Needs review
chx’s picture

Status: Needs review » Reviewed & tested by the community

I think all the concerns have been addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Migrate issues are excluded from the beta evaluation as long as they don't mess with the rest of core. Opened #2397233: Link module has untested features to store additional configuration per link to address the missing test coverage in the Link module found by this issue. Committed a5f0e6a and pushed to 8.0.x. Thanks!

  • alexpott committed a5f0e6a on 8.0.x
    Issue #2233883 by benjy, hussainweb, ultimike: Link migration needs to...

Status: Fixed » Closed (fixed)

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