Problem/Motivation

Entities created through the UI are validated (required fields confirmed to be present, field values confirmed to be in proper ranges, etc.) through form validation - the entity API (save()) does not do validation, so if migrated data is to be validated it needs to be done explicitly. As benjy says:

I think we've discussed this elsewhere but no, entity->save() doesn't cause validation, you have to do $entity->validate() for that which opens up a can of worms with migrate.

Still, no reason not to permit those who choose to do so to open that can...

Proposed resolution

Add a "validate" option to entity destinations - when true, call $entity->validate().

destination:
  plugin: entity:node
  validate: true

Remaining tasks

  1. Write the code (should take about 2 minutes).
  2. Write tests (slightly longer).

User interface changes

N/A

API changes

Adds a configuration option to entity destinations.

Data model changes

None.

Release notes snippet

The ability to validate content entities during a migration is available. On the destination just set validation: true. See https://www.drupal.org/node/3073707 for more info.

CommentFileSizeAuthor
#81 interdiff-2745797-79-81.txt665 bytesBR0kEN
#81 2745797-81.patch14.3 KBBR0kEN
#79 interdiff-2745797-75-79.txt2.08 KBBR0kEN
#79 2745797-79.patch14.3 KBBR0kEN
#75 interdiff-2745797-73-75.txt9.93 KBBR0kEN
#75 2745797-75.patch13.93 KBBR0kEN
#73 2745797-73.patch12.17 KBBR0kEN
#73 interdiff-2745797-66-73.txt3.18 KBBR0kEN
#72 2745797-72--8.6.x.patch12.14 KBBR0kEN
#72 interdiff-2745797-67-72.txt3.17 KBBR0kEN
#67 interdiff-2745797-64-67.txt6.01 KBBR0kEN
#67 2745797-67--8.6.x.patch11.33 KBBR0kEN
#66 interdiff-2745797-62-66.txt6.01 KBBR0kEN
#66 2745797-66.patch11.36 KBBR0kEN
#64 2745797-64--8.6.x.patch10.15 KBBR0kEN
#62 interdiff-2745797-59-62.txt2.16 KBBR0kEN
#62 2745797-62.patch10.17 KBBR0kEN
#59 interdiff-2745797-58-59.txt3.79 KBBR0kEN
#59 interdiff-2745797-42-59.txt12.27 KBBR0kEN
#59 2745797-59.patch10.19 KBBR0kEN
#58 interdiff-2745797-42-58.txt16.09 KBBR0kEN
#58 2745797-58.patch14.11 KBBR0kEN
#55 interdiff-2745797-49-55.txt9.46 KBBR0kEN
#55 2745797-55.patch13.53 KBBR0kEN
#49 interdiff-2745797-42-49.txt10.37 KBBR0kEN
#49 2745797-49.patch12.8 KBBR0kEN
#42 interdiff-39-42.txt3.59 KBjofitz
#42 2745797-42.patch8.18 KBjofitz
#39 interdiff-35-39.txt6.24 KBpiggito
#39 add_option_to_content-2745797-39.patch7.24 KBpiggito
#35 add_option_to_content-2745797-35.patch7.47 KBpiggito
#35 interdiff_30-35.txt1.12 KBpiggito
#30 add_option_to_entity-2745797-30.patch7.34 KBrenaudcuny
#22 interdiff-2745797-14-19.txt802 bytesbiguzis
#19 add_option_to_entity-2745797-19.patch7.36 KBbiguzis
#14 interdiff_9-14.txt3.14 KBheddn
#14 add_option_to_entity-2745797-14.patch7.43 KBheddn
#9 2745797-9.patch8.15 KBharings_rob
#5 2745797-5.patch1.51 KBharings_rob
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance commented
Issue tags: +Novice
harings_rob’s picture

harings_rob CreditAttribution: harings_rob commented
Assigned: Unassigned » harings_rob

I am working on this.

harings_rob’s picture

harings_rob CreditAttribution: harings_rob commented
FileSize
2745797-5.patch1.51 KB

Hi Mikeryan,

Attached is a "pre patch", could you let me know if this is the correct approach?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

You also have to do something with the returned data of the validation.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

re #4, that is the right direction. But you need to throw a row exception if the validation fails.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance commented
Version: 8.3.x-dev » 8.2.x-dev
Status: Active » Needs work
Issue tags: +Needs tests 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -106,11 +106,24 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+      $entity->validate();

    As dawehner points out, we need to handle failed validations. I'm thinking probably the best thing to do if the returned violation list is non-empty would be to throw an exception - we'll need a new exception class, MigrateEntityViolationException or somesuch, with the violation list as an argument. MigrateExecutable would catch this and save any messages found to the migration message table via $this->saveMessage(). See ContentEntityForm::flagViolations() to see how to iterate the violation list. Actually, you could basically copy this to MigrateExecutable, replace the setErrorByName call with saveMessage(), and remove the getFormDisplay() call. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -106,11 +106,24 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+   *   True if validation is needed, false otherwise.

    TRUE and FALSE should be all-caps.

