Views entity area config is not deployable and missing dependencies

Looking good :) Manually tested, works perfectly.

Also: is there a reason we must support IDs at all? Why can't this support UUIDs only? Because users might know the ID by heart and might want to enter that?

1. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   + * Provides a generic autocompletion service,
   

   s/autocompletion/entity autocompletion/
   s/,/./

2. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +   * @var \Drupal\Core\Entity\Query\QueryFactory
   

   Why not QueryFactoryInterface?

   (We encountered this same problem in #2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference also, so this is more a genuine question than a review remark.)

3. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +   * Constructs a UserAutocomplete object.
   

   s/UserAutocomplete/EntityAutocomplete/

4. +++ b/core/modules/system/system.routing.yml
   @@ -456,3 +456,10 @@ system.admin_content:
   +  path: '/entity_autocomplete/{entity_type}'
   

   AFAIK we generally prefix such routes with /system?

5. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -28,6 +30,36 @@ class Entity extends TokenizeAreaPluginBase {
   +    $this->entityManager = $entity_manager;
   

   Missing the matching protected property for this.

6. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -82,16 +118,32 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      // Try to load by ID and then by UUID.
   +      if (($entity = $entity_storage->load($entity_id_uuid)) || (($entities = $entity_storage->loadByProperties(['uuid' => $entity_id_uuid])) && ($entity = reset($entities)))) {
   

   I wonder if we can't figure out whether the value is a UUID and then not try both anymore?

7. +++ b/core/modules/views/src/Tests/Handler/AreaEntityTest.php
   @@ -84,6 +85,25 @@ public function testEntityArea() {
   +    // Specific a UUID instead of the entity ID.
   

   s/Specific/Specify/

8. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -65,11 +98,14 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      '#title' => $this->t('ID / UUID'),
   

   What about "Entity ID or UUID"

9. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -65,11 +98,14 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      '#description' => $this->t('You can put in either the entity ID or the UUID'),
   

   I think we can omit this description, with the slight title change, it seems entirely redundant?

Only one nit left now:

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -136,7 +143,8 @@ public function render($empty = FALSE) {
       // Try to load by ID and then by UUID.
-      if (($entity = $entity_storage->load($entity_id_uuid)) || (($entities = $entity_storage->loadByProperties(['uuid' => $entity_id_uuid])) && ($entity = reset($entities)))) {
+      $is_uuid = Uuid::isValid($entity_id_uuid);
+      if ((!$is_uuid && $entity = $entity_storage->load($entity_id_uuid)) && ($entities = $entity_storage->loadByProperties(['uuid' => $entity_id_uuid])) && ($entity = reset($entities))) {

The comment now no longer matches the logic.

1. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +  public function getMatches($string, $entity_type_id) {
   +    $matches = [];
   +    if ($string) {
   +      $entity_type = $this->entityManager->getDefinition($entity_type_id);
   +      $entity_storage = $this->entityManager->getStorage($entity_type_id);
   +      if (!($entity_label_key = $entity_type->getKey('label'))) {
   +        return [];
   +      }
   +      $entity_ids = $this->entityQuery->get($entity_type_id)
   +        ->condition($entity_label_key, $string, 'CONTAINS')
   +        ->accessCheck(TRUE)
   +        ->range(0, 10)
   +        ->execute();
   +      $entities = $entity_storage->loadMultiple($entity_ids);
   +      foreach ($entities as $entity) {
   +        /** @var \Drupal\Core\Entity\EntityInterface $entity */
   +        if ($entity->access('view')) {
   +          $matches[] = ['value' => $entity->uuid(), 'label' => $entity->label()];
   +        }
   +      }
   +    }
   +
   +    return $matches;
   +  }
   

   I hope you realize that this

   1) is a security vulnerability (it will return entities a user doesn't have access to because ->accessCheck(TRUE) doesn't actually do anything)

   and 2) it will return incomplete results (it won't return the anonymous user for example).

   If doing a generic entity autocomplete system would've been this simple, don't you think we would've removed all those ER selection plugins by now?

2. +++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
   @@ -0,0 +1,224 @@
   +class EntityTest extends UnitTestCase {
   

   Can we rename this class to something more than EntityTest? It will be quite annoying to bump into it in an IDE autocompletion when you're actually looking for the entity_test class.

And some constructive feedback (replying to the "Proposed resolution" in the IS):

Allow to store the UUID and use it to render the references entity

For this you need to do a ID <-> UUID mapping like EntityReferenceFieldItemList is doing in processDefaultValue() and defaultValuesFormSubmit().

Add a generic autocomplete implementation which works beyond fields (entity reference)

We have #2107243: Decouple entity reference selection plugins from field definitions and #1959806: Provide a generic 'entity_autocomplete' Form API element for that.

+++ b/core/modules/views/config/schema/views.area.schema.yml
@@ -8,7 +8,7 @@ views.area.entity:
       label: 'ID'

UUID :)

And can we have a content dependency added to the view too? Thanks.

Here are some minor issue.

1. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +   * Get matches for the autocompletion of entity labels.
   

   Gets

2. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +   * @param $entity_type_id
   

   type missing.

3. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +      if (!($entity_label_key = $entity_type->getKey('label'))) {
   

   Is this even possible?

4. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +        ->accessCheck(TRUE)
   

   I don't think this is enough. Yeah see #12.1. I think we should postpone this on #2107243: Decouple entity reference selection plugins from field definitions and #1959806: Provide a generic 'entity_autocomplete' Form API element

5. +++ b/core/lib/Drupal/Core/Entity/EntityAutocomplete.php
   @@ -0,0 +1,80 @@
   +        /** @var \Drupal\Core\Entity\EntityInterface $entity */
   

   It should be above and it should be for $entities like this.
   /** @var \Drupal\Core\Entity\EntityInterface[] $entities */

I have a question, will it become /taxonomy/term/{uuid}?

@dawehner, apparently you're implying that I don't get namespaces? Well, I don't have any answer for that, do what you want.

What exactly needs review here?

NW because

Let's remove the autocompletion from this issue then. The UX in HEAD is not worse.

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -65,11 +106,13 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      '#title' => $this->t('Entity ID or UUID'),

Is it possible to use the entity type label here instead of "Entity"?

Tiny nitpick that can be fixed on commit:

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -82,16 +126,35 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      // Validate whether the ID is a UUID, in that case load the entity as UUID

s/as/by/

But what about this more important part, that Alex Pott remarked in #13:

And can we have a content dependency added to the view too? Thanks.

The interdiff is wrong, but the patch is right. Here's the right interdiff for #21.

So close, just one remark left:

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -82,16 +126,65 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+    // Ensure that we don't add dependencies for placeholders.
+    if (strpos($this->options['entity_id_uuid'], '!') === FALSE) {

We don't have test coverage for this yet.

Splendid!

I still don't understand why we're going with this "it can be either an ID or an UUID" (based on what? phase of the moon?) approach instead of doing it properly like I suggested in #12.

Sorry, I forgot about that.

So this is the part that needs either to be answered or resolved:

For this you need to do a ID <-> UUID mapping like EntityReferenceFieldItemList is doing in processDefaultValue() and defaultValuesFormSubmit().

Well, because I just simply don't see the big advantage.

Is consistency too much to ask for?

Why is loading by UUID a bad idea?

Who said loading by UUID is a bad idea? What I'm saying is that we need to enter only IDs in the UI and automatically convert and store them as UUID in config.

Seriously, ... we don't always have the UUID

When do we not have a UUID for an entity, either content or config? That would be a bug.

in case loading UUID is a problem this should be handled internally by the entity system.

Again, there's no problem with loading by UUID, the only problem is being consistent with that we enter in the UI and with what gets stored in config.

Oh, wait, maybe I don't understand the current approach because I don't get namespaces.

Ok, I don't have anything else to add to this discussion.

I'm talking about never storing an entity ID in config, only the UUID. I can't tell from a quick look at the patch if that interdiff is the only thing needed.

In any case, the !<arg> case needs to be handled separately somewhere and if what we get from config doesn't start with '!', it must to be an UUID and handled as such.

This also means that the name should be more like entity_uuid_or_param.

This also means that the name should be more like entity_uuid_or_param.

This is a good point. The current patch essentially is entity_id_or_uuid_or_arg ("param" is wrong, "arg" is what you mean). If we'd only support UUIDs, then it'd indeed become entity_uuid_or_arg.

Where did I say anything about dropping the existing feature? I just said that we should only store UUIDs or the current !<arg> thing in config and process them differently on load/runtime if config gives us something that starts with "!" or not.

I also didn't say the current implementation is horrible, not sure where you got this impression? Maybe it's because that "I don't get namespaces" stuff really stepped on my tail.

I think not being able to do a config deployment is a critical bug. And since the fix requires changing the config structure, that makes it an upgrade path blocker. I haven't yet gotten up to speed with the patch to provide a meaningful review, but am hoping to do so when I get a chance, unless this gets fixed before then.

I think we should only store UUIDs in Views, anywhere, because every content dependency in a view needs to be deployable and so storing a serial entity ID breaks the principles of the configuration system. However, whether the user enters a serial ID, UUID, entity label, or whatever when configuring their view is probably worth discussing. I think the minimum to do here is to ensure that all serial IDs get converted to the UUIDs of the current site when the View is configured (are there others remaining beside the entity area handlers?). Then, we can look into usability improvements to how the user chooses the referenced entity in separate issues if needed.

(This is a crosspost with #41)

So I'm a little slow on the uptake apparently, but @dawehner just explained this to me. Apologies to everyone who already understood this -- the !1 placeholder has nothing to do with any particular serial ID; rather, it's a placeholder for a views argument available at runtime (e.g. for the taxonomy/term/% pages as @dawehner points out above). The reason that it's a ! and not a % is so that it's run through strip_tags() (see in ViewExecutable::_buildArguments().)

I can see the case for renaming the key since the current name confused me too. And yeah the interdiff in #33 seems to make sense, if the goal is to ensure only a UUID get saved. But OTOH my brain cannot parse what the old code was doing. ;)

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_area.yml
@@ -21,7 +23,7 @@ display:
-          entity_id: '1'
+          entity_id_uuid: '1'

Per @dawehner the fact that this was still a 1 was simply a bug in the patch. It is fixed in #41. I wonder why no tests failed in #23?

1. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -60,16 +101,19 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      '#autocomplete_route_name' => 'system.entity_autocomplete',
   +      '#autocomplete_route_parameters' => ['entity_type' => $this->entityType],
   

   These need to go too.

2. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -82,16 +126,65 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +  public function submitOptionsForm(&$form, FormStateInterface $form_state, &$options = []) {
   

   $options is not passed here, you need &form_state->getValue('options') or something.

3. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -82,16 +126,65 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      // Validate whether the ID is a UUID, in that case load the entity by UUID
   +      // otherwise use the ID.
   +      if ($entity = $this->loadEntityByIdOrUuid($entity_id_uuid)) {
   

   Move the comment inside loadEntityByIdOrUuid()?

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -60,16 +101,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+    $form['entity_id_uuid'] = [
+      '#title' => $this->t('@entity_type_label ID or UUID', ['@entity_type_label' => $label]),
       '#type' => 'textfield',
-      '#default_value' => $this->options['entity_id'],
-    );
+      '#default_value' => $this->options['entity_id_uuid'],
+    ];
...
+  public function submitOptionsForm(&$form, FormStateInterface $form_state) {
+    parent::submitOptionsForm($form, $form_state);
+
+    // Always try to store the UUID, if possible.
+    $options = $form_state->getValue('options');
+    if ($entity = $this->entityManager->getStorage($this->entityType)->load($options['entity_id_uuid'])) {
+      $options['entity_id_uuid'] = $entity->uuid();
+    }
+    $form_state->setValue('options', $options);
+  }

With this approach, the behavior of the patch is that:
- Edit a View, add a "Global: Rendered entity - Custom Block" to the Header, type in the ID (e.g., "1").
- Submit the form, and then go back to edit that header item. Now, what's visible in the UI is the UUID.

I think I agree with @amateescu that that's not desired, and that a possible way of fixing that is to have two separate keys in $options: 'entity_id' and 'entity_uuid', just as we do in the schema of field.value.entity_reference within core.data_types.schema.yml. Even if the UI problem becomes moot once we use an autocomplete element, I think the code flow would be easier to follow with two separate keys.

Additionally, the patch does the ID to UUID conversion for config entities as well. For example, do the example from #46 with a "Global: Rendered entity - Block" and "bartik_powered". Do we need/want that, or is it better for config references to stick to ID?

a possible way of fixing that is to have two separate keys in $options: 'entity_id' and 'entity_uuid', just as we do in the schema of field.value.entity_reference

Building on that idea,

+++ b/core/modules/views/config/schema/views.area.schema.yml
@@ -8,9 +8,9 @@ views.area.entity:
-    entity_id:
+    entity_id_uuid:
       type: string

What if we changed this to:

    entity_reference:
       type: field.value.entity_reference

So $options would look like $options['entity_reference']['target_id'] and $options['entity_reference']['target_uuid']. Or, if we don't like a non-field using a field value's schema type, we could make a new core data type (e.g., entity_reference) and have both field.value.entity_reference and views.area.entity.entity_reference extend it.

I wonder if doing that would also assist with the issues linked at the bottom of #12 and using those elements for this use case.

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -82,16 +124,68 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        $dependencies['content'][] = $this->entityType . ':' . $entity->bundle() . ':' . $entity->uuid();

Per #47, this can be either a config dependency or a content dependency; we can't assume only content.

a possible way of fixing that is to have two separate keys in $options: 'entity_id' and 'entity_uuid', just as we do in the schema of field.value.entity_reference

@effulgentsia, I'm sorry but I don't think that will be of much help here and neither for the two issues linked in #12, one is about the implementation of ER's selection plugins, which don't have any knowledge of entity IDs or UUIDs, and the other one is just about code (Form API), so config is not involved in neither of them. It seems that my point didn't quite get across so I'll try to explain again how I see a good resolution for this issue.

Let's start with (what I hope is) the desired outcome:

- in the "entity area thingy" the user can enter
1) an entity ID
2) an entity UUID
3) an "argument placeholder" (!<arg>)

- when the view is saved in config, for the cases above we need to
1) convert the entity ID to its UUID
2) and 3) do nothing (pass-through)

Now, when the user comes back to edit the view/header area, they will see
1) an entity ID, which can be different from the initial one if the view is edited on a different environment
2) an entity ID, converted from the UUID entered initially
3) the same argument placeholder

Note that in point 2) above, there is a change like the one you noticed, but reversed (UUID -> ID) since we store both 1) and 2) as UUIDs and we cannot know if the user entered an ID or UUID in the first place.

