Let entity_ref Selection handlers be in charge of the field validation

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php
@@ -49,6 +49,20 @@ public function countReferenceableEntities($match = NULL, $match_operator = 'CON
+   * Validates that entities can be referenced by this field.

Selection handlers are not tied to the ER field anymore, so this one-line description shouldn't mention it either. The same is true for the issue title for that matter :)

Selection handlers don't know about fields, but fields do rely on Selection handlers, right ?

Yes :)

And the description of SelectionInterface::validateReferenceableEntities() in HEAD is the one that's wrong, not the fault of this patch, sorry :/

The change makes totally sense. Had a short look at the patch:

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php
   @@ -49,6 +49,24 @@ public function countReferenceableEntities($match = NULL, $match_operator = 'CON
   +   * Validates that entities can be referenced.
   +   *
   +   * @todo naming...
   

   validateReferenceableAutoCreatedEntities() maybe ?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraint.php
   @@ -26,10 +26,24 @@ class ValidReferenceConstraint extends Constraint {
   +  public $invalidAutocreateMessage = 'This entity (%type: %label)cannot be referenced.';
   

   missing space before "cannot"

The patch looks great to me!

I can't decide if validateReferenceableAutoCreatedEntities() is more clear than the current method name, I'm ok with either of them.

Promoting to critical based on #2587957-26: Arbitrary files can be referenced in file fields even if validation using form API would fail

Since this is critical now, it no longer needs to be triaged as an RC target. Thanks!

The underlying bug is that the autocreate widget creates unpublished nodes, which is probably not the desired behavior to begin with.

The fix for this should be pretty simple, we just move the responsibility of creating the new entity to the selection handlers, because they already have hardcoded logic about what makes an entity referenceable :)

Hm.. having a separate interface would also help with hiding the autocreate feature in the UI if the target entity type doesn't support it, but that means we would need to provide a new selection handler for each core / contrib / custom entity type, which is.. not very DX friendly :/

About having two methods, one to create the entity and one to validate it, isn't that a bit superfluous? I mean.. the method that creates the new entity should also know very well if it's valid or not.

I can work on the patch once we reach an agreement on the direction :)

Right, two methods it is then. Now we just need to settle on separate interface or not :)

