Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem

We also need validation tests for each class i.e. DynamicEntityReference, DynamicEntityReferenceItem and DynamicEntityReferenceFieldItemList.

+++ b/src/Plugin/DataType/DynamicEntityReference.php
@@ -61,4 +61,13 @@ class DynamicEntityReference extends EntityReference {
+    $this->definition->addConstraint('Bundle', ['bundle' => $this->parent->parent->getSetting($this->parent->getValue()['target_type'])['handler_settings']['target_bundles']]);

There should be some better way to do this.

1. +++ b/src/DynamicEntityReferenceFieldItemList.php
   @@ -131,4 +131,20 @@ class DynamicEntityReferenceFieldItemList extends EntityReferenceFieldItemList {
   +   * {@inheritdoc}
   
   +++ b/src/Plugin/DataType/DynamicEntityReference.php
   @@ -61,4 +61,25 @@ class DynamicEntityReference extends EntityReference {
   +   * {@inheritdoc}
   
   +++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
   @@ -472,4 +472,13 @@ class DynamicEntityReferenceItem extends ConfigurableEntityReferenceItem {
   +   * {@inheritdoc}
   

   We should add some additional documents to explain this.

2. +++ b/src/Plugin/Validation/Constraint/ValidDynamicReferenceConstraint.php
   @@ -26,6 +26,7 @@ class ValidDynamicReferenceConstraint extends Constraint {
   +  public $wrongIDMessage = 'The referenced entity (%type: %id) does not exist.';
   +  public $wrongTypeMessage = 'The referenced entity (%type: %id) is not allowed for this field.';
   

   Can we please improve these messages? So that user can understand which value is not correct.

We still have this @todo left

    // @todo Implement a test case for invalid bundle references.

perhaps we should have to add test for entities which don't have the bundles.

Thanks for the fixes. Some more nitpicks :P

1. +++ b/src/Tests/DynamicEntityReferenceFieldTest.php
   @@ -119,16 +128,21 @@ class DynamicEntityReferenceFieldTest extends EntityUnitTestBase {
   +    $dud_referenced_entity = entity_create($this->referencedEntityType, array('type' => 'dud'));
   

   We can use EntityTest::create here.

2. +++ b/src/DynamicEntityReferenceFieldItemList.php
   @@ -131,4 +131,24 @@ class DynamicEntityReferenceFieldItemList extends EntityReferenceFieldItemList {
   +      if (!$return) {
   ...
   +      else {
   

   Can we please explain this if/else as well?

3. +++ b/src/Plugin/DataType/DynamicEntityReference.php
   @@ -61,4 +61,29 @@ class DynamicEntityReference extends EntityReference {
   +    $field = $this->parent->parent;
   

   Can we please add /* @var \.. $field */ here for better understanding?

4. +++ b/src/Tests/DynamicEntityReferenceFieldTest.php
   @@ -78,13 +78,22 @@ class DynamicEntityReferenceFieldTest extends EntityUnitTestBase {
   -    FieldConfig::create(array(
   +    FieldConfig::create([
   

   This is fine here. Let's use @grom358's script in a follow up issue and fix it for whole module.

@larowlan thank you so much for fixing this. Patch looks good. Let me update IS and ping @fago for opinion.

1. +++ b/src/Plugin/Validation/Constraint/ValidDynamicReferenceConstraintValidator.php
   @@ -46,10 +47,54 @@ class ValidDynamicReferenceConstraintValidator extends ConstraintValidator {
   +          '%bundles' => implode(', ', $target_type_settings['handler_settings']['target_bundles']),
   

   Isn't this the disclosure of information?

2. +++ b/src/Plugin/Validation/Constraint/ValidDynamicReferenceConstraintValidator.php
   @@ -46,10 +47,54 @@ class ValidDynamicReferenceConstraintValidator extends ConstraintValidator {
   +  public function getConstraints() {
   

   I think it is not suppose to be here.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -154,6 +148,28 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +  public function getConstraints() {
   +    $constraints = parent::getConstraints();
   +    list($current_handler) = explode(':', $this->getSetting('handler'), 2);
   +    if ($current_handler === 'default') {
   +      $handler_settings = $this->getSetting('handler_settings');
   +      if (!empty($handler_settings['target_bundles'])) {
   +        $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();
   +        $constraints[] = $constraint_manager->create('ComplexData', [
   +          'entity' => [
   +            'Bundle' => [
   +              'bundle' => $handler_settings['target_bundles'],
   +            ],
   +          ],
   +        ]);
   +      }
   +    }
   +    return $constraints;
   +  }
   

   @larowlan have you this seen from #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled.
   Do you think we should accommodate/bypass this hunk in DER? I believe this is not same for DERItem.

4. +++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
   @@ -15,6 +15,13 @@
   +    $constraints[] = $constraint_manager->create('ValidReference', []);
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -33,8 +30,7 @@
   - *   constraints = {"ValidReference" = {}}
   

   Also #2577963: Let entity_ref Selection handlers be in charge of the field validation removes ValidReference from ERItem and moves it to ERItemList. Should we also consider that in follow up when #2577963: Let entity_ref Selection handlers be in charge of the field validation is fixed.

5. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -111,7 +111,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -      'target_bundles' => array(),
   +      // For the 'target_bundles' setting, a NULL value is equivalent to "allow
   +      // entities from any bundle to be referenced" and an empty array value is
   +      // equivalent to "no entities from any bundle can be referenced".
   +      'target_bundles' => NULL,
   

   FYI after #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted NULL means all bundles and empty array means no bundle so following is the wrong assumption in the patch

   +++ b/src/Plugin/Validation/Constraint/ValidDynamicReferenceConstraintValidator.php
   @@ -46,10 +47,54 @@ class ValidDynamicReferenceConstraintValidator extends ConstraintValidator {
   +    // No bundles selected = all bundles are valid.
   

6. Also, I believe we are missing EntityType constraint for entity property in DERItem we should add it in DERItem::getConstraints(). I have updated IS to accommodate that case.

#2577963: Let entity_ref Selection handlers be in charge of the field validation is in so I pushed a fix for the failing tests 5df5af3. Here is a reroll and WIP patch. Please feel free to pick this up.

There are proper tests for each use case if you'd like to complete it then you just need to fix the fails and add new asserts about bundle and entity type constraints. Please provide the interdiff for reviewer. Thanks.

Updated the issue summary.

Reroll and some clean up.

Thanks @Denchev for the patch and thanks @slashrsm for the review.

+++ b/src/Plugin/Validation/Constraint/ValidDynamicReferenceConstraintValidator.php
@@ -2,50 +2,153 @@
+      $handler = $this->selectionManager->getSelectionHandler($value->getFieldDefinition(), NULL, $target_type);

This $target_type can be NULL which we don't want.

Here is how I thought we should proceed.

1. Validate not NULL first for properties and return violationnullMessage if any field property is NULL.
2. If the entity is new then keep them in an array$new_entities and validate them later cause we need to validate the entity type first.
3. Validate target type this includes new entities as well. Keep adding the violations$wrongTypeMessage. Remove the new entities from new entities array.
4. Now validate the new entities using validateReferenceableNewEntities. Keep adding the violationsinvalidAutocreateMessage.
5. Validated target_id using validateReferenceableEntities. Keep adding the violations.

See ValidReferenceConstraintValidator in core for more details.

Wow, nice update. Thanks for the patch. Here is the answer to your questions:

1. This is perfect.
2. Yeah
3. That's alright.
4. Yeah, sure.

Can you please upload this path for 8.x-2.x and test it againest that branch?

It is green for 8.x-1.x.

Re-roll for 8.x-2.x.

Thanks! @Denchev. I think this is ready. I asked @larowlan to review this so assigning it to him now. Meanwhile, if @Berdir would be able to have a look at it then it'd be great.

Thank you. Committed and pushed to both branches.

We have a followup bug #2822849: Fatal error when using 8.x-1.x-dev version..