Link migration needs to convert source url into the appropriate route format for storage

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.

Please see NodeLastChangedTest for a DrupalUnitTestc creating nodes.

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 .

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

Lets try get this started.

Forgot to add the new file.

Better, yet. Use FieldStorageConfig

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.

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.

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

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

And there was much rejoicing.

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?

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.

Round #2.

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.

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

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

Added schema.

+++ 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?!

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

Removed attributes and external.

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?

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

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

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

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

Thank you

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

@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 'langcode' in $options for URL generation instead of 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?

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.

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.

Forgot to update the test.

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

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

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.

I think all the concerns have been addressed.

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!