Allow multiple target entity types in the entity reference field

swoon

Hard-blocker for #2407505: [meta] Finalize the menu links (and other user-entered paths) system so claiming it as #CriticalADay ;)

So stuck the JavaScript and widget in entity_reference module, like the existing Entity reference widgets.
If you want a widget, you need to enable entity-reference module. If you just want the primitive data type, core is enough.

Added some tests.

Lets see how broken this is.

Nice lunchtime task

Great start.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/DataType/DynamicEntityReference.php
   @@ -0,0 +1,75 @@
   +  public function setValue($value, $notify = TRUE) {
   

   this is same as parent.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
   @@ -0,0 +1,231 @@
   + *   list_class = "\Drupal\Core\Field\EntityReferenceFieldItemList",
   

   Should be DER.

3. +++ b/core/modules/system/src/Tests/Entity/EntityUnitTestBase.php
   @@ -179,7 +179,7 @@ protected function refreshServices() {
   -      $id = $string ? $this->randomMachineName() : mt_rand(1, 0xFFFFFFFF);
   +      $id = $string ? $this->randomMachineName() : mt_rand(1, 0xFFFFFF);
   

   This is not related.

Fixes #9, that test passes locally so no sure what's up there.
Looks like db-updates need to run?

Screenshot from stark

can't work the fail out, hoping someone else can

Hm. This isn't necessarily a hard-blocker to #2407505: [meta] Finalize the menu links (and other user-entered paths) system, but was originally listed as a possible example of how we'd implement the UI. Before we go down any particular implementation path, IMO we really should do a round brainstorming, mockups, etc. building on the previous thinking on this at #1123662: Participants did not know how to get the path while creating a menu link and #1101600: Users need to be able to select from list when adding menu items to a menu.

Peter, is this the issue you intended that brainstorming to take place? If so, we probably need a new issue that doesn't have a patch and is more of a brainstorming/discussion piece.

IMHO DER would give us a basic tool we could use as a initial prototype. Whether its the solution at the end
is not sure, but its something we can use in the transition phase, without have to worry about further UI bits.

Wow, that is quite some code, but not that different to entity references.

Here is a quick review.

1. +++ b/core/lib/Drupal/Core/Field/DataDynamicReferenceDefinition.php
   @@ -0,0 +1,37 @@
   +/**
   + * A typed data definition class for defining dynamic references.
   + */
   +class DataDynamicReferenceDefinition extends DataReferenceDefinition {
   

   It would be nice to explain why we need to define a custom data reference definition (I guess its because we want to provide a string for each entity type, right?)

2. +++ b/core/lib/Drupal/Core/Field/DataDynamicReferenceDefinition.php
   @@ -0,0 +1,37 @@
   +    /* @var $definition \Drupal\Core\TypedData\DataReferenceDefinition; */
   

   Afaik we use it the other way round, much like @param type $var

3. +++ b/core/lib/Drupal/Core/Field/DynamicEntityReferenceFieldItemList.php
   @@ -0,0 +1,132 @@
   +/**
   + * Defines a item list class for dynamic entity reference fields.
   + */
   +class DynamicEntityReferenceFieldItemList extends EntityReferenceFieldItemList {
   +
   

   It would be also nice to explain why we need a custom field item list.

4. +++ b/core/lib/Drupal/Core/Field/DynamicEntityReferenceFieldItemList.php
   @@ -0,0 +1,132 @@
   +          $entity_ids = \Drupal::entityQuery($target_type)
   +            ->condition('uuid', $uuids, 'IN')
   +            ->execute();
   

   According to #2389335: Deprecate entity.query service and replace with using the entity storage's getQuery() method we should just use the storage to get an entity query.

5. +++ b/core/lib/Drupal/Core/Field/DynamicEntityReferenceFieldItemList.php
   @@ -0,0 +1,132 @@
   +  public function defaultValuesFormSubmit(array $element, array &$form, FormStateInterface $form_state) {
   +    // We want to bypass the EntityReferenceItem::defaultValuesFormSubmit()
   +    $default_value = FieldItemList::defaultValuesFormSubmit($element, $form, $form_state);
   

   SO this doesn't work, see : http://3v4l.org/ioZq3

6. +++ b/core/lib/Drupal/Core/Field/DynamicEntityReferenceFieldItemList.php
   @@ -0,0 +1,132 @@
   +    $all_ids = array();
   

   Just in case you want to convert to [] in all new added files.

7. +++ b/core/lib/Drupal/Core/Field/Plugin/DataType/DynamicEntityReference.php
   @@ -0,0 +1,48 @@
   +      $entity = entity_load($target_type, $this->id);
   

   Can we at least use \Drupal::entityManager()->getstorage->load...

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
   @@ -0,0 +1,231 @@
   + * - exclude_entity_types: Allow user to include or exclude entity_types.
   

   If you can do both, why not name it available_entity_types or something more generoc?

9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
   @@ -0,0 +1,231 @@
   +    $properties['entity'] = DataDynamicReferenceDefinition::create('entity')
   ...
   +      ->setComputed(TRUE)
   +      ->setReadOnly(FALSE);
   

   Ideally this properties would be inherited from the DataDynamicReferenceDefinition ... given that it should now these two already, don't it?

10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
    @@ -0,0 +1,231 @@
    +        'length' => EntityTypeInterface::BUNDLE_MAX_LENGTH,
    

    It should be EntityTypeInterface::ID_MAX_LENGTH, yes its the same value, but ...

11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
    @@ -0,0 +1,231 @@
    +      // We have to bypass the EntityReferenceItem::setValue() here because we
    

    Should we maybe use "here, because"? At least I would do it like that.

12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
    @@ -0,0 +1,231 @@
    +        $key = $target_entity_type instanceof ConfigEntityType ? 'config' : 'content';
    ...
    +            if ($entity) {
    +              $dependencies[$key][] = $entity->getConfigDependencyName();
    +            }
    

    We do have now $entity->getConfigDependencyKey()

13. +++ b/core/modules/entity_reference/js/dynamic-entity-reference-widget.js
    @@ -0,0 +1,53 @@
    +  Drupal.behaviors.dynamicEntityReferenceWidget = {
    

    It would be helpful to describe it quickly what the behaviour is, we attach here. In other words, WHY, do we need some custom JS

14. +++ b/core/modules/entity_reference/src/EntityReferenceController.php
    @@ -99,4 +100,61 @@ public function handleAutocomplete(Request $request, $type, $field_name, $entity
    +   *   (optional) The entity ID the entity reference field is attached to.
    +   *   Defaults to ''.
    

    Mh, the optionality does not map the signature, ... not sure what we do here.

15. +++ b/core/modules/entity_reference/src/EntityReferenceController.php
    @@ -99,4 +100,61 @@ public function handleAutocomplete(Request $request, $type, $field_name, $entity
    +    if (!isset($definitions[$field_name])) {
    +      throw new AccessDeniedHttpException();
    +    }
    

    Sounds for me like a NotFoundHttpException ...

16. +++ b/core/modules/entity_reference/src/EntityReferenceController.php
    @@ -99,4 +100,61 @@ public function handleAutocomplete(Request $request, $type, $field_name, $entity
    +    if ($field_definition->getType() != 'dynamic_entity_reference' || !$access_control_handler->fieldAccess('edit', $field_definition)) {
    

    Can we pass the $account from the controller in?

17. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -0,0 +1,208 @@
    + *   id = "dynamic_entity_reference_default",
    ...
    +class DynamicEntityReferenceWidget extends AutocompleteWidget {
    

    What about name it "entity_reference_dynamic_autocomplete" to be more in sync with the parent?

18. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -0,0 +1,208 @@
    + *   field_types = {
    + *     "dynamic_entity_reference"
    + *   }
    + * )
    

    Nitpick: Just in case you want to, we support trailing commas now. This is just one of possible implementations.

19. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -0,0 +1,208 @@
    +      '#attributes' => array(
    +        'class' => array('dynamic-entity-reference-entity-type'),
    +      ),
    

    I'm curious what modern JS standards say, should we use classes here or data attributes to attach the behaviour?

20. +++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/DynamicEntityReferenceWidget.php
    @@ -0,0 +1,208 @@
    +    $target_type_info = \Drupal::entityManager()->getDefinition($target_type);
    

    Widgets can use DI, can't they?

@webchick Menu links are going to need a field type that can reference any entity type. While entity reference is generic, each field definition is tied to a specific type - so you can configure a field storage to reference nodes OR taxonomy terms, but not nodes AND taxonomy terms at the same time.

This is because entity reference stores the entity type in the field configuration and there's no entity type in the schema (see https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Field!Plugin!Fiel...).

With dynamic entity reference the entity type is in the schema, and it allows for entity types to be excluded, but otherwise any entity type on the system is fair game. (http://cgit.drupalcode.org/dynamic_entity_reference/tree/src/Plugin/Fiel...).

So while the specific UI that DER provides isn't a hard blocker (although it's also available now without a lot of work), the field type itself is.

This is because entity reference stores the entity type in the field configuration and there's no entity type in the schema.

I'd like to note that this is true only for the current code in D8 HEAD and D7 contrib. The entity reference field in D7 initially provided a 'target_type' schema column with this exact goal in mind (dynamic entity reference), but it didn't work out due to Views integration, something related to views relationships not being able (or maybe invalid SQL) to join multiple entity base tables from a single field table, but I don't remember the exact details.

something related to views relationships not being able (or maybe invalid SQL) to join multiple entity base tables from a single field table

For DER I tried to create the views integration in #2321721: Provide a views relationship for each dynamic entity reference field it worked as it turned out views missing some functionality #2378729: JoinPluginBase doesn't allow extra conditions on left table. The patch just need reroll+views config schama once #2378729: JoinPluginBase doesn't allow extra conditions on left table is in.

So I was wondering about whether we should merge the two field types, but #21 and #22 suggest that could be tricky, useful to know!

Not being able to use views for this doesn't feel like a blocker at all - the core use-case only requires basic EntityFieldQuery (find the menu links referencing X entity) and entity loading (load the entity referenced by Y menu link) - so something we could add separately.

Form #19 @dawehner thank you for the review. If a quick review consist of 20 points then I am wondering what would be in depth review look a like. :D. Here are some replies.

1. Yeah need fix.
2. It is same as parentDataReferenceDefinition so I think it also needs a fix.
3. Yeah need fix.
4. Nice catch
5. So how can we call parent parent then?
6. I think this is out of the scope here. We can fix this for all the core once we agree to do that.
7. Yeah need fix.
8. so you want to invert the logic here by changing the name? Sounds good to me. It is there because @larowlan design the module to only exclude the entity types so when I created this option I went with this name to avoid the confusion but for core of course we can fix that.
9. Well EntityReferenceItem::propertyDefinitions() is strongly dependent on ER field settings which are different for DER field settings so I don't see a possible way to use parent method.
10. This is same as parent.
11. I wrote that comment but I am no expert here perhaps @larowlan re-write it better.
12. Yeah this need fix. Already fixed for parent.
13. Yeah need fix.
14. It is optional cuz we are setting it NULL in routing file. We can modify function signature here.
15. Well it is same as EntityReferenceController::handleAutocomplete()
16. it is same as EntityReferenceController::handleAutocomplete()
17. this is a good suggestion but this is the only formatter so naming it then like this did make sense but of course we can rename it.
18. ok
19. Is that issue fixed yet? We can update it if data- issue is fixed.
20. This is same a parent.

We can use views with DER. I had a working patch before I found those issues.
For more DER screenshots please see #2331331: Widget for cardinality 1 is missing the field label. Here is the screenshot form seven for both single field and multi value field.

Gotcha, thanks all for the clarification. I'll update the issue summary at #2407505: [meta] Finalize the menu links (and other user-entered paths) system accordingly.

tagging for re-roll during office hours today

So, having read #20 and #21, I'm not sure why we need to add an additional field type to core, rather than building on the functionality of ER. That seems to move us in the wrong direction, IMO.

Probably worth noting that comment module has entity_id and entity_type in it's schema - which could re-use whatever we come up with here.

Yes, I should have been more explicit myself, sorry :) #21 should read more as: I would prefer if we can bake this functionality into the existing ER field type rather than a separate non-configurable field which has to override quite a bit of assumptions/code from the existing one.

On the other hand, I think I have a viable alternate solution for this entire problem space that I will finish writing soon and post it in the parent meta issue.

Irc conversation

There's a der normalizer in the contrib project if needed

Note I asked @tstoekler to have a look at this issue in case there's something he can suggest w.r.t. the exception

I think we need DynamicEntityReferenceItem::defaultFieldSettings() here.

Fixed #42 and some minor adjustments. Thanks @pwolanin for fighting with test bot to fix the fails.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
@@ -0,0 +1,232 @@
+ *   constraints = {"ValidReference" = {}}

This should be Valid DynamicReference IMO

Fixing #43 in #2405607: Fix DER constraint validation it is not a blocker imo

no longer a blocker

Adding related issue.

Is this still being considered for core? Especially considering parent issue is going with just node entity type for now*

* #2418017: Implement autocomplete UI for the link widget : Resolution

I think it could, but at this point I'd be inclined to mark it a feature and postpone to 8.1.x. Other opinions?

Also note that there's also #2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element

Since this issue is no longer critical, untagging for critical office hours. Don't know if we have or should start a "Major Office Hours" or similar tag. Leaving the "CriticalADay" tag, since maybe that concept still applies to work done while an issue was critical at some point.

So since this turned out not to be a blocker for the menu link work, I think this is actually a feature request?

I think this is correct let's focus at #2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element and #2427803: [PP-1] Expose target type, selection handler and selection settings in LinkWidget for now.
I'll keep on improving dynamic entity reference in contrib and I'll make sure we can replace entity reference completely with dynamic entity reference without BC break that's why I moved it to 8.1.x instead of 9.0.x.

@larowlan @jibran is the goal still for 8.1?

Patch from #46does not apply any more (which is kinda obvious for a patch that's a year old, but just tagging as needs reroll).

#2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element would need to happen first.

Parent issue has been resolved, so re-opening this.

#2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element would need to happen first.

Seems like the data type should come before the form widget, no?
ie. #2423093 should be postponed on this issue, and not vice versa.

bump

this is important guys :P

Seems like the data type should come before the form widget, no?
ie. #2423093 should be postponed on this issue, and not vice versa.

No, @amateescu had it right.

#2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element is about the form element, which is used by the field widget but is more generic.

You could argue that this issue could address the field type only, and then a followup (postponed on both #2423093: Allow multiple target entity types in the 'entity_autocomplete' Form API element and this issue) could address the widget, but as no one is actually working on any of these that's of dubious value.