harings_rob’s picture

harings_rob CreditAttribution: harings_rob commented
FileSize
2745797-9.patch8.15 KB

Attached is the full patch including the tests.

I am not sure about the exception class as I have never done this before.

Please let me know where I can improve.

Regards,

harings_rob’s picture

harings_rob CreditAttribution: harings_rob commented
Status: Needs work » Needs review

The last submitted patch, 5: 2745797-5.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Status: Needs review » Needs work
Issue tags: -Needs tests

The meat of it looks good, some cleanup and rearrangement suggestions below. I've tested locally, and it works well in practice.

  1. +++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,40 @@
+namespace Drupal\migrate\Exception;

    I'm not sure why RequirementsException (which I presume you were looking at) is under an Exception directory - other migrate exceptions, and exceptions defined in other core modules, are directly in src, so I would move this up to the Drupal\migrate namespace. 

  2. +++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,40 @@
+class MigrateEntityViolationException extends MigrateException {

    I'm going back and forth on whether this should extend MigrateException as is done here (so it will get caught without any other changes), or should extend \Exception and MigrateExecutable should explicitly catch this exception. I'm leaning towards keeping it this way, but I can be easily swayed if others disagree... 

  3. +++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,40 @@
+   * @param int $code
+   *   The Exception code.
+   * @param \Exception $previous
+   *   The previous exception used for the exception chaining.
+   * @param int $level
+   *   The level of the error, a Migration::MESSAGE_* constant.
+   * @param int $status
+   *   The status of the item for the map table, a MigrateMap::STATUS_*
+   *   constant.
...
+  public function __construct(EntityConstraintViolationListInterface $violations, $code = 0, \Exception $previous = NULL, $level = MigrationInterface::MESSAGE_ERROR, $status = MigrateIdMapInterface::STATUS_FAILED) {

    But, since we're never going to pass these parameters, there's no reason to have them - we can let the MigrateException constructor default them. 

  4. +++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,40 @@
+    foreach ($violations as $violation) {

    Shouldn't that be $violations->getEntityViolations() as it is in ContentEntityForm::flagViolations()? Well, I guess the fact that it's \Traversable means it will work... I'll see when I test locally. Later - yes, works just fine for reals, so this is OK. 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -99,15 +100,42 @@ public function import(Row $row, array $old_destination_id_values = array()) {
-   * @param array $old_destination_id_values
-   *   (optional) An array of destination ID values. Defaults to an empty array.
...
-  protected function save(ContentEntityInterface $entity, array $old_destination_id_values = array()) {

    True, there's no point to this parameter, but fixing that is out of scope for this issue. 

  6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -99,15 +100,42 @@ public function import(Row $row, array $old_destination_id_values = array()) {
-    return array($entity->id());
+    return [$entity->id()];

    Similarly, coding standard fixes are out of scope (I believe there's an existing core issue to update to short array throughout core via automation). 

  7. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -99,15 +100,42 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+   *   True if validation is needed, false otherwise.

    s/True/TRUE/, s/false/FALSE/

I'm prepared to RTBC with the above issues addressed, thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I'm not sure why RequirementsException (which I presume you were looking at) is under an Exception directory - other migrate exceptions, and exceptions defined in other core modules, are directly in src, so I would move this up to the Drupal\migrate namespace.

Just a quick comment. We are kind of trying to not make the mistake again to define new exceptions inside the main root namespace. Collection exceptions in its own namespace IMHO removes clutter for example.

+++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,40 @@
+  public function __construct(EntityConstraintViolationListInterface $violations, $code = 0, \Exception $previous = NULL, $level = MigrationInterface::MESSAGE_ERROR, $status = MigrateIdMapInterface::STATUS_FAILED) {
+    $messages = [];
+
+    foreach ($violations as $violation) {
+      /** @var \Symfony\Component\Validator\ConstraintViolationInterface $violation */
+      $messages[] = $violation->getMessage();
+    }
+    parent::__construct(implode(', ', $messages), $code, $previous, $level, $status);

This is loosing some information, for example which violation message applied to which field of an entity. Maybe we should extract that information as well, or at least store the entire list of violations?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs work » Needs review
FileSize
add_option_to_entity-2745797-14.patch7.43 KB
interdiff_9-14.txt3.14 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Here's the result of some testing. We need to handle this a little more gracefully, or no one is going to change the validate flag. If I disable validate, then I get zero errors/warnings. If I enable it, dies right away and I have to run it with '--no-halt-on-error'.

$  drush  mi upgrade_d7_node_event --update --force --no-halt-on-error
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could not be converted to string                                [error]
MigrateEntityViolationException.php:26
Error: Call to a member function getValue() on null in                                                                           [error]
Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator->validate() (line 18 of
/var/www/drupalvm/web/core/modules/file/src/Plugin/Validation/Constraint/FileValidationConstraintValidator.php) #0
/var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(185):
Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator->validate(Object(Drupal\image\Plugin\Field\FieldType\ImageItem),
Object(Drupal\file\Plugin\Validation\Constraint\FileValidationConstraint))
#1 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(140):
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object(Drupal\image\Plugin\Field\FieldType\ImageItem),
'00000000151fcfa...', Array)
#2 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(147):
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\image\Plugin\Field\FieldType\ImageItem))
#3 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(147):
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\file\Plugin\Field\FieldType\FileFieldItemList))
#4 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(99):
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter),
Array, true)
#5 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php(90):
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter),
NULL, NULL)
#6 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/TypedData.php(135):
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter))
#7 /var/www/drupalvm/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php(373): Drupal\Core\TypedData\TypedData->validate()
#8 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(137):
Drupal\Core\Entity\ContentEntityBase->validate()
#9 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(111):
Drupal\migrate\Plugin\migrate\destination\EntityContentBase->validateEntity(Object(Drupal\node\Entity\Node))
#10 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(91):
Drupal\migrate\Plugin\migrate\destination\EntityContentBase->save(Object(Drupal\node\Entity\Node), Array)
#11 /var/www/drupalvm/web/core/modules/migrate/src/MigrateExecutable.php(239):
Drupal\migrate\Plugin\migrate\destination\EntityContentBase->import(Object(Drupal\migrate\Row), Array)
#12 /var/www/drupalvm/vendor/drush/drush/includes/drush.inc(720): Drupal\migrate\MigrateExecutable->import()
#13 /var/www/drupalvm/vendor/drush/drush/includes/drush.inc(711): drush_call_user_func_array(Array, Array)
#14 /var/www/drupalvm/web/modules/contrib/migrate_tools/migrate_tools.drush.inc(280): drush_op(Array)
#15 [internal function]: _drush_migrate_tools_execute_migration(Object(Drupal\migrate\Plugin\Migration),
'upgrade_d7_node...', Array)
#16 /var/www/drupalvm/web/modules/contrib/migrate_tools/migrate_tools.drush.inc(246): array_walk(Array, '_drush_migrate_...',
Array)
#20 /var/www/drupalvm/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#21 /var/www/drupalvm/vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#22 /var/www/drupalvm/vendor/drush/drush/drush.php(12): drush_main()
#23 {main}.
Error: Call to a member function getValue() on null in /var/www/drupalvm/web/core/modules/file/src/Plugin/Validation/Constraint/FileValidationConstraintValidator.php on line 18 #0 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(185): Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator->validate(Object(Drupal\image\Plugin\Field\FieldType\ImageItem), Object(Drupal\file\Plugin\Validation\Constraint\FileValidationConstraint))
#1 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(140): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object(Drupal\image\Plugin\Field\FieldType\ImageItem), '00000000151fcfa...', Array)
#2 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(147): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\image\Plugin\Field\FieldType\ImageItem))
#3 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(147): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\file\Plugin\Field\FieldType\FileFieldItemList))
#4 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(99): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter), Array, true)
#5 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php(90): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter), NULL, NULL)
#6 /var/www/drupalvm/web/core/lib/Drupal/Core/TypedData/TypedData.php(135): Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object(Drupal\Core\Entity\Plugin\DataType\EntityAdapter))
#7 /var/www/drupalvm/web/core/lib/Drupal/Core/Entity/ContentEntityBase.php(373): Drupal\Core\TypedData\TypedData->validate()
#8 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(137): Drupal\Core\Entity\ContentEntityBase->validate()
#9 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(111): Drupal\migrate\Plugin\migrate\destination\EntityContentBase->validateEntity(Object(Drupal\node\Entity\Node))
#10 /var/www/drupalvm/web/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php(91): Drupal\migrate\Plugin\migrate\destination\EntityContentBase->save(Object(Drupal\node\Entity\Node), Array)
#11 /var/www/drupalvm/web/core/modules/migrate/src/MigrateExecutable.php(239): Drupal\migrate\Plugin\migrate\destination\EntityContentBase->import(Object(Drupal\migrate\Row), Array)
#12 /var/www/drupalvm/vendor/drush/drush/includes/drush.inc(720): Drupal\migrate\MigrateExecutable->import()
#13 /var/www/drupalvm/vendor/drush/drush/includes/drush.inc(711): drush_call_user_func_array(Array, Array)
#14 /var/www/drupalvm/web/modules/contrib/migrate_tools/migrate_tools.drush.inc(280): drush_op(Array)
#15 [internal function]: _drush_migrate_tools_execute_migration(Object(Drupal\migrate\Plugin\Migration), 'upgrade_d7_node...', Array)
#16 /var/www/drupalvm/web/modules/contrib/migrate_tools/migrate_tools.drush.inc(246): array_walk(Array, '_drush_migrate_...', Array)
#17 /var/www/drupalvm/vendor/drush/drush/includes/command.inc(371): drush_migrate_tools_migrate_import('upgrade_d7_node...')
#18 /var/www/drupalvm/vendor/drush/drush/includes/command.inc(222): _drush_invoke_hooks(Array, Array)
#19 /var/www/drupalvm/vendor/drush/drush/includes/command.inc(190): drush_command('upgrade_d7_node...')
#20 /var/www/drupalvm/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(67): drush_dispatch(Array)
#21 /var/www/drupalvm/vendor/drush/drush/includes/preflight.inc(66): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#22 /var/www/drupalvm/vendor/drush/drush/drush.php(12): drush_main()
#23 {main}
Error: Call to a member function getValue() on null in Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator->validate() (line 18 of /var/www/drupalvm/web/core/modules/file/src/Plugin/Validation/Constraint/FileValidationConstraintValidator.php).