Therefore, since the desired outcome is to never store an ID in config, but only a UUID or an argument placeholder, I still think a single config entry named entity_uuid_or_arg is enough for this use case.

Also, we can decide that the user can not enter a UUID at all since we don't have that possibility anywhere else in D8's user interface as far as I know, so the possibly confusing outcome of 2) is no longer a problem.

Does that explanation make sense?

We will also need a validation step to ensure that the ID (or UUID if we decide we want 2)) is actually valid for the given entity type (I have no idea if this is already implemented, I just thought it needs to be mentioned).

Therefore, since the desired outcome is to never store an ID in config

Per #47, is that true in the case that we're referencing a config entity?

Per #47, is that true in the case that we're referencing a config entity?

Yes, I think that #47 will complicate things without adding any benefit. The user will only see the ID in the UI anyway.

Yes, I think that #47 will complicate things without adding any benefit.

What about default config, shipped for example with an install profile? For example, let's say the Standard profile wants to ship with a View that puts a block into one of its areas? That block doesn't have a UUID yet.

I still think a single config entry named entity_uuid_or_arg is enough

If we want a single entry, then I would change that to entity_uuid_or_token, since tokenizeValue() can handle more than just URL arguments. However, per above, that name wouldn't cover the case of referencing a config entity by ID. So, what about simply entity?

