Missing mapping for "nodereference_url" widget

Is there any changes on the field side or only on the instance? And needs tests by adding a field of this type this to the test fixture, so back to NW.

Hi, I just ran into the situation for a Drupal 6 to Drupal 8 Migration, the error still exists. Did anyone already test if #6 leads to a working migration? I'll try it.

Does someone know a test file as similar example to use to write tests for this? I don't know where to begin properly...

@Anybody, thanks for asking!

For tests, data needs to be added to the Drupal 6 test fixture and then assertions added to \Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldTest and \Drupal\Tests\field\Kernel\Migrate\d6\MigrateFieldWidgetSettingsTest. There are some notes for working with the fixtures at https://www.drupal.org/docs/8/api/migrate-api/generating-database-fixtur....

If you have any questions asking in Drupal slack, #migration.

@quietone: Thank you! As this was still on my list, but I currently don't have the time, I asked my teammate @Grevil, if he could have a look. He'll do his very best to help and might eventually contact you / #migrate users on Slack. Not that easy to get into this for the first time...
Thank you!

@quietone: Just had a look at the Drupal 6 test fixture with @Grevil and I think we have a problem:
nodereference_url was a widely used Drupal 6 contrib module (discontinued) which would still be good to support for migration import in core.
But we'd have to install the module in Drupal 6 for the test fixture export to be clean, and I don't think that would make sense and would be a high price to pay? I don't think we want a contrib module as further Drupal 6 general dependency?
Especially because we map that old field / widget to a core default now...

Of course, we could also fake add the entries to the test fixture without installing the module, but I think that's even worse...

So no idea how to proceed here! Any strategy?

Ok, I created an issue fork integrating the relevant fixture entries and tests. Since @Anybody mentioned in #14, that nodereference_url is just a contrib module and not mentioned in the contrib module list under https://www.drupal.org/docs/drupal-apis/migrate-api/generating-database-..., I created the fixture entries manually because I wouldn't want to export the d6 fixture with an "unauthorized" contrib module installed. While creating the manual entries, I have not created table entries for "locales_source", "i18n_strings", "menu_links" or "menu_router".

Note: The relevant commit is in the branch "3000717-missing-mapping"

@Grevil: Thanks! If these two commits are the right ones: https://git.drupalcode.org/project/drupal/-/merge_requests/2571/commits (top 2), then I'd suggest to
- Rebase that branch / MR or replay the changes again on the latest version, if it's easier.
- Close the other MR: MR!2572 (I'll do that now).

Afterwards, set to "Needs review". :)

Relevant changes (2 commits) from MR!2571 attached as patches if we should have trouble with rebasing! :)

I created a new branch "nodereference-url-missing-mapping-3000717", on base of 9.5.x and applied the fixture changes and patch #6.

@quietone, @heddn, maybe you can have a look if everything seems fine?

The tests are failing so I'm going to throw this one back to NW.

If anyone needs the patch from #6 for 9.4.x, here it is.

Sorry, unnecessary patch, it did not apply locally because of other patches applied locally...

Great, I found the left issue with the failing test. We have to increment the changed config count as we added field_company_4!

Wohoo, tests are green :) @quietone, @heddn would you mind taking a look from outside?

Great work @edysmp & @Grevil!

I am starting to review this issue. If it is still assigned to me a week from now, then feel free to un-assign it or re-assign it.

Near the changes in the upgrade test, I see these lines:

      // The 'standard' profile provides the 'comment' comment type, and the
      // migration creates 12 comment types, one per node type.
      'comment_type' => 14,
// ...
      // The 'book' module provides the 'book' node type, and the migration
      // creates 12 node types.
      'node_type' => 14,

The comments are from 2017, and the lines that follow the comments are from 2021: #2565931: Handle long comment bundle names. It looks as though the comments need to be updated.

In order to avoid merge conflicts, we could add the unrelated comment update to the MR for this issue or we could create a follow-up issue once this one is fixed.

      // The 'standard' profile provides the 'comment' comment type, and the
      // migration creates 12 comment types, one per node type.
      'comment_type' => 14,

Thanks @benjifisher, I'm not sure if I got you right, but I think that comment just explains why the following line has a return value of 14 and is not related to any other lines?

So I guess this should stay as it is?

@Anybody: Sorry, I may have confused things by mentioning the comments. Fixing the comment is not in scope for this issue. We might do it anyway as part of this issue because fixing the comment separately could cause a merge conflict.

According to the comment, the number of comment types should be 1 + 12 or 13. That is what it was before #2565931: Handle long comment bundle names.

More relevant to this issue is a suggestion on Slack that maybe we do not have to add tests for this field type after all, despite what @heddn said in #5. The full transcript will be on #3318916: [meeting] Migrate Meeting 2022-11-10 2100Z soon, but for now here is part of what @quietone wrote:

I am inclined to argue that tests are not needing for this. We are only adding strings to a mapping, all the code doing the mapping and converting the field is well tested.

[I am] Looking for [precedents].

Here is a similar one for a field instance, #3314134: Add i18n_taxonomy_term_reference_plain to TaxonomyTermReference

Also, changing the Drupal 7 fixture might cause tests in contrib to fail.

Also, I asked,

Can you help explain the point of this issue?

It looks as though the D6 nodereference module provides two different field types. They both represent node references, and they are both mapped to an entity-reference field.

Do the two fields have the same data structure and only differ in the widget?

It looks as though the nodereference_select field type is already supported, and this issue adds support for nodereference_url.

Two more questions:

Has anyone reported manual testing of this patch?

Does this patch handle both the config migration and the content migration?

I have reviewed this, it has taken a while to remember a few things about Drupal 6 fields and migrate.

I read the code and ran the tests and everything looks fine. But something was bothering me and then I remembered about \Drupal\Tests\migrate_drupal\Kernel\d6\FieldDiscoveryTest and wondered why it wasn't changed. testAddAllFieldProcesses should include the new field and addAllFieldProcessesAltersData the new widget type.

The reason testAddAllFieldProcesses passes is that it does not include a count of all the fields, that is minor and was probably avoided some noise early in the development of the Migrate Drupal. addAllFieldProcessesAltersData passes because the mapping has been placed in the migration yaml instead of in \Drupal\migrate_drupal\Plugin\migrate\field\ReferenceBase::getFieldWidgetMap.

Then I reread the summary and a light bulb went off, this widget is provided by a Drupal 6 module that is new to the migration system. I have no objection to this being added but more changes are needed. The system table in the source fixture needs to change to include the module, a migration state entry is needed for the module, and various functional tests will need to be updated.

So, yes, tests are needed here.

Does this patch handle both the config migration and the content migration?

Not directly. It is assumed that it is handled correctly by the FieldMigration and the getField* methods in \Drupal\node\Plugin\migrate\source\d6\Node.

I decided to make the changes myself. I was wrong about adding changes to ReferenceBase. I assumed that userreference_url was also supplied but it is not. Silly assumption given the name of the Drupal 6 module.

Thank you so much @quietone, also, for all this helpful additional information.
And of course for working on this and creating the patch. It's really hard to get into all the details.

Then I reread the summary and a light bulb went off, this widget is provided by a Drupal 6 module that is new to the migration system.

Indeed, see the issue summary, but a very common one, later replaced bay entity_reference.

Patch looks good, we'll also test it internally, currently the tests are waiting for the branch to pass.

Thank you so much once again!

From the interdiff in #40:

--- a/core/modules/field/migrations/state/field.migrate_drupal.yml
+++ b/core/modules/field/migrations/state/field.migrate_drupal.yml
@@ -2,6 +2,7 @@
 finished:
   6:
     nodereference: core
+    nodereference_url: core
     userreference: core
     content: field
     email: core

I think this should add the nodereference_url to the list of modules that will be upgrade on /upgrade/review. It would be nice to take a screenshot while doing manual testing and add it to this issue.

I don't think manual tested is needed. As far as I recall, we have not added screenshots of the placement of the Module name on the review page since the initiate commits of that feature. The process to add the Module name is well tested and the patch includes a test of the Review Page.

This needed a reroll.

In regard to #42, 'Node Reference URL Widget', is added to the list of modules that will be upgraded and it is tested.

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MultilingualReviewPageTest.php
@@ -101,6 +101,7 @@ protected function getAvailablePaths() {
+      'Node Reference URL Widget',

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/NoMultilingualReviewPageTest.php
@@ -93,6 +93,7 @@ protected function getAvailablePaths() {
+      'Node Reference URL Widget',

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
@@ -150,6 +150,7 @@ protected function getAvailablePaths() {
+      'Node Reference URL Widget',

To keep this moving I created a fresh D6 site, downloaded CCK and NodeReferenceUrl, and created content with a field that uses the url node reference widget. And here is a screenshot of upgrading that site from /upgrade

Thanks @quietone, know we briefly spoke in slack about this one in the #migrate channel.

Change looks good and test coverage is there so think it's good.

Test failure on

1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton
Failed asserting that a NULL is not empty.

Retesting.

Restarted the tests but that’s one of the random ones we see now

Random failure.

Crediting @benjifisher

Committed to 10.1.x, needs re-roll for 10.0.x and 9.5.x so marking it as patch (to be ported).

Added patch for 9.5.x

Tried fixing test fail

Committed/pushed/cherry-picked the backport to 10.0.x and 9.5.x, thanks!