Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled

Suppose the related could be closed as duplicate

The module actually unsets this base setting and uses the target_bundles setting from the handler settings.

EntityReferenceItem::defaultStorageSettings() sets 'target_bundle' to NULL, but it is not defined by schema, so for some field.storage configs I get "field.storage.my_entity_type.my_er_field:settings.target_bundle missing schema". Not sure why it does not happen for all such configs.

Is field.storage_settings.entity_reference.target_bundle supposed to be replaced by entity_reference.default.handler_settings.target_bundles?

Edit: For the record, to solve my specific problem, I had to enable the entity_reference module which replaces the field type implementation. Sorry if this was not so relevant to the actual issue.

Yeah, now that we have moved the notion of "selection handler" up to the core ERItem, not sure that the notion of $storage_settings['target_bundle'] (outside of the "handler") means anything anymore.

entity_reference\ConfigurableEntityReferenceItem::defaultStorageSettings() removes it with a comment that "The target bundle is handled by the 'target_bundles' property in the 'handler_settings' instance setting" (which is now true also for Core's ERItem, not just ConfigurableERItem)

AFAICT it is only specified by forum/config/install/field.storage.node.taxonomy_forums.yml (which looks stale anyway, since it doesn't define a 'handler')

@amateescu, what do you think ?

Oooooooooooooooooooooooooooh :-)
/me repeatedly jumps on the "follow" button on that issue

/me wonders if @yched did an even or uneven number of jumps... ;)

HAH ! @Berdir++ :-D

This will be fixed in #2429191: Deprecate entity_reference.module and move its functionality to core.

Closing this as duplicate of #2429191: Deprecate entity_reference.module and move its functionality to core.

Giving a shot at this, because it would be really good to get rid of this stale/duplicate setting in case #2429191: Deprecate entity_reference.module and move its functionality to core doesn't make it.

(also, the patch in #2429191: Deprecate entity_reference.module and move its functionality to core is currently wrong on this : the 'target_bundles' is now part of the field-level settings, so we don't know it in ERItem::propertyDefinitions(), where all we have is a FieldStorageDefinition)

Just an update: #2429191: Deprecate entity_reference.module and move its functionality to core is fixing this.

Defer the "ERItem::getConstraints()" to an actual method on the Selection handler, instead of hardcoding it to just the "default" selection handler.

Aw facepalm.

We need some upgrade function for that, of course.

Damn. That last fail in EntityFieldtest beats me, and I have no clue how #2429191: Deprecate entity_reference.module and move its functionality to core is green :-)

Basically that part in EntityFieldtest :

    $definition = BaseFieldDefinition::create('entity_reference')
      ->setLabel('Test entity')
      ->setSetting('target_type', 'node')
      ->setSetting('target_bundle', 'article');
    $reference_field = \Drupal::TypedDataManager()->create($definition);
    $reference = $reference_field->appendItem(array('entity' => $node))->get('entity');
    $violations = $reference->validate();
    $this->assertEqual($violations->count(), 1); 

tests the *last* constraint added in that code from ERItem::propertyDefinitions() below (the one applied on $properties['entity']->getTargetDefinition()) :

      $properties['entity']->addConstraint('Bundle', $settings['target_bundle']);
      // Set any further bundle constraints on the target definition as well,
      // such that it can derive more special data types if possible. For
      // example, "entity:node:page" instead of "entity:node".
      $properties['entity']->getTargetDefinition()
        ->addConstraint('Bundle', $settings['target_bundle']);

With the 'target_bundles' now living on the FieldDefinition, we can't add the bundle constraint in ERItem::propertyDefinitions(), and need to add it in ERItem::getConstraints(), and AFAICT that only allows us to add the first constraint (on $properties['entity'], meaning a constraint on $entity->ref_field), and not the second (on $properties['entity']->getTargetDefinition(), meaning a constraint on the TypedData you get by $entity->ref_field->get('entity')). So the test fails.

Changing EntityFieldtest to

-$reference = $reference_field->appendItem(array('entity' => $node))->get('entity');
+$reference = $reference_field->appendItem(array('entity' => $node));
$violations = $reference->validate();
$this->assertEqual($violations->count(), 1);

makes the test pass.

But #2429191: Deprecate entity_reference.module and move its functionality to core does not add the $properties['entity']->getTargetDefinition() constraint either, and is green without changing anything in EntityFieldtest. Beats me for now :-)

So basically, unless @fago or someone else suggests otherwise :
- we do add the Bundle constraint on $properties['entity'] (the constraint that applies to $referrer->reference_field[delta], and is checked when we validate the $referrer entity)
- but I see no way to keep the Bundle constraint on $properties['entity']->getTargetDefinition() (the constraint that applies to the $referrer->reference_field[delta]->get('entity') TypedData, and which I don't really know why it's useful)

That latter constraint is never actually set in HEAD anyway, since :
- it reads the referenceable bundle from the 'target_bundle' storage setting
- that setting is not the one that holds the info for configurable e_r fields, so the constraint is not set for those
- it might only be set for base e_r fields, and those typically never limit to a hardcoded list of bundles...

So I don't think we lose anything by not setting that constraint anymore, since we're never actually setting it anyway (except in the code that explicitly tests it in EntityFieldTest)

With this patch, we at least set the first constraint (the one on $properties['entity'], that is actually used when validating the $referrer entity) correctly for configurable e_r fields, which is not the case in HEAD.

So, the patch should be ready for review :-)