What about default config, shipped for example with an install profile? For example, let's say the Standard profile wants to ship with a View that puts a block into one of its areas? That block doesn't have a UUID yet.

The same case can be made for a content entity as well, for example the 'Home' menu link, so I'd say in this case the view will need to go through the same "when the view is saved in config" step from #50, which means the implementation will have to be somewhere else than a form submit handler.

So, what about simply entity?

That sounds fine, what I'm really against here is having 'id' in the name, based on the premise that we'll never store IDs.

The same case can be made for a content entity as well, for example the 'Home' menu link

Sorry to bring up old wounds, but the Standard profile's 'Home' menu link is not a content entity.

There's no support in core to ship default content. A profile can add some content entities in its hook_install(), in which case it then has a UUID to work with and can update its installed config accordingly. But nowhere else where config references config do we require it to deal with a UUID and the resulting need for programmatic interaction as part of default config.

So, I still think that the config key we're discussing in this issue needs to be able to support the following value types:
- a string with tokens (e.g., "!1")
- a UUID when referencing a content entity
- an ID when referencing a config entity (I could be convinced against this, but haven't been yet)

Therefore, if we want a single key that can be any of the above, I propose we name it entity.

Also, we can decide that the user can not enter a UUID at all since we don't have that possibility anywhere else in D8's user interface as far as I know, so the possibly confusing outcome of 2) is no longer a problem.