+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -260,6 +260,15 @@ public function validateReferenceableEntities(array $ids) {
+    // ViewsSelection does not supprt autocreate, there is no way to check that

typo: supprt

Then autocreate becomes opt-in by entity type, that's kind of a behavior break, but it also sort of reflects the current state of affairs (autocreate is mostly supported on taxo terms)

I think I'm more in favor of keeping autocreate as opt-out by default, for the reasons mentioned in #49.

Here's a patch that fixes the creation of new entities for core entity types by moving the createNewEntity() method to the selection handlers. Both new methods are now on a new interface, as discussed above, and DefaultSelection implements this interface in order to keep the current behavior from HEAD.

The test fail from #54 is caused by a test form element that was supposed to have its validation disabled but it didn't :/

Still a behavior change though : if your custom selection class doesn't implement the new interface, no autocreate.

We could write a short change notice for this, which should also mentioned that \Drupal\Core\Entity\Element\EntityAutocomplete::createNewEntity() was moved to the new "selection with autocreate" interface, but I'll leave it to core committers to decide if we need it or not.

+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -18,6 +18,16 @@ class EntityReferenceFieldItemList extends FieldItemList implements EntityRefere
+    $constraints[] = $constraint_manager->create('ValidReference', []);

I tried the above patch and DER is failing tests because of this. DER doesn't use ValidReference constraint. It has its own ValidDynamicReference constraint so after this patch DER has to unset ValidReference constraint in DERFieldItemList class which we can do in #2405467: Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem so no biggie.

Other then constraint DER doesn't touch EntityAutocomplete or EntityReferenceSelection just use them as is so moving the method createNewEntity won't affect DER at all.

+1 form DER.

If we're going to have a behaviour change then we need a CR

Here it is: https://www.drupal.org/node/2606656

#2594565: File extension + max_filesize should be validated using a Constraint on file fields got committed which causes conflicts here, sorry.

No worries, here's a reroll :)

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
   @@ -7,39 +7,142 @@
   +  public function __construct(SelectionPluginManagerInterface $selection_manager, EntityManagerInterface $entity_manager) {
   +    $this->selectionManager = $selection_manager;
   +    $this->entityManager = $entity_manager;
   

   entity_manager is deprecated, use one of the new services.

2. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -42,6 +42,32 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +    // In order to create a referenceable comment, it needs to published.
   +    $entity->status->value = CommentInterface::PUBLISHED;
   

   can we use setPublished() here if we know that the entity is of CommentInterface? When we deal with base fields we should never use the weird $entity->{$random_field_name}->value access method but always the dedicated methods on the interface.

3. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -42,6 +42,32 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +      $entities = array_filter($entities, function ($entity) {
   +        return $entity->status->value === CommentInterface::PUBLISHED;
   +      });
   

   same here, setPublished().

4. +++ b/core/modules/file/src/Plugin/EntityReferenceSelection/FileSelection.php
   @@ -27,8 +27,39 @@ class FileSelection extends DefaultSelection {
   +    // In order to create a referenceable file, it needs to have a "permanent"
   +    // status.
   +    $entity->status->value = FILE_STATUS_PERMANENT;
   

   can we use setPermanent() here from FileInterface?

5. +++ b/core/modules/file/src/Plugin/EntityReferenceSelection/FileSelection.php
   @@ -27,8 +27,39 @@ class FileSelection extends DefaultSelection {
   +    $entities = array_filter($entities, function ($entity) {
   +      return $entity->status->value === FILE_STATUS_PERMANENT || $entity->uid->target_id === $this->currentUser->id();
   +    });
   

   can we use isPermanent() and getOwnerId() here form FileInterface?

I'm a bit worried about new files that are set to permanent immediately, but I'm not familiar enough with the ER selection handling code, would have to test that.

+++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
@@ -42,6 +42,32 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
+    $entity = parent::createNewEntity($entity_type_id, $bundle, $label, $uid);

We should probably not call the variable $entity here but rather $comment instead. That way it is immediately clear that we are dealing with comments only here, because the selection handler is only for comments anyway.

Also in all the other selection handler places added by this patch.

Hm, we only allow auto creation of new entities on taxonomy term reference fields in core. There is currently no way to auto create a file through an entity reference field.

In order to even get an entity reference field that references files (not a file field!) you need to add a field referencing nodes, then edit it and switch it to referencing files.

I don't see the use case for using auto_create with files since there is no form in Drupal that would allow you creating files. The one and only place to add files is via field fields, file entities are completely dependent on that field, they can only be created there. The additions to FileSelection.php look a bit bogus to me, since that code can never be used in Drupal core. Maybe we should let that open for some contrib module like media that knows more about the use case of file creation/selection? Or maybe I'm just missing something here.

+++ b/core/lib/Drupal/Core/Field/EntityReferenceFieldItemList.php
@@ -18,6 +18,16 @@ class EntityReferenceFieldItemList extends FieldItemList implements EntityRefere
+    $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();

should use $this->getTypedDataManager() instead of \Drupal.

Thanks for the reviews, @klausi :)

This patch fixes #67, #68, #70 and the current test failures.

Hm, we only allow auto creation of new entities on taxonomy term reference fields in core. There is currently no way to auto create a file through an entity reference field.

That's only true for the UI, the "autocreate" API in HEAD supports all entity types. For example, this piece of code uses the autocreate/autosave functionality:

$user = User::create([
  'name' => 'username',
  'status' => 1,
]);

// Don't call $user->save();

$referencing_entity->user_reference_field->entity = $user;

// At this point, we should be checking if the referenced user entity is valid, which we don't do in HEAD and it's what this patch fixes.
$referencing_entity->validate();

// At this point, the referenced user entity will also be saved as part of the "parent" save process.
$referencing_entity->save();

In order to even get an entity reference field that references files (not a file field!) you need to add a field referencing nodes, then edit it and switch it to referencing files.

I don't understand this at all.. can you explain in a bit more detail what you see as not working?

I think what he meant is that the "File" option under References is a file field and not an entity reference field. But you don't need to select node first and then change, you can also select other and then select file.

But yes, it's definitely not possible to create a working new file through a normal entity reference field, unlike terms or other things, creating the entity isn't enough, you actually need a file behind it. As @yched commented in the other issue, the only possible use case is using that for validation through the inherited behavior/configuration in a file field reference instead of the custom constraint that we added now.

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionWithAutocreateInterface.php
   @@ -0,0 +1,52 @@
   +   * This method should replicate the logic implemented by
   +   * validateReferenceableEntities(), but applied to newly created entities that
   +   * have not been saved yet.
   

   what validateReferenceableEntities() method do you mean? It does not exist on this interface? Should you point to another interface that has this method? Or should this interface extend the other interface?

2. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
   @@ -42,6 +42,33 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +        return $entity->status->value === CommentInterface::PUBLISHED;
   

   we should use $comment as variable name and $comment->isPublished()

3. +++ b/core/modules/node/src/Plugin/EntityReferenceSelection/NodeSelection.php
   @@ -48,4 +48,31 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +      $entities = array_filter($entities, function ($entity) {
   +        return $entity->status->value === NODE_PUBLISHED;
   +      });
   

   same here, let's use $node and ->isPublished().

4. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -171,6 +171,40 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +      $entities = array_filter($entities, function ($entity) {
   +        return $entity->status->value === 1;
   +      });
   

   same here, let's use $user and ->isActive()

This patch does not add any new test coverage, could you list in the issue summary which test cases cover the new code? I'm having a hard time finding them. Basically there should be a test somewhere that adds an invalid + unsaved entity into a reference field of another valid entity, then calls validate() on that entity and get the violations from the invalid referenced entity.

It seems that EntityAutocomplete::validateEntityAutocomplete() duplicates the work of ValidReferenceConstraintValidator a bit, but since EntityAutocomplete is a form element that must function independently of the entity validation system this should be fine.

Otherwise the changes make sense to me.

Fixed all the points from #73 and added test coverage. Thanks for reviewing again :)

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceItemTest.php
@@ -367,6 +381,100 @@ public function testValidation() {
+    $title = $this->randomString();

Please no random data in tests, see #2571183: Deprecate random() usage in tests to avoid random test failures. Please remove all random*() calls from the lines that you add and use hard-coded strings instead.

The test does not cover the case where multiple entities are referenced in one field. We should at least have one that mixes multiple existing and new entities in one field so that we test that the array_diff/reduce stuff works.

I discussed this issue with @catch and @xjm, so want to update folks here that RC4 will be tagged within the next couple of hours and will be the final RC. If this doesn't land by then, I think we'll need to do the following:

1. Move this issue to 8.1 for the parts that can be done in a minor, and if necessary, open an issue for 9.x to do the rest.
2. Open a different critical issue for 8.0 to at least add text to the UI that lets people know that when they configure the ER selection, that it's only for UX purposes of the widget, and not a firm constraint (e.g., a content editor can bypass it via the autocomplete widget by manually typing a different ID). Such UI text could potentially be committed between RC4 and 8.0.0.

The randomness in the tests should not block commit, until #2571183: Deprecate random() usage in tests to avoid random test failures lands nothing is deprecated, and we've not agreed to block patches for using those methods yet. I agree with dropping the randomness in general though.

The extra test coverage would be great, but I think this patch gets perilous for 8.x at all once RC4 is tagged, so I would strongly consider doing that in a follow-up.

Or to put it differently:

- if we put it into RC4 as is, and something horrible is picked up before 8.0.0 is tagged via RC4 testing, we can roll it back.

- if we put it in after RC4, we might not pick anything horrible up until after 8.0.0 is tagged.

- if we don't put it in at all, that is horrible.

So the first option, for me at least, is the least horrible.

I don't see this being fixable in 8.1.x as is, I think we'd have to do something like https://www.drupal.org/project/field_validation in core and then have an 'entity reference selection handler validator' which could get messy fast just to me what for me is a natural user expectation of the current interface.

Given all of those things, moving to RTBC.

I agree with catch's comment in #78 - committing this patch as-is is probably the best way to go. I think we're missing test coverage where multiple new entities are valid and invalid and where we mix new and existing entities.

Committed fbe0991 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
index 4d1a257..6dd6e10 100644
--- a/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,7 +14,6 @@
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Render\Element\Textfield;
 use Drupal\Core\Site\Settings;
-use Drupal\user\EntityOwnerInterface;
 
 /**
  * Provides an entity autocomplete form element.

Removed unused use on commit.

Opened #2614408: Add test coverage for multiple invalid + mixed existing/new entity reference validation.

per #79.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -40,7 +40,6 @@
  *   default_formatter = "entity_reference_label",
  *   list_class = "\Drupal\Core\Field\EntityReferenceFieldItemList",
- *   constraints = {"ValidReference" = {}}
  * )

There are at least three contrib modules that used that constraint like this. Payment, Simplenews and Entity Reference Revisions.

The problem is that ValidReferenceConstraintValidator dies with a fatal error if you still have that as it expects a list of field items now and not a single one.

Would have been nice to avoid an error but we need to at least mention this in the change record. Not quite sure what to write, is it enough to just remove this or do those modules need to do anything?

As discussed with @Berdir, I updated the change record to mention the changes needed for the ValidReference constraint: https://www.drupal.org/node/2606656/revisions/view/9103328/9127282