Problem/Motivation

A full migration test of Ubercart 7 to Commerce using the UI (commerce_migrate) is failing because d7_rdf_mapping reports errors if the rdf mapping already exists. Reporting errors if the entity exists doesn't happen in other migrations and let's not do it here.

$ drush mim d7_rdf_mapping
 [error]  'rdf_mapping' entity with ID 'node.article' already exists. (/opt/sites/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:462)
 [error]  'rdf_mapping' entity with ID 'node.page' already exists. (/opt/sites/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:462)
 [notice] Processed 2 items (0 created, 0 updated, 2 failed, 0 ignored) in 1.8 seconds (66.8/min) - done with 'd7_rdf_mapping'

Tracked this to Entity::getEntity() where the entity_id is obtained from the row and then the entity is checked to see if it exists. If it does not exist it will be attempt to create storage and I think that is where the error occurs. Suffice it to save that the entity id should be on the row for getEntity() to check if it exists.

When that was fixed I was getting the following error on loading an node of type article.

1) Drupal\Tests\migrate_drupal_ui\Functional\d7\MigrateUpgrade7Test::testMigrateUpgradeExecute
Exception: Warning: date() expects parameter 2 to be integer, array given
date_iso8601()() (Line: 343)

Investigating that led to the discovery that RDF uses a different method for handling dates not, date_iso8601, which is what is migrated. That need to be changed to Drupal\rdf\CommonDataConverter::dateIso8601Value.

Proposed resolution

Add an id property to the d7_rdf_mapping so that Entity::getEntity() has an id value to use to check if the entity already exists.
Add a static_map to convert the callback 'date_iso8601' to 'Drupal\rdf\CommonDataConverter::dateIso8601Value'.

Remaining tasks

Patch, there is a fail patch as well.
Review
Commit

CommentFileSizeAuthor
#12 interdiff.txt4.1 KBquietone
#12 2998666-12.patch5.92 KBquietone
#10 interdiff.txt662 bytesquietone
#10 2998666-10.patch2.17 KBquietone
#3 2998666-3.patch1.16 KBquietone
#3 2998666-3-fail.patch712 bytesquietone
#2 2998666-2.patch477 bytesquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs work
Issue tags: +Needs tests
StatusFileSize
new 2998666-2.patch477 bytes

Using this patch the contrib test passes. Deliberately not running the testbot, wait until there is a test.

quietone’s picture

quietone commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2998666-3-fail.patch712 bytes
new 2998666-3.patch1.16 KB

Added a test and a fail test. After the rdf migration test is run the migrate map is destroyed and then the rdf migration is run again to show that it works with pre existing rdf mappings.

No interdiff because these are small files.

The last submitted patch, 3: 2998666-3-fail.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 3: 2998666-3.patch, failed testing. View results

quietone’s picture

quietone commented

Not sure why the patch is failing on

1) Drupal\Tests\migrate_drupal_ui\Functional\d7\MigrateUpgrade7Test::testMigrateUpgradeExecute
Exception: Warning: date() expects parameter 2 to be integer, array given
date_iso8601()() (Line: 343)

Anyone have an idea?

quietone’s picture

quietone commented
Priority: Normal » Major

I came across this post about getting warnings after upgrading
https://www.drupal.org/forum/support/upgrading-drupal/2018-09-18/warning...

which is the same error. Changing to Major.

The workaround it most likely to remove the rdf mappings from the site configuration. At least that is what worked locally in my test environment.

edit: the same error meaning the same error as the test for the patch in #3

quietone’s picture

quietone commented
Title: Add id to d7_rdf_mapping migration » Warnings after D7 upgrade caused by rdf migration

Improve the title

quietone’s picture

quietone commented

I've been trying to figure this out tonight.

By searching I found this https://www.drupal.org/project/drupal/issues/1784234 which gave some context and background. And since I don't know RDF, that was helpful. That got me to look at the mappings in core, specifically core/profiles/standard/config/install/rdf.mapping.node.article.yml because I'm testing with article type.

It was obvious straight away that the rdf.mapping.node.article is using a different callback from the migrated one for the date fields. And that callback, CommonDataConverter, expects the input to be an array, whereas date_iso8601 expects a string which is what is causing the warning.

I made a test by just editing the article mapping to change the date callbacks and then didn't get any warnings when loading articles. No time to make a patch, tonight.

datatype_callback:
callable: date_iso8601

needs to be changed to 

datatype_callback:
callable: 'Drupal\rdf\CommonDataConverter::dateIso8601Value'
quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2998666-10.patch2.17 KB
new interdiff.txt662 bytes

This adds a static_map in the process to accomplish the suggestions in #9. Locally, that fixed the error.

Status: Needs review » Needs work

The last submitted patch, 10: 2998666-10.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2998666-12.patch5.92 KB
new interdiff.txt4.1 KB

Hah, I completely forgot to update the rdf migation test.

quietone’s picture

quietone commented
Issue summary: View changes
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/rdf/tests/src/Kernel/Migrate/d7/MigrateRdfMappingTest.php
@@ -107,7 +107,7 @@ public function testRdfMappingMigration() {
-            'callable' => 'date_iso8601',
+            'callable' => 'Drupal\rdf\CommonDataConverter::dateIso8601Value',

So, I applied the patch and after doing that, things are lot more clear. The yaml looking here is not from a migration yaml. It is the configuration of rdf. And it actually does expect a class name in the configuration. And that configuration should be this updated format.

And with that and passing tests, this can be RTBC.

The last submitted patch, 2: 2998666-2.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 68773eb705 to 8.7.x and 201bcb1007 to 8.6.x. Thanks!

  • catch committed 68773eb on 8.7.x 
    Issue #2998666 by quietone, heddn: Warnings after D7 upgrade caused by...

  • catch committed 201bcb1 on 8.6.x 
    Issue #2998666 by quietone, heddn: Warnings after D7 upgrade caused by...

Status: Fixed » Closed (fixed)

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