Also, raising to 'Major', because that whole "configurable and non-configurable e_r fields use a different setting for referenceable bundles" really is a WTF :-)

There's tag for that ;)

Nice to see that field storage is bundle-independent with that.
The main question about naming and a way like selection allows override field item constraints.
Current implementation supposed to "Append" constraints so getItemConstraintAditions()
But why external handler class affects field defined constraints???

It looks that constraint should be at item-list-level, so field settings!

Here's a few nits to put in CR and IS

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php
   @@ -78,4 +78,15 @@ public function validateAutocompleteInput($input, &$element, FormStateInterface
   +   * Returns the array of constraints to apply to the Item objects.
   ...
   +  public function getItemConstraints();
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -356,6 +356,27 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
   +  public function getItemConstraints() {
   ...
   +    if (!empty($settings['target_bundles'])) {
   ...
   +      $constraints[] = $constraint_manager->create('ComplexData', [
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -161,6 +152,15 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +  public function getConstraints() {
   ...
   +    return array_merge($constraints, $handler->getItemConstraints());
   

   New method = CR + IS

   this looks like getItemConstraintAditions() in that implementation

2. +++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
   @@ -689,9 +689,9 @@ public function testEntityConstraintValidation() {
        $definition = BaseFieldDefinition::create('entity_reference')
   ...
   -      ->setSetting('target_bundle', 'article');
   +      ->setSetting('handler_settings', ['target_bundles' => ['article' => 'article']]);
   

   this should be pointed in CR
   so contrib update their entities

3. +++ b/core/modules/system/system.install
   @@ -1643,5 +1643,30 @@ function system_update_8007() {
   +function system_update_8008() {
   ...
   +    // Remove 'target_bundle' from settings.
   +    $field_storage->clear('settings.target_bundle');
   +    $field_storage->save(TRUE);
   

   this just removes the setting but needs to update handler settings somehow, is not it?

4. +++ b/core/profiles/standard/config/install/field.storage.user.user_picture.yml
   @@ -19,7 +19,6 @@ settings:
   -  target_bundle: null
   

   yay! field storage is not dependent on bundle!

@andypost : Thanks for the review :-)

Current implementation supposed to "Append" constraints so getItemConstraintAditions()
But why external handler class affects field defined constraints???
It looks that constraint should be at item-list-level, so field settings!

- The way for a field type to specify runtime constraints depending on the field settings is through TypedDataInterface::getConstraints(). This can be done on the ItemList or the Item, depending on where the constraints you want to add should apply. Here, we're talking about constraints about each item so we do it in ERItem::getConstraints(), doing it on ERItemList would make no sense and be more painful to express.

- The actual set of constraints for "the entities referenceable by the ERItem" is actually known by the "selection handler" plugin used by this field : the default SelectionBase handler defines referenceable entities by "allowed bundles" (and in that case we need to add the corresponding Bundle constraint), but the ViewsSelection handler defines them by a View (and there's no constraint we can add here).
So ERItem::getConstraints() actually defers to the "selection handler" to get the constraints.

- Which "selection handler" is used by the field and with wich settings is entirely defined in the ER field-level settings, so yes, in the end the constraints added by ERItem::getConstraints() *are* defined by the field settings.

- Agreed that SelectionInterface::getItemConstraints() is misnamed, as it doesn't return "all the constraints for the Item", but rather "constraints to add to the existing ones". I think it's best to actually change it to "receive the pre-existing (static/default) constraints, and add to them", so that the name is correct. That way, it can also remove constraints if needed (just like Item::getConstraints() can)

Other points :

1. Sure, will add a CR and the beta summary once the patch is reviewed.

2. Yes, but in practice :
- configurable e_r fields already store their "referenceable bundles" in $field_settings['handler_settings']['target_bundles']
- so the change only affects base fields, and those typically very rarely hardcode bundle names in their definition in code

3. See 2. Configurable fields already have the 'target_bundles' setting in the right position within 'handler_settings', this patch is just about removing the "other', unused 'target_bundle' setting. It contains no useful info, so nothing to move, we just have to ditch it

4. :-)

system_update_8008 got added meanwhile, bumping number

interdiff #29 makes a lot of sense!

configurable e_r fields already store their "referenceable bundles" in $field_settings['handler_settings']['target_bundles']

I thought that selection handler can reuse this
otoh the difference is selection handler settings about allowed bundles should be a subset of field setiings

in other issue introduced "auto_create_bundle" - so this way (using selection handler settings) to limit bundles becomes sane

Looks good to me, new methods make sense

Updated the IS.

Also, I realized that the current code in HEAD means we don't validate "target_bundles" when e_r.module is enabled :

// Assuming field_ref only accepts article nodes
$node->field_ref->target_id = $nid_of_a_page_node;
$node->validate($node)->count(); // === 0, no violation, the node validates. 

Arguably makes this a critical bug, added the "Needs D8 critical triage" tag.

CRs:
- https://www.drupal.org/node/2576151 for the settings change
- https://www.drupal.org/node/2576157 for SelectionInterface::getItemConstraints()

The earlier we bump this to critical, the earlier it gets the attention needed to not block RC at the last minute.

All criticals are looked upon by core maintainers and the 'triage' tag is now deprecated.

If we decide it is not critical and should not block release, that is great!

But if we do, it is better to up the count now than to have it sitting around later.

So the arguments for this being critical:

- it could allow certain forms of malicious behaviour on certain sites (say the create permissions on the configured bundle are very restricted and those referenced bundles get into a prominent listing, but instead you can reference a forum topic and get that listed in the same prominent place). Very much depends on configuration of the site.

- it's a data-integrity issue - the field values don't match the field configuration.

- once we're into release candidate, we'd probably need to add some logic to clean up invalid entries which would be complex. Or if we didn't then existing content which would previously save OK would start having validation errors.

The first one I am meh about, the latter two are more of a reason to do this sooner than later.

The patch in general looks good to me on very quick visual review.

Nice. Shouldn't we test system_update_8009() ?

@Fabianx : Thanks, I guess I got shy ;-)

@catch:

It's a data-integrity issue - the field values don't match the field configuration

Yep. We say we let you specify which bundles can be referenced, but in practice we don't enforce that on $entity->validate().
- So REST operations bypass that check completely, you can submit invalid references.
- In entity forms, our current widgets are OK :
The "select" widget is inherently immune (only lets you pick in a list of valid options)
The "autocomplete" widget leverages the '#type' => 'entity_autocomplete', which calls a dedicated SelectionInterface::validateEntityAutocomplete() method in the "selection handler" and sets a form error on non-refereceable entities.
Different widgets might be broken though, unless they implement their own (form-only) checks

@claudiu.cristea : why not I guess. I'll steal that from your patch in #2429191: Deprecate entity_reference.module and move its functionality to core :-)

OK, I was mulling whether all those "is the reference valid according to the field settings ?" checks should rather be done as part of the existing ValidReference constraint (which currently just checks that the target_ids are those of entities that exist in the db).

Turns out, ConfigurableEntityReferenceItem currently *expects* ValidReference to do that check :
- ConfigurableEntityReferenceItem implements OptionsProviderInterface (so they can use the "options" widgets : select dropdown / checkboxes...), and the implementation asks the Selection plugin for the "list of allowed entities".
- So TypedData automatically assigns them an AllowedValuesConstraint, which forbids assigning values that are not on that list
- But ConfigurableEntityReferenceItem::getConstraints() *unsets* that AllowedValuesConstraint (because it would be very memory-ineffective when there are 1000s of referenceable entities), with an explicit comment stating that it's OK because we have the ValidReferenceConstraint
- Except ValidReferenceConstraint doesn't actually check that :-)

Meaning, summarizing the major/critical validation bug in HEAD :

// Assuming field_ref only accepts article nodes
$node->field_ref->target_id = $nid_of_a_page_node;
$node->validate()->count(); // === 0, no violation, major/critical bug

Commenting out the part where ConfigurableEntityReferenceItem::getConstraints() removes AllowedValuesConstraint, fixes the bug :
$node->validate()->count(); // === 1, OK, validation fails as it should

So :
- AllowedValuesConstraint would do the proper validation, but not in a performant way
- There is an expectation that ValidReferenceConstraint would do the right checks, but it doesn't. Maybe it should :-)

Also, the current approach (working at the level of Item constraints) is actually not well-suited for the ViewsSelection handler, that lets you define referenceable entities with a View, and currently doesn't expose any validation constraint either.

A constraint checking that the reference is correct in that case means executing the View, and doing that for each Item independently would be more costly than necessary. As @andypost suggested in #28, a constraint at the ItemList level would in fact be much more effective here.

Same argument applies to the BundleConstraint we currently use, actually, since it single-loads each of the referenced entities.

And in fact, SelectorInterface already has a method to check if a set of entity IDs are valid references : that's validateReferenceableEntities($ids). So asking each Selector to add constraints that fit its logic (possibly having to code custom constraints and validators) would suck, since they already implement that logic in their validateReferenceableEntities().

So that would be :
- Turn ValidReferenceConstraint into a constraint on the ERItemList
- Have ValidReferenceConstraintValidator collect the target_ids across each Item,
call $selector_plugin->validateReferenceableEntities($ids).
and flag a violation on each delta whose target_id is not in the resulting list.

That would result in not actually ever using or setting the 'Bundle' constraint, even for the much more common case of "referenceable entities are defined by bundle".

Thoughts ?

Minor mistake in the sample code that demonstrates the validation bug in the IS

Nits:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -100,16 +96,11 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +      // We can add a constraint for the target entity type. The list of
   

   Let's not use the personal "We can...".

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -161,6 +152,17 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
   +    /* @var \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface $handler */
   

   Missed an asterisk. /** instead of /*.

3. +++ b/core/modules/system/system.install
   @@ -1663,5 +1663,30 @@ function system_update_8008() {
   +    // Deal only with entity reference fields and descendants.
   +    if ($class != $item_class && !is_subclass_of($class, $item_class)) {
   +      continue;
   +    }
   ...
   +    // Remove 'target_bundle' from settings.
   +    $field_storage->clear('settings.target_bundle');
   +    $field_storage->save(TRUE);
   

   This can be compacted? Invert the if condition, remove continue and put the removal inside if() {} but chained:

   if ($class == $item_class || is_subclass_of(...)) {
     $field_storage->clear(...)->save();
   }
   

@yched,

I'll steal that from your patch in #2429191: Burn entity_reference.module and scatter its ashes all over core :-)

LOL! Then you should credit me for that :)

- Have ValidReferenceConstraintValidator collect the target_ids across each Item,
call $selector_plugin->validateReferenceableEntities($ids).

That sounds sensible to me. Can either multiple load those entities, or if necessary add them to a query with IN() depending on the constraint.

Based on #40 and this being restricted to REST and custom widgets, I might consider this major rather than critical, but not changing status for now.

Have ValidReferenceConstraintValidator collect the target_ids across each Item,
call $selector_plugin->validateReferenceableEntities($ids)

I started working on that, the tricky part with that approach is that it doesn't work for new entities referenced on-the-fly ("auto-save" feature of the ER field type)

Still, using the low db cost of validateReferenceableEntities($ids) makes sense for entities that already exist.

So maybe SelectorInterface does need an additional isEntityValid($entity) method, that ValidReferenceConstraintValidator would call on "new entities with no ID". Still means some logic duplication, but easy to implement (and well, the existing methods in SelectorInterface are already about offering different flavors of the "determine valid entities" logic in various shapes optimized for various caller use cases).

Which leads to : ViewsSelector is inherently incompatible with auto-save. You cannot validate that an $entity that isn't in the database yet will match a given View. How fun :-) That would probably need taking into account for the field config UI in #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles. For the validation issue here, I guess ViewsSelector::isEntityValid() would just say "no / not supported" for unsaved entities.

Will post a patch asap.

OK, I started working on the approach from #47 in #2576501: Helper issue for ER validation [#2577963] , but relying on Selection::validateReferenceableEntities($ids) (and thus on EntityQuery) seems to cause weird test fails in the infamous \Drupal\Core\Entity\Query\Sql\Tables::addField() :-/

I still think that this is the correct approach to properly validate entity_ref fields, but I'd really like to have the 'target_bundle' / 'target_bundles' WTF resolved before RC.

Thus I propose we get back to the "short" version in #11, that removes the unused setting and "hardcodes" a minimal check on the bundles in ERItem (which HEAD attempts to do, but using the wrong 'target_bundle' setting - the one that is always empty). This fixes the reference validation bug for the most common cases.

Then we can figure out the larger "how do we make sure that the validation uses the actual same logic than the one implemented in the Selection handler" in a separate issue.

Attached patch does that. No interdiff, this is basically #11 with an upgrade test.

Also, @claudiu.cristea should be credited as well, the update and update test are stolen from his patch in #2429191: Deprecate entity_reference.module and move its functionality to core :-)

- Same facepalm as in #16, some people just never learn :-/
- Fixed the EntityReferenceTargetBundleUpdateTest, which I simplified a bit too much over @claudiu.cristea's version

And left out a test adjustment that was added in #18.
This should be green.

+++ b/core/modules/field/src/Tests/Update/EntityReferenceTargetBundleUpdateTest.php
@@ -0,0 +1,53 @@
+    $configFactory = $this->container->get('config.factory');
+
+    // Add the deprecated 'target_bundle' setting to the 'node.field_tags' field
+    // storage config.
+    /** @var \Drupal\Core\Config\Config */
+    $config = $configFactory->getEditable('field.storage.node.field_tags');
+    $config->set('settings.target_bundle', NULL);
+    $config->save(TRUE);

Better put that into the dump files, everything before running the updates might be really unreliable to change.

Yeah, actually I was surprised to have to do some manipulations in the test to re-add the stale 'target_bundle' to the field.storage.node.field_tags that comes from drupal-8.bare.standard.php.gz.

But that's because this 'target_bundle' entry that we want to remove is in fact not present in yamls for configurable e_r field storages.
ConfigurableEntityReferenceItem::defaultStorageSettings() *removes* it from the list of storage settings (and has done so since #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition in march 2014), so it's not a setting for configurable e_r fields, and thus it is never populated nor saved.

So in fact we don't need an update, and don't need to test it :-)

I'm a bit confused, the standard profile files are configurable fields right? People might have installed from those files, so don't we need an update path for those then?

Updated the IS for the current, scaled-down fix.

Also, if we go with this, the draft CR in https://www.drupal.org/node/2576157 will need to be deleted.

@dawehner #57 : Damn, you're right. Some of the field.storage.* yamls shipped in standard/config do have the old setting, some others don't (field_tags, which I was using in the test, doesnt).

So, never mind #56.
Attached patch re-adds the update and the test, and modifies the test to use field.storage.node.field_image instead.
Interdiff is with #54

Sigh, wrong patch (interdiff was correct). Reuploading both.

And re-added the part about the update in the IS :-/

@amateescu #62 :

So I think we need to revert #56

Yep, done :-) (crosspost...)

... and add a check in the update function to see if the field config storage has the setting or not, and if it does, we need to move it into the settings of all the field configs for that field storage.

Not sure about that. The 'target_bundle', if it was ever present, was completely ignored at runtime, the field behaved as was specified in the field settings ['handler_settings']['target_bundles'], which thus already contains the right target bundles intended for the field.
So IMO we can just ditch the old 'target_bundle' ?

Just reviewed and this looks fine to me.

This is tagged beta target just because there's an upgrade path. I'm hoping to tag a beta early-ish tomorrow.

Since there's no actual data migration in the upgrade path, just dropping the dead key, I'm not particularly worried if this goes in between beta and RC compared to what it might have had to do, so leaving the tab but it's definitely 'target' not 'blocker' for beta.

I don't want to commit this minutes after RTBC, so leaving it in the queue a bit later, but I'll try to re-review later today and commit if no-one beats me to it.

#2064191-55: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled still exists

Making field_update_8001() more compact.

I'm sorry, i'm blind :)

+++ b/core/modules/field/src/Tests/Update/EntityReferenceTargetBundleUpdateTest.php
@@ -0,0 +1,53 @@
+    // Load the 'node.field_image' field storage config, and check that is has
+    // a 'target_bundle' setting.

Minor, can be fixed on commit or ignored: "check that it has". s/is/it/.

@amateescu, IMO it's also more readable :)

Committed/pushed to 8.0.x, thanks!

After committing this, I realised we could probably add an integration test for entityreference module to confirm it doesn't break this, but that might be redundant when [# #2429191] lands. So follow-up if people think it'd be useful until then.

https://www.youtube.com/watch?v=vE8b3qsjxUA

w00t ! So happy to see you go, 'target_bundle' :-)
I'll open a separate issue for the "find a way to more closely connect e_r validation with the Selection plugin" thing.

+1 on @claudiu.cristea's #68 (which is what got committed anyway)
and +1 that #2429191: Deprecate entity_reference.module and move its functionality to core would be *very* nice to get in as well.

I'll open a separate issue for the "find a way to more closely connect e_r validation with the Selection plugin" thing.

#2577963: Let entity_ref Selection handlers be in charge of the field validation

Minor: I found the dead target_bundle setting left in 2 places inside docs and a test assertion message. Opened #2578083: Followup: Clean docs after #2064191 followup.

There was a @todo left in EntityReferenceFieldTest. I'll move it to #2578083: Followup: Clean docs after #2064191.

    // @todo Implement a test case for invalid bundle references after
    //   https://www.drupal.org/node/2064191 is fixed.

@jibran : posted a patch in #2579411: Resolve @todo by adding test in EntityReferenceFieldTest

I published the 'DO NOT PUBLISH' change record ;) ...

Just kidding, published the right one ...

@Fabianx : lol, thanks !
Any way someone can actually delete the stale CR draft at https://www.drupal.org/node/2576157 ?

Deleted the old CR to prevent any confusion.