Yes, I agree with this. I don't think we should ever display a UUID in the UI, except in places like the config diff modal.

Now, when the user comes back to edit the view/header area, they will see...an entity ID, converted from the UUID entered initially

Yes, so then the question becomes where in the code to do that conversion. In the form? Or somewhere within the onLoad() flow? I think the conversion should be symmetrical. So, if we do ID -> UUID during form submit, then we should do UUID -> ID during form build. But, if we do ID -> UUID during config save, then we should do UUID -> ID during config load.

Summarizing #46-#55:

There is consensus on the following next steps to take for this patch:

• Rename the config parameter entity_id_uuid to entity - proposed in #53, confirmed in #54.
• Do not show the UUID in the interface, or allow the user to enter a UUID. Instead always store the UUID for content entities, and always show the ID of the entity in the current environment to the user - proposed in #50, confirmed in #55.

Still to be discussed:

• Shall we use UUIDs for config entities? What if a default config entity (eg a block defined in an install profile) is referenced?
• Where shall the UUID->ID and ID->UUID conversion take place? In the form submit handler / form builder, or when loading / saving the config of the view?

edit: this does not include @dawehner's input from comment #56.

We discussed #57 with @amateescu, @alexpott, @dawehner, @bircher, and myself. Writing up now what we concluded...