Status: Needs review » Needs work

The last submitted patch, 14: add_option_to_entity-2745797-14.patch, failed testing.

iMiksu’s picture

iMiksu
Location Finland
CreditAttribution: iMiksu at Wunder commented
Issue tags: +DCampBaltics

Lets try to investigate why tests failed.

biguzis’s picture

biguzis CreditAttribution: biguzis at Wunder commented
Assigned: harings_rob » biguzis
biguzis’s picture

biguzis CreditAttribution: biguzis at Wunder commented
Status: Needs work » Needs review
FileSize
add_option_to_entity-2745797-19.patch7.36 KB

In MigrateEntityViolationException.php changed line #26 from $messages[] = $violation->getRoot() . ' : ' . $violation->getPropertyPath() . ' : ' . $violation->getMessage(); to $messages[] = $violation->getMessage();

biguzis’s picture

biguzis CreditAttribution: biguzis at Wunder commented
Assigned: biguzis » Unassigned
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Can we get a real simple manual migration where we turn on validation? In my example in #15, I was migrating from D6 so there was very likely some bad data. It would be nice to see a manual test to see how validation works in real life. What errors get thrown, etc.

And we also should add some tests. And an interdiff on #19 is always good.

biguzis’s picture

biguzis CreditAttribution: biguzis at Wunder commented
FileSize
interdiff-2745797-14-19.txt802 bytes

Interdiff for #19

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Status: Needs review » Needs work

Manually tested, hacking migrate_example (setting field_migrate_example_country to required and removing the country data for one row), I get the following in the message table:

This value should not be null.

So, that brings us back to dawehner's earlier comment:

This is losing some information, for example which violation message applied to which field of an entity.

Unfortunately, getEntityViolations() (as well as the iterator over $violations) does not return field-specific information. It looks like to be able to include field names in the messages, we need to iterate over getFieldNames(), calling getByField() for each name (see EntityConstraintViolationListInterface).

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue tags: -Novice

At this point, probably not as novice. Removing tag. @biguzis, feel free to continue working on it though. Don't get scared away.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Title: Add option to entity destinations for validation » Add option to content entity destinations for validation

Re-titling to reflect this only support content entities, not config.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

> you have to do $entity->validate() for that which opens up a can of worms with migrate.

Could someone explain more what the can of worms is?

If you don't validate, then either:

- a) your migration will crash, due to data being too long / wrong format / not in the allowed values. That's the same overall effect as validating, in that you have to deal with the problem before you can complete the migration, but not as nice DX.
- b) you migration runs, but you have created bad data in your site. So you've just postponed the problem of cleaning it up.

I don't see why this needs to be an option -- or at the very least, validation should be enabled by default.

joachim’s picture

joachim CreditAttribution: joachim commented
Issue tags: +DX (Developer Experience) 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -106,11 +107,40 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+    return isset($this->configuration['validate']) ? TRUE : FALSE;

I don't think that's quite right. With this, ANY value in the 'validation' property will cause validation. So these all would:

validate: no
validate: FALSE
validate: 0
validate: kittens

The new configuration item should be documented at the top of the class too. (The others aren't, but there's an issue to document process plugins, so destination plugins should start documenting their options too.)

I tried this patch with my own Drupal6-Drupal migration. As it happens, I have a custom process plugin which populates a boolean field.

So I tried setting that to return something that would break it: 'cake'.

This happened:

Object of class Drupal\Core\Entity\Plugin\DataType\EntityAdapter could[error]
not be converted to string MigrateEntityViolationException.php:26
E_RECOVERABLE_ERROR encountered; aborting. To ignore recoverable      [error]
errors, run again with --no-halt-on-error

Breaking that line apart to see which method on the exception returns something non-stringy, it's:

$violation->getRoot() . ' : ' .

I'm going to add: I really really think this should be classed as a bug, not a feature. As I've just been reminded by intentionally crashing my migration, when this happens you have to go through the steps of resetting it and possibly rolling it back. That's not a graceful failure.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Re #26: "Can of worms", I think stem from the experience of running several D6/D7 => D8 migrations for clients. I always do spot checking of data. Even going as far as looking at the last 10-20 most recent posts of a various pieces of content. Issues arise when there is a malformed malformed D6 piece of data that didn't work in D6, but no one knew (or cared). In many cases, D6 doesn't have the same level of data validation that D8 does. And typically we are dealing with very old, legacy link data or youtube videos. Most of my clients don't care or have the budget to fix all the data.

If there is a DB level problem where the data get's truncated or won't fit into the DB, those still get logged as errors and should get fixed. But if the format for the youtu.be link is spelled youtub.e, that wouldn't get fixed and in many cases it is easier to fix in the new system of D8, where validation will throw errors when re-saving the content. But most clients don't have the money, time, resources to fix up their broken data. So it just stays broken.

BTW, I'm all for adding it as an option. But I'm not sure how often I'll turn it on.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

renaudcuny’s picture

renaudcuny CreditAttribution: renaudcuny at OSCE: Organization for Security and Co-operation in Europe commented
FileSize
add_option_to_entity-2745797-30.patch7.34 KB

Re-rolling patch for 8.3.x.

renaudcuny’s picture

renaudcuny CreditAttribution: renaudcuny at OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Needs review
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Assigned: Unassigned » mikeryan
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan as a volunteer and at Virtuoso Performance commented
Assigned: mikeryan » Unassigned
Status: Needs review » Needs work

My comments in #2745797-23: Add option to content entity destinations for validation and joachim's in #2745797-27: Add option to content entity destinations for validation still need to be addressed.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

piggito’s picture

piggito CreditAttribution: piggito as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff_30-35.txt1.12 KB
add_option_to_content-2745797-35.patch7.47 KB

/modules/migrate/src/Plugin/migrate/destination/EntityRevision.php overrides save function so I think we should call entity validation outside of save function in order to target EntityRevisions (and EntityReferenceRevisions) aswell.
I'm providing a patch just applying this change over the patch in #30.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.4.x-dev » 8.5.x-dev
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work
Issue tags: +Vienna2017

Shouldn't we have some type of interface changes for needsValidation and validateEntity on MigrateDestinationInterface. But we cannot do that without breaking BC. But then we cannot be sure that all things are validable. Maybe that's OK. And we only care about validating content entities.

  1. +++ b/core/modules/migrate/src/Exception/MigrateEntityViolationException.php
@@ -0,0 +1,31 @@
+class MigrateEntityViolationException extends MigrateException {

    Let's not call this entity in the name. Not all things migrated are destined for an entity. It is more re-usable if the exception is more generic. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -111,6 +116,32 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
+    return isset($this->configuration['validate']) ? TRUE : FALSE;

    A lot of times, we just check if isset. And return that value. if it is set, then we assume the configuration was desired.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented 
+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
@@ -0,0 +1,139 @@
+    \Drupal::service('event_dispatcher')->addListener(MigrateEvents::IDMAP_MESSAGE, array($this, 'mapMessageRecorder'));
+    // Execute the migration.
+    $executable = new MigrateExecutable($this->migration, $this);
+    $executable->import();

A bit of a nit. But all, or nearly all, of the migration integration tests do this in the setup() method. Can this be moved into setup?

piggito’s picture

piggito CreditAttribution: piggito as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
add_option_to_content-2745797-39.patch7.24 KB
interdiff-35-39.txt6.24 KB

Rerolling patch and applying changes suggested in #37 and #38

Status: Needs review » Needs work

The last submitted patch, 39: add_option_to_content-2745797-39.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Was looking at the documentation and found a few things.

  1. +++ b/core/modules/migrate/src/Exception/ValidationException.php
@@ -0,0 +1,31 @@
+ * Class ValidationException.

    Let's have something more meaningful here. Maybe 'Thrown when migrated content fails validation' or some such.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -112,6 +117,32 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
+   * Whether or not the entity needs validation.

    Let's be more descriptive, something like 'Gets the validation configuration key.'? 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -112,6 +117,32 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
+    return isset($this->configuration['validate']);

    Since this is adding a new configuration key it will need documenting. This should be done after #2862662: Add documentation to EntityContentBase destination plugin is in.

  4. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
@@ -0,0 +1,130 @@
+ * Tests the EntityContentBase destination.

    This is testing the validation of an entity and that exceptions are thrown, not the EntityContentBase destination, which is tested in MigrateEntityContentBaseTest. It needs changing.

  5. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
@@ -0,0 +1,130 @@
+    // Migration definition.
...
+    $executable->import();

    This block is setup, is there a reason it can't be in the setup() method?

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2745797-42.patch8.18 KB
interdiff-39-42.txt3.59 KB

1-5: done.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work

This is shaping up nicely.

  1. +++ b/core/modules/migrate/src/Exception/ValidationException.php
@@ -0,0 +1,31 @@
+class ValidationException extends MigrateException {

    Let do call this with Entity in the name. It only works with ECB and entities. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -171,6 +182,32 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
+  protected function validateEntity(ContentEntityInterface $entity) {

    Let's add a MigrateValidatableInterface that has needsValidation and validate on it. Then implement that in ECB. That way we can easily add validation to other destinations or sources or anything as we see fit. Then in ECB, we can just call validate/needsValidation because it is an instanceof MigrateValidatableInterface. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -25,6 +26,8 @@
+ * - validate: (optional) Boolean, indicates if the entity should be validated,

    Can we show a code example below where we describe what happens with when validate is flagged true?

joachim’s picture

joachim CreditAttribution: joachim commented

I've just spotted what I think is a pretty big inconsistency.

For migrations, we don't validate entities at all, and we're only considering adding an option.

However, EntityContentBase::processStubRow() checks all fields for whether they are required:

    $fields = $this->entityManager
      ->getFieldDefinitions($this->storage->getEntityTypeId(), $bundle_key);
    foreach ($fields as $field_name => $field_definition) {
      if ($field_definition->isRequired() && is_null($row->getDestinationProperty($field_name))) {
        // Use the configured default value for this specific field, if any.
        if ($default_value = $field_definition->getDefaultValueLiteral()) {
          $values[] = $default_value;
        }
        else {
          // Otherwise, ask the field type to generate a sample value.
          $field_type = $field_definition->getType();
          /** @var \Drupal\Core\Field\FieldItemInterface $field_type_class */
          $field_type_class = $this->fieldTypeManager
            ->getPluginClass($field_definition->getType());
          $values = $field_type_class::generateSampleValue($field_definition);
          if (is_null($values)) {
            // Handle failure to generate a sample value.
            throw new MigrateException('Stubbing failed, unable to generate value for field ' . $field_name);
          }
        }

which is effectively doing a large part of entity validation!

AdamPS’s picture

AdamPS CreditAttribution: AdamPS at AlbanyWeb commented

Nice patch, thanks. However the error reporting can sometimes be a bit unhelpful...

1)

source_ids_hash level message
e2f94adfd264b8baef67132ba141d8ab74ed7112828e1e5c9d2d2 1 This value should be of the correct primitive type.
1b80f334fa8

The validation fails as I assigned a string into a bool. The error should report which field is the problem.

EDIT: Also it should report which row/entity is the problem in a way that the end user can understand - source_ids_hash is not so easy to deal with. As another example, a message such as "The username contains an illegal character." without indication of which user is not so easy to resolve.

2) The validation fails when static_map doesn't match. There is no error, just a skipped row. There is no entry in the messages to indicate what the cause was.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Assigned: Unassigned » BR0kEN
Status: Needs work » Needs review
Issue tags: +DevDaysCluj, +DevDaysTransylvania
FileSize
2745797-49.patch12.8 KB
interdiff-2745797-42-49.txt10.37 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -13,11 +15,6 @@
- * Available configuration keys:
- * - translations: (optional) Boolean, if TRUE, the destination will be
- *   associated with the langcode provided by the source plugin. Defaults to
- *   FALSE.
- *

@@ -48,18 +45,14 @@
- *   translations: true
...
- * This will save the processed, migrated row as a node with the relevant
- * langcode because the translations configuration is set to "true".
- *

    These changes should be reverted. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -48,18 +45,14 @@
+abstract class Entity extends DestinationBase implements ContainerFactoryPluginInterface, DependentPluginInterface, MigrateValidatableEntityInterface {

    I don't think we want to do this here. Config entities extend from here and shouldn't be validated. They already do their own validation internally. Rather add this interface to ECB. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -235,4 +228,18 @@ public function calculateDependencies() {
+  public function needsEntityValidation() {
+    return isset($this->configuration['validate']);

    Maybe name this isValidationEnabled? 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -75,6 +80,9 @@
+ *   # Run "Drupal\Core\Entity\FieldableEntityInterface::validate()" before

    Suggestion, "Run entity and field validation before saving an entity." 

  5. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -161,6 +171,19 @@ public function import(Row $row, array $old_destination_id_values = []) {
+    assert($entity instanceof FieldableEntityInterface);
...
+    assert($violations instanceof EntityConstraintViolationListInterface);

    Can we add some comments why we need the asserts? This pattern isn't very common yet, so some rational in a comment could assist.

  6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -161,6 +171,19 @@ public function import(Row $row, array $old_destination_id_values = []) {
+      throw new EntityValidationException($violations);

    As part of this, let's also pass the full entity to the exception too and list the entity id in the message. This is to respond to #46

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

#50.1 do you think this should go in a separate issue since that documentation isn't related to the class it's added to?

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

#50.2, in my opinion, it's better to have this for all type of entities because custom destination plugin is able to provide an additional, migration-related level of validations easily.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

re #51: If you feel we need to remove those comments, then open another issue. It doesn't belong in this one and is just extra noise.

re #52. I see your point, but most people don't create custom destinations. They are rare and usually an indication of code smell. 98% of the time the ones provided by core are enough. And even when someone does find a reason to create one, custom content destinations extend from ECB. If they don't, they should.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

#53.1 agree.
#53.2, yeah but I also meant configuration entities. Everyone who'll extend `EntityConfigBase` (btw, `ECB` abbr valid for both `EntityConfigBase` and `EntityContentBase`) is also have the ability to provide a validation. I'd prefer to be consistent and handle all entity types from core.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
2745797-55.patch13.53 KB
interdiff-2745797-49-55.txt9.46 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

I'm still not fully convinced that we need to add validation for config entities. They already do their own validation on save from my experience. So adding the validation on the entitycontentbase should be be enough.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

In most scenarios that method will simply be executed with no payload during the import of configurations. I agree that mostly it shouldn't be needed.

My points are:
- we implement a consistent solution for both config and content entity types;
- we leave an ability for an overridden/custom destination plugin to implement their own validation.

Let's leave this in NR now, waiting for other opinions. I'm not against restricting this to the content entity only, so will do this once someone else say it's not a good idea to provide this solution for config destination too.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
2745797-58.patch14.11 KB
interdiff-2745797-42-58.txt16.09 KB

I rethink a bit the need of having this for config entities.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
2745797-59.patch10.19 KB
interdiff-2745797-42-59.txt12.27 KB
interdiff-2745797-58-59.txt3.79 KB

Ooops.

The last submitted patch, 58: 2745797-58.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work

This is really shaping up nicely. Just a couple nits.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -161,6 +171,24 @@ public function import(Row $row, array $old_destination_id_values = []) {
+    return isset($this->configuration['validate']);

    Can we use !empty? That way if someone sets to false, this won't trigger. 

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentValidationTest.php
@@ -0,0 +1,118 @@
+    $this->assertFalse(isset($this->messages[2]));

    It would be cleaner to use assertArrayNotHasKey or assertCount(2, $this->messages)

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
2745797-62.patch10.17 KB
interdiff-2745797-59-62.txt2.16 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

Cosmetic only changes in #62. They'll hopefully come back green. All feedback addressed. We have test coverage. I think we're good to go.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
2745797-64--8.6.x.patch10.15 KB

The same patch for 8.6.x.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Reviewed & tested by the community » Needs work
  1. The message like [node ]: This value should not be null. have to contain a field/property name to which it's assigned.
  2. The entity locator for new entities has to be changed from [node ] to [node].
  3. The entity validation should be run when $entity->isValidationRequired() is TRUE even if $this->isEntityValidationEnabled() is FALSE.
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
2745797-66.patch11.36 KB
interdiff-2745797-62-66.txt6.01 KB
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
2745797-67--8.6.x.patch11.33 KB
interdiff-2745797-64-67.txt6.01 KB

The last submitted patch, 66: 2745797-66.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 67: 2745797-67--8.6.x.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented 
  1. +++ b/core/modules/migrate/src/Exception/EntityValidationException.php
@@ -13,6 +13,18 @@
+  const MESSAGES_GLUE = '||';

    How about MESSAGES_SEPARATOR? 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -161,7 +166,8 @@
+    if ($this->isEntityValidationEnabled() || $entity->isValidationRequired()) {

    This is a change from current levels of validation. I'm not sure we want to do this. It could have backwards compatibility implications.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

#70.1 makes sense. Why not?
#70.2 the ContentEntityBase::preSave() will throw an exception if an entity has to be validated. So I assume having this condition we simplify the process of configuring the migrations.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
interdiff-2745797-67-72.txt3.17 KB
2745797-72--8.6.x.patch12.14 KB
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
interdiff-2745797-66-73.txt3.18 KB
2745797-73.patch12.17 KB
rosinegrean’s picture

rosinegrean CreditAttribution: rosinegrean at PitechPlus commented
Issue tags: -DevDaysCluj
BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
FileSize
2745797-75.patch13.93 KB
interdiff-2745797-73-75.txt9.93 KB
  1. Enhanced tests coverage.
  2. Improved code documentation.
  3. Removed redundant codebase.
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -153,7 +166,10 @@ public function import(Row $row, array $old_destination_id_values = []) {
+    if ($entity->isValidationRequired() || $this->isEntityValidationRequired()) {

@@ -161,6 +177,24 @@ public function import(Row $row, array $old_destination_id_values = []) {
+  public function isEntityValidationRequired() {

I think we want to wrap the entity check isValidationRequired into isEntityValidationRequired. And log an error or warning if the entity doesn't support validation but someone has configured validation.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Needs work

Setting to NW per #76

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented

And log an error or warning if the entity doesn't support validation but someone has configured validation.

The Drupal\Core\Entity\ContentEntityInterface cannot to not have the validation since it implements the Drupal\Core\Entity\FieldableEntityInterface that defines the validate() method.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
2745797-79.patch14.3 KB
interdiff-2745797-75-79.txt2.08 KB
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work

Almost there.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -180,8 +180,11 @@
+    // possible to save that entity unvalidated.

Nit: this seems a little odd wording. Could we phrase it:

The entity to check for required validation.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
2745797-81.patch14.3 KB
interdiff-2745797-79-81.txt665 bytes
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

This has gone through extensive reviews at this point. We've got tests and I think we are covered. The only odd thing about this patch is the use of asserts. We don't typically see them, but that isn't up to me to decide. So bumping over to RTBC and we'll see what additional feedback we might receive.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented 
  1. +++ b/core/modules/migrate/src/Exception/EntityValidationException.php
@@ -0,0 +1,87 @@
+      assert($violation instanceof ConstraintViolationInterface);

    Ensures each violation is of a correct type. 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -153,7 +166,10 @@ public function import(Row $row, array $old_destination_id_values = []) {
+    assert($entity instanceof ContentEntityInterface);

    Needed because methods below expect a content entity.

Also, using assert() you replace the PHPDoc comment for IDE autocompletion.

Generally saying, use of asserts helps to make sure everything goes as expected during development. For production-like environments the code enclosed in assert() is simply invisible for interpreter.

https://www.drupal.org/docs/8/api/runtime-assertions

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 81: 2745797-81.patch, failed testing. View results

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Discussed on the cross initiative call around whether this should be the new default. @heddn pointed out that it is often easier to fix issues on the new site rather than fix them prior to the migration. On the other hand some of our Drupal 8 minor update issues have been cause been invalid entities created by migrate. We agreed to create follow-up to discuss what the default should be.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs release note, +Needs change record

This looks great, but we need a change-record here to announce the new feature and a release notes snippet in the issue summary.

Tagging and setting to NR for those.

BR0kEN’s picture

BR0kEN
Primary language Ukrainian
CreditAttribution: BR0kEN as a volunteer and at FFW, Drupal Ukraine Community for FFW, Drupal Ukraine Community commented
Issue tags: -Needs change record

Added change record - https://www.drupal.org/node/3073707. Can I help with the release note?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs release note

Added a release note and reviewed and updated the CR. With that, I think we're good for RTBC again.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +8.8.0 highlights

  • larowlan committed 17f9026 on 8.8.x 
    Issue #2745797 by BR0kEN, piggito, harings_rob, heddn, biguzis, Jo...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 17f9026 and pushed to 8.8.x. Thanks!

Published the change record 🎉

Nice work folks

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Closed (fixed) » Active
Issue tags: +Needs followup

#86:

Discussed on the cross initiative call around whether this should be the new default. @heddn pointed out that it is often easier to fix issues on the new site rather than fix them prior to the migration. On the other hand some of our Drupal 8 minor update issues have been cause been invalid entities created by migrate. We agreed to create follow-up to discuss what the default should be.

Nearly 4 months have passed, and that follow-up has not yet been created.

Because of the huge consequences of migrations that don't validate entities, and because the discussion in #86 is not accessible to me now, I'm taking the atypical and usually inappropriate action of reopening this closed issue, so that the follow-up is filed. It needs to provide sufficient context to allow Migrate contributors to help Drupal get to a place closer to where Drupal migrations do validate all migrated entities. Invalid/incorrect migrations cause problems for content authors (when they're editing content and validation errors occur for unchanged fields), for decoupled application developers (when JSON:API fails due to incompletely/incorrectly migrated data model definitions) as well as for site owners (when minor-to-minor update paths fail, as @alexpott already pointed out in #86).

Please forgive me 🙏😇

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Active » Fixed
Issue tags: -Needs followup

The discussion in #86 is preserved for posterity at https://youtu.be/9rBkBl4uGlg?t=1265

I'd rather just open the follow-up rather than re-opening this issue,so I opened #3095456: [Plan] Discuss strategy for long-term use of entity validation in the migration system

Nothing to forgive, my friend. Thank you for stirring the pot on these important discussions that have fallen by the wayside.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Fixed » Closed (fixed)

Much appreciated! 🙏🥳