Discussed with @alexpott, @amateescu, @dawehner, myself, and intermittently @bircher. ER and Views (and probably other modules) have a shared usecase here to:

• Store a deployable reference in their configuration to a specific entity.
• Reference either a content or configuration entity with the same API.
• For content entities, allow the user enter a serial ID that is then converted to a deployable ID (UUID).

Currently, ER handles this in EntityReferenceFieldItemList::defaultValuesFormSubmit() like so:

foreach ($default_value as $delta => $properties) {
  unset($default_value[$delta]['target_id']);
  $default_value[$delta]['target_uuid'] = $entities[$properties['target_id']]->uuid();
}

So the target_id key is used in the user-facing form to provide the entity ID, and then converted to a UUID before it is saved via the Entity API. @alexpott noted that this doesn't support default configuration (which does not have a UUID) -- i.e., a module cannot provide both an entity reference field and a default value for it in its configuration. To fix that, we can move this logic into the pre-save operation. Edit: see #63 and #2390729: Update ER's ID to UUID default value conversion code to use the new "getTarget()" API.

@alexpott also pointed out that in general it's much better to store and rely on the config ID for configuration entities (rather than the UUID). Since it can be a UUID ID or a config ID, we decided it would make the most sense to just name the key it target (rather than entity, entity_id_or_uuid, target_uuid, target_id... or whatever).

We agreed that the best way to address all of the above would be to actually add some methods in the Entity API that ER, Views, or whatever other implementing code can rely on:

EntityInterface::getConfigTarget()
Will provide the the correct way to store the value of a target entity--i.e. the UUID for content entities, or the config ID for configuration entities. As elsewhere, the Entity base class will provide the default implementation, and ConfigEntity will override it.

EntityManagerInterface::loadByConfigTarget()
Loads the entity from the string provided by getConfigTarget() (both for rendering and for preparing the form?). Would include this check:
$key = $target_entity_type instanceof ConfigEntityType ? 'config' : 'content' currently found in EntityReferenceItem::calculateDependencies()

Related issue: #2390615: Add method to determine config dependency key depending on entity type

For this issue, we'll:

1. Add the two methods.
2. Rename the Views key to 'target'.
3. Convert the logic in Drupal\views\Plugin\views\area\Entity::render() (and ::buildOptionsForm()?) to use the new load by target method.
4. Somehow-or-other allow the View to get the target for the entity during pre-save (instead of in submitOptionsForm() as in the current patch). Edit: see #63.

Once we add these methods, there will also be some followup in ER (see #2390729: Update ER's ID to UUID default value conversion code to use the new "getTarget()" API) to use them there as well.

For another followup issue: There appears to be no validation for the value of the taxonomy term ID field. You can enter term IDs that don't exist. You can enter placeholders that aren't available. It happily accepted elephant as a taxonomy term ID. It accepted !this%is!not%legit. Etc. :) This is an existing bug in HEAD (so out of scope here), and probably pervasive elsewhere in Views.

I'll start incorporating this in the patch.

Additional discussion with @dawehner, @alexpott, @amateescu -- the bit about moving functionality into preSave() instead of submit handlers actually becomes a non-issue once we store the config ID instead of UUIDs. So #2390773: Bubble up View::preSave is no longer related, and #2390729: Update ER's ID to UUID default value conversion code to use the new "getTarget()" API becomes about simply converting to the new methods rather than also moving the logic out of the submit handlers.

Here are initial implementations for the new methods. One thing that occurred to me is that it doesn't feel quite right to hard-code the instanceof check in the entity manager -- we could instead supply a method gets the entity key for the target ID--UUID key in the content entity base implementation, and ID key in the config entity base implementation. Then load by that key regardless. But I've not incorporated that here yet--want to get it working first. :)

Working patch (in that it worked when I tested it in the UI, and the integration tests pass). Still to do:

1. Fix the unit test failures.
2. Add test coverage for the new Entity API methods.
3. Several @todos in the patch for some code improvements. Note particularly the one about whether we should store the user's input separately from the target config.
4. Review existing test coverage and possibly add test coverage for new scenarios based on the new logic.

Oops, forgot the interdiff.

Fixes the silly ViewUI fail.

work on phpunit fails, still not quite there

Thanks @larowlan and @dawehner.

Filed #2392823: [meta] Much Views UI input is not validated and #2392833: Entity area handler input is not validated as followups for validation. Also updated the issue summary a bit (still could use some work).

I'll improve the patch some more.

Some work. Addresses #74 points 1-3, and one of the @todo. I also broke a unit test. Might have actually broken the functionality; didn't test it manually yet.

Re: #74.4, I discussed this with both @alexpott and @amateescu, and they said they were comfortable with the behavior, so I've just removed the @todo.

Re: #74.5:

+++ b/core/modules/views/src/Plugin/views/area/Entity.php
@@ -82,16 +135,65 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      if (strpos($this->options['target'], '!') === FALSE) {
+        if ($entity = $this->entityManager->loadEntityByConfigTarget($this->entityType, $this->options['target'])) {
+          $target_entity = $entity;
+        }
+      }
+      else {
+        $target_id = $this->tokenizeValue($this->options['target']);
+        if ($entity = $this->entityManager->getStorage($this->entityType)->load($target_id)) {
+          $target_entity = $entity;
+        }
+      }

Its actually kinda sad that loadEntityByConfigTarget does not work if you specify the entity ID of an content entity and you need to special case it here.

I actually don't think that the entity system should support that; it'd be a bit magical. The load method should match what's returned from the getter, and calling code can be explicit about what it expects and what it doesn't. Discussed with @alexpott and he agreed. So I've left this as is.

Re: #79.5:

+++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
@@ -244,6 +244,63 @@ public function tokenizeValue($value, $row_index) {
+  public function hasViewsToken($value, $only_token = FALSE) {

it is a little bit odd to have singular here, but meh

I actually think the singular is correct -- we are checking that it has at least one token, and the method can also check that it has only one token. So I've left these with singular names.

Re: #79.9:

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -1135,6 +1135,12 @@ public function getConfigDependencyName() {
+  public function getConfigTarget() {
+  }

Poor ViewUI, nobody likes oyu :) ... shouldn't that be return $this->getConfigTarget() ... ?

Fixed. There are also a couple other config-related methods on ViewUI that do this in HEAD (which is what I was copying). I'll file a followup to fix those.

Attached also addresses #79.4, which additionally actually solved another bug on form build similar to the one in the dependency calculation with the style plugin not being initialized. (@alexpott helped me fix the unit tests.) Still to do: the various @todo, file followups for #79.1-2 and the stuff related to #79.9. For #79.3 and 6-8, I may actually spin these new methods out into their own separate patch so that we aren't creeping scope quite so much. For now, though, here's a working patch. I'll work on this more later.

Sorry this issue took multiple different directions and needs an issue summary update.

Yeah it's also not done yet. Sorry... should have set it NW. Haven't had time to finish this past week.

Spun out #2396607: Allow Views token matching for validation (and remove dead code) as per #80.

This will need an update for #2342287: Allow Twig in Views token replacement but doing so in #2396607: Allow Views token matching for validation (and remove dead code).

Discussed with @alexpott, @effulgentsia, @catch, and @webchick. This issue is critical because of the missing configuration dependencies for this handler. The content reference problem by itself is not critical, per #2248369-20: [meta] Deploying configuration that depends on content, but needs to have a compatible solution (which this issue will provide).

Here's the patch rerolled on top of the current patch in #2396607: Allow Views token matching for validation (and remove dead code). The -do-not-test.patch contains the patch for this issue. Working on #80 (and #83) now.

The rock that is #2396607: Allow Views token matching for validation (and remove dead code) has turned out to have many creepy-crawlies underneath, so removing the soft block on that issue and adding a @todo to remove 12 strpos() later. :P

Filed #2412891: Let config entity methods be tested on ViewUI per #80 and fixing a silly logic error.

Fixing another bug in the patch. Looks like we definitely need to expand the integration tests here...

Okay, so here is what needs to be done on this issue still:

1. Update the issue summary. The proposed resolution is in #61.
2. Manual testing: Test combinations of the following:
   • A view with an entity area handler that references a content entity.
   • A view with an entity area handler that references a config entity.
   • A view with an entity area handler containing an arg token for a content entity type.
   • A view with an entity area handler containing an arg token for a config entity type.
   • Deploying a view where the entity does already exist on the target site, for both a config and content entity.
   • Deploying a view where the entity does not already exist on the target site, for both a config and content entity.

   One usability question from the patch to test as part of that:

   +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -60,16 +101,33 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    if (strpos($this->options['target'], '{{') === FALSE && strpos($this->options['target'], '!') === FALSE && strpos($this->options['target'], '%') === FALSE) {
   +      // @todo If the entity does not exist, this will will show the config
   +      //   target identifier. Is that the correct behavior?
   +      if ($target_entity = $this->entityManager->loadEntityByConfigTarget($this->entityType, $this->options['target'])) {
   +        $target = $target_entity->id();
   +      }
   +    }
   

3. Integration test coverage for the Views parts of the functionality. There are several @todo in the patch. Specifically we should add test coverage for:
   • Ensuring that we don't add dependencies for placeholders, and that we do for legit config and content entities. (Test coverage for entities that are invalid/don't exist can be added in #2392833: Entity area handler input is not validated.)
   • An entity area handler referencing a config entity (CRUD, functionality).
   • The entity area handler configuration form (that could be a followup if it's not part of the above.)

.

IS update
Putting my hand up for the integration tests, but won't likely be until Monday 26th before I get a chance to start them.

WebTestBase and KernelTestBase both already have the trait (thanks @alexpott).

I did a bunch of manual testing and... dah dah DUNH... found a bug.

1. Create a taxonomy term.
2. Go to admin/structure/views/add.
3. Create a view named "entityarea test" with the default page display.
4. Add a header -> "Global Rendered entity - Taxonomy term".
5. Enter 1 in the "taxonomy term ID field.
6. Save the view.
7. Visit entityarea-test. You should see your term displayed at the top of the content listing.
8. Visit admin/config/development/configuration/single/export and select your view. You should see that the "target" is properly the UUID of the entity.
9. Edit the view and enter an argument placeholder !1. in the header's taxonomy term ID configuration instead. Save the view.
10. Visit entityarea-test. There should be no taxonomy term rendered at the top of the view, because there is no argument to match the placeholder.
11. Visit admin/config/development/configuration/single/export and select your view. You should see that the "target" is properly !1.
12. Edit the view again and save 1 in the header's taxonomy term ID configuration instead.
13. Visit entityarea-test Expected behavior: Rendered taxonomy term at the top of the listing. Actual behavior: No rendered taxonomy term at the top of the listing.
14. Visit admin/config/development/configuration/single/export and select your view. Expected behavior: The target is the UUID of the term. Actual behavior: The target is the serial ID 1.
15. Edit the view header again and save a second time. This time, the term UUID is properly saved to the target and the view works again.

So far I've only reproduced this when changing from an argument placeholder to a term ID, not when changing between different term IDs. So something in the view is stale or something when saving... needs further debugging. And then a test for whatever the underlying bug is, and then a fix for said bug. ;)

Attached resolves #99. The bug was that the submit method was checking the previous value of the target field, not the new one in form state, and so failing to load the entity in that case when the previous value had a token.

So to add test coverage the test view could simply save first with a token value, then resave with an existing content entity ID, and assert that the correct value is then saved to the view. The test would fail with the attached interdiff reverted, and pass with it included.

Alrighty, I've done thorough testing adding, editing, and deploying a content entity area handler using both the term ID and an argument placeholder. The dependency management works like it is supposed to. For example, if I first create a view on the test site referencing a taxonomy term that exists only on the dev site:

The term is properly displayed at the top of the view.

Then deploy it to prod:

No term at the top of the view, and it correctly falls back to displaying nothing in that area handler.

Then, if I create a term with a matching UUID on prod:

And then the matching prod term is displayed at the top of the view.

This works fine for core and will support content deployment in contrib. The question is whether or not we want to display the UUID to the user there when it is not available on the site, or something else. I'll filed a followup issue for that. (It's surely going to apply elsewhere in Views where we have content entity dependencies.)

Argument placeholders are also handled correctly. I didn't test global tokens. Invalid term IDs fall back to the same behavior as unavailable UUIDs:

Next I'm going to manually test config entities. Also expanding the test coverage a bit and removing @todos that are no longer needed.

Attached expands the UI test coverage to catch all the clever ways I broke the options form during the course of this issue. I've also confirmed the calculateDependencies() test coverage is sufficient to catch the previous bugs, removed various no-longer-relevant @todos, filed #2392833: Entity area handler input is not validated, and updated the issue summary. I plan to do a bit more manual testing for config entities and maybe for non-argument tokens, but I think this is close and probably ready for a final round of review.

@dawehner, re: #104, yes, that's why I said I was filing a followup. :) Just posting screenshots to prove, well, that it actually works. You can, actually for real, deploy the view, and then add the content entity to the target site with the same UUID but a different serial ID, and it works.

If you are shocked by a UUID but not by the entirety of a views UI, you have quite a good life.

lol

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -981,6 +982,30 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    //   https://www.drupal.org/node/2341357
   

   Note to self: Add this issue to the summary.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -981,6 +982,30 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    // @todo We need to add test coverage for references to config entities as
   +    //   nothing failed when this first condition was broken.
   

   Oops, forgot to check whether this was still the case or not. This @todo can probably go as well. I'll test this tomorrow. Edit: Or we might need to add a tidbit to a unit test.

@xjm - so does that mean the 'remaining tasks' in the IS can be updated to remove the manual testing?
Thanks

@larowlan, I updated the manual testing to list what I still haven't done (and will do on my flight today). Cheers!

I thoroughly tested config entity targets, field tokens, and global tokens. I found a couple bugs, which are fixed in the attached:

1. Drupal\Tests\views\Unit\Plugin\area\EntityTest doesn't cover rendering and calculating dependencies with a config ID, but the integration test in Drupal\views\Tests\Handler\AreaEntityTest does. Good enough, so I've removed that @todo.
2. Another @todo in the patch was referencing this issue instead of #2412983: Consider adding a method to indicate the entity type's target identifier key, so I've fixed that.
3. As of #88, global tokens were broken. Fixed in attached, with test coverage.
4. There is a bug in HEAD where the UI lists the wrong render tokens in the handler options form following #2342287: Allow Twig in Views token replacement (out of scope here; fix in #2396607: Allow Views token matching for validation (and remove dead code)). However, if you enter the correct Twig-compatible tokens (e.g. {{ title }}) instead of what's suggested in the UI, both HEAD and this patch work fine. I added unit test coverage for the render tokens as well.

I also tested deploying with config entity targets. So far as I can tell, a missing config dependency for the view... has no effect whatsoever on the deployment. ??

There is no checking (yet) that all dependencies exist during configuration import - we only use dependencies to order the import. If the dependency does not exist it has no effect on ordering.

Created #2416109: Validate configuration dependencies before importing configuration to do #113

Hooray, thanks @alexpott. That means everything for this patch has been very thoroughly tested and appears to be in working order. :)

I can't speak for the views part, but I found a couple of small issues on the entity side :/

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -981,6 +982,28 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
   +    // @todo Consider adding a method to allow entity types to indicate the
   +    //   target identifier key rather than hard-coding this check. Issue:
   +    //   https://www.drupal.org/node/2412983.
   +    if ($entity_type instanceof ConfigEntityType) {
   +      $entity = $this->getStorage($entity_type_id)->load($target);
   +    }
   +
   +    // For content entities, the config target is given by the UUID.
   +    else {
   +      $entity = $this->loadEntityByUuid($entity_type_id, $target);
   +    }
   

   We could use Drupal\Component\Uuid\Uuid::isValid() here instead of the hardcoded check.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -470,6 +470,26 @@ public function getFormModeOptions($entity_type_id, $include_disabled = FALSE);
   +   * @return \Drupal\Core\Entity\EntityInterface|FALSE
   +   *   The entity object, or FALSE if there is no entity with the given UUID.
   

   Why do we return FALSE instead of NULL? Also, the $target param is not always a UUID :)

   Same for the exception below.

I can't speak for the views part

+1 for views changes :)

Sounds like amateescu's feedback was addressed? Assigning to alexpott for sign-off because it looks like he's been pretty involved here.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9a5eb86 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
index 31d6868..bb89281 100644
--- a/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
+++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
@@ -299,7 +299,6 @@ public function testCalculateDependenciesWithUuid() {
   public function testCalculateDependenciesWithEntityId() {
     $this->setupEntityManager();
 
-    $uuid = '1d52762e-b9d8-4177-908f-572d1a5845a4';
     $entity = $this->getMock('Drupal\Core\Entity\EntityInterface');
     $entity_type = $this->getMock('Drupal\Core\Entity\EntityTypeInterface');
     $entity->expects($this->once())

Remove unused variable on commit.

Re: #118, right, the idea is to ensure that config entities use their ID rather than their UUID. Because they have valid UUIDs too. That's why I suggested an API to make it up to the entity type to indicate what key corresponds to its target identifier. We can discuss more in #2412983: Consider adding a method to indicate the entity type's target identifier key?

The reason for that is that we inherit the behaviour from EntityManager::loadEntityByUuid()

@dawehner, you told me a couple of weeks ago in a different issue that if HEAD has some bad code, it doesn't mean new code has to be bad too (in this case, documentation). I suppose this principle got lost somewhere on the way? :P

Opened #2417339: Fix @return documentation for EntityManagerInterface::loadEntityByConfigTarget() to fix the docs.

@xjm, I'm not sure we actually want to enforce that behavior, but you're right, let's continue over there.