Problem/Motivation

Views allows you to render a specific entity for example in the header/footer of a view.

This functionality is for example used for "taxonomy/term/..." but is especially helpful for custom help texts above a view.

Sadly it uses the entity ID, for referencing to the entity, so deployment of the view is problematic.
On top of that it is not straightforward to find the UUID

Proposed resolution

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).

We agreed that the best way to address all of the use cases 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()

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.

Remaining tasks

#111 needs review.

Followups

User interface changes

None yet; the patch just changes what is stored when the user enters valid input.

API changes

  • New methods
Files: 
CommentFileSizeAuthor
#111 interdiff-105-111.txt6.49 KBxjm
#111 vdc-entityarea-2341357-111.patch38.27 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,071 pass(es). View
#105 interdiff-101-104.txt6.65 KBxjm
#105 vdc-entityarea-2341357-104.patch37.82 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,033 pass(es). View
#101 interdiff-target-bug.txt1.06 KBxjm
#101 vdc-entityarea-2341357-101.patch35.1 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,940 pass(es). View
#98 interdiff-assert-trait.txt691 bytesxjm
#98 vdc-entityarea-2341357-98.patch35.12 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,914 pass(es). View
#96 2341357-96.patch35.19 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,895 pass(es), 0 fail(s), and 24 exception(s). View
#96 interdiff.txt8.02 KBdawehner
#93 interdiff-92.txt2.08 KBxjm
#92 vdc-entityarea-2341357-92.patch29.52 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,552 pass(es). View
#91 interdiff-logic-duh.txt1.88 KBxjm
#91 vdc-entityarea-2341357-91.patch28.98 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,534 pass(es). View
#89 interdiff-entityarea-no-token.txt3.99 KBxjm
#89 vdc-entityarea-2341357-89.patch28.98 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,508 pass(es), 1 fail(s), and 1 exception(s). View
#88 vdc-2341357-with-2396607.patch39.62 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,197 pass(es). View
#88 interdiff.txt7.47 KBxjm
#88 vdc-entityarea-2341357-88-do-not-test.patch28.37 KBxjm
#80 interdiff-76-80.txt5.89 KBxjm
#80 vdc-2341357-80.patch32.79 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,546 pass(es). View
#77 interdiff.txt10.56 KBxjm
#76 vdc-2341357-76.patch33.24 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,306 pass(es), 1 fail(s), and 0 exception(s). View
#73 view-area-uuid-2341357.73.patch27.86 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,092 pass(es). View
#73 interdiff.txt1.8 KBlarowlan
#71 view-area-uuid-2341357.71.patch28.26 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,074 pass(es), 2 fail(s), and 0 exception(s). View
#71 interdiff.txt3.96 KBlarowlan
#68 interdiff-viewui.txt980 bytesxjm
#68 vdc-2341357-68.patch27.31 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,068 pass(es), 4 fail(s), and 0 exception(s). View
#66 interdiff.txt16.29 KBxjm
#65 vdc-2341357-65.patch26.35 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,195 pass(es), 5 fail(s), and 0 exception(s). View
#64 views-2341357-64-do-not-test.patch4.56 KBxjm
#45 interdiff.txt2.5 KBdawehner
#45 2341357-45.patch20.48 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,154 pass(es). View
#41 interdiff.txt723 bytesdawehner
#41 2341357-41.patch20.55 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View
#33 interdiff.txt2.29 KBdawehner
#23 interdiff.txt3.97 KBdawehner
#23 2341357-23.patch20.51 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,112 pass(es). View
#22 interdiff.txt5.12 KBWim Leers
#21 2341357-21.patch18.23 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,075 pass(es). View
#19 interdiff.txt2.02 KBdawehner
#19 2341357-19.patch16.01 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,085 pass(es). View
#10 2341357-10.patch21.02 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,920 pass(es). View
#10 interdiff.txt7.87 KBdawehner
#7 2341357-7.patch14.27 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,840 pass(es), 17 fail(s), and 0 exception(s). View
#7 interdiff.txt3.37 KBdawehner
#4 2341357-4.patch14.11 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,829 pass(es). View
#4 interdiff.txt5.31 KBdawehner
#1 vdc-2341357-1.patch13.82 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,556 pass(es), 3 fail(s), and 0 exception(s). View

Comments

dawehner’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
13.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,556 pass(es), 3 fail(s), and 0 exception(s). View

Here is a first try. Let's call it major as it helps to deploy views.

Status: Needs review » Needs work

The last submitted patch, 1: vdc-2341357-1.patch, failed testing.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
14.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,829 pass(es). View

Alright, here we go.

dawehner’s picture

Issue summary: View changes

Update the IS.

Wim Leers’s picture

Title: Allow the entity area to use UUIDs instead of IDs and provide autocompletion. » Allow the entity area to use UUIDs instead of IDs and provide autocompletion
Status: Needs review » Needs work

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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
14.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,840 pass(es), 17 fail(s), and 0 exception(s). View

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

Well yeah, we know that the QueryFactoryInterface is the factory for each entity type. In order
to reduce the amount of work done in the constructor, let's store the generic factory.

AFAIK we generally prefix such routes with /system?

I don't care, changed it.

Missing the matching protected property for this.

Fixed.

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

Good idea, let's check first run a UUID validation.

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?

We can't do that, because we allow to use IDs for usecases like "/taxonomy/term/{taxonomy_term}". We use tokens there to fetch the argument from the URL.

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

True.

Wim Leers’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 7: 2341357-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
21.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,920 pass(es). View

... given that I introduced a logic error in the last patch I decided to write a unit test for that file.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
amateescu’s picture

  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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

jibran’s picture

Issue tags: +security

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’s picture

Status: Needs work » Needs review
Related issues: +#2353611: Make it possible to link to an entity by UUID

@amateescu
Okay, I do see your point. Let's remove the autocompletion from this issue then. The UX in HEAD is not worse.

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.

... No I won't. The class is called Entity, so the test should simply be EntityTest, so you can easily switch between class and test. If someone doesn't get namespaces,
we have other problems.

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

See #2353611: Make it possible to link to an entity by UUID

amateescu’s picture

@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?

jibran’s picture

Status: Needs review » Needs work

NW because

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

+++ 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"?

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,085 pass(es). View
2.02 KB

@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?

Oh damn, I forgot to upload the patch.

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

Good idea!

Wim Leers’s picture

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.

dawehner’s picture

FileSize
1.63 KB
18.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,075 pass(es). View

Sure.

Wim Leers’s picture

Status: Needs review » Needs work
FileSize
5.12 KB

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,112 pass(es). View
3.97 KB

Thank you for the quick review ... no idea what happened with the interdiff ...

Alright, let's complete the test coverage ...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Splendid!

amateescu’s picture

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.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

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().

dawehner’s picture

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.

Well, because I just simply don't see the big advantage. Why is loading by UUID a bad idea?

dawehner’s picture

Seriously, ... we don't always have the UUID, in case loading UUID is a problem this should be handled internally by the entity system.

amateescu’s picture

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.

amateescu’s picture

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

dawehner’s picture

@amateescu
The thing is that views allows you to input "!1" which points to the first argument of the view. The argument will be the ID most of the time anyway.
So entity_id_uuid is kinda a good name.
Given that we need code on runtime to handle the converted ID directly.

amateescu’s picture

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

dawehner’s picture

FileSize
2.29 KB

So do we really just talk about the following change?

amateescu’s picture

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.

Wim Leers’s picture

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.

dawehner’s picture

In any case, the ! 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.

Well, just to be clear, this parameter is the usecase for which this currently is useful (taxonomy/term/%). The UUID here is an addition, so I would argue that adding the UUID should not drop the existing feature.

I still don't get why it has to be an UUID and why the current implementation is so horrible.

amateescu’s picture

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.

effulgentsia’s picture

Category: Task » Bug report
Priority: Major » Critical
Issue tags: +D8 upgrade path

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.

xjm’s picture

Title: Allow the entity area to use UUIDs instead of IDs and provide autocompletion » Make Views entity area config deployable with UUIDs

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.

dawehner’s picture

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.

Thank you for summarizing exactly what the patch is doing.

We convert IDs to UUIDs, in case possible. In case we can't know, because its a placeholder, we load the entity using the placeholder resolves ID/UUID.

dawehner’s picture

FileSize
20.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,101 pass(es). View
723 bytes

Had a bit of a discussion with xjm:

To be clear, let's all forgot about this issue first.
HEAD supports you specify "!1" which will pull the UID which is part of the URL, "/foo/{entity_test}" and use it in the area,
to render an entity at the top of a view. This functionality is used for the taxonomy/term/{} view in order to render the entity description at the top.

This patch adds support to also reference an entity by UUID, as you want to have proper deployment functionality.

xjm’s picture

(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?

xjm’s picture

olli’s picture

  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()?

dawehner’s picture

FileSize
20.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,154 pass(es). View
2.5 KB

Thank you for your review!

These need to go too.

Good catch.

Move the comment inside loadEntityByIdOrUuid()?

Fixed

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

Interesting, I wasn't sure and just looked it up in the argument default plugins, and indeed they are still special here.

effulgentsia’s picture

+++ 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.

effulgentsia’s picture

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?

effulgentsia’s picture

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.

effulgentsia’s picture

+++ 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.

amateescu’s picture

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).

effulgentsia’s picture

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?

amateescu’s picture

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.

effulgentsia’s picture

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?

amateescu’s picture

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.

effulgentsia’s picture

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.

dawehner’s picture

Just in general, I do see your pursuit of semanticness for what we actually store in the configuration and how we describe it.
Given that I would certainly like to separate storing the placeholder, from storing the ID/UUID. Using placeholders
here is a separate concept in my point of view, one you would probably not be able to describe properly in generic schema information.

At the moment we don't have any semanticness about UUID being something special, compared to just an arbitrary string.
Given that though I really ask myself, whether describing more, than a pure string is a requirement to get the bug fixed, which is,
that you cannot deploy those views. Adding a proper schema and making the storage more sane seems major for me.

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.

Well, sure, but I think making the UI nice to use, is not and should not be part of this issue, given that you would
always have to start with some form of autocompletion anyway, otherwise finding the IDs is too hard anyway.

Talking about usecase for config entities:
I could really easy imagine usecases like the following, on which we want to point to a config entity: You have config entities which
represent some kind of help text, which should be shown above the view.

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)

Even not directly related, but this reminds me of #2353611: Make it possible to link to an entity by UUID, we often want to have ID and OR UUID.

pfrenssen’s picture

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.

xjm’s picture

Issue tags: +Ghent DA sprint
xjm’s picture

Status: Needs review » Needs work

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

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

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.

dawehner’s picture

This is a no-followup: #2390773: Bubble up View::preSave

xjm’s picture

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.

xjm’s picture

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. :)

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
Issue tags: +Entity Field API, +Needs tests
FileSize
26.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,195 pass(es), 5 fail(s), and 0 exception(s). View

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.
xjm’s picture

FileSize
16.29 KB

Oops, forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, 65: vdc-2341357-65.patch, failed testing.

xjm’s picture

FileSize
27.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,068 pass(es), 4 fail(s), and 0 exception(s). View
980 bytes

Fixes the silly ViewUI fail.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 68: vdc-2341357-68.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +CriticalADay
FileSize
3.96 KB
28.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,074 pass(es), 2 fail(s), and 0 exception(s). View

work on phpunit fails, still not quite there

Status: Needs review » Needs work

The last submitted patch, 71: view-area-uuid-2341357.71.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
27.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,092 pass(es). View
dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -418,4 +418,15 @@ public function getCacheTags();
    +   * entity in other configuration.
    

    That is a bit misleaading if you think about content entitites. What about using "in configuration" instead of "in other configuration"?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -966,6 +966,27 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
    +    if ($entity_type instanceof ConfigEntityType) {
    

    Why do we not use the interface here? $entity_type instanceof ConfigEntityTypeInterface ...

  3. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -60,16 +101,28 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +      // @todo If the entity does not exist or the target identifier is invalid,
    +      //   this will will show the config target identifier or whatever other
    +      //   unvalidated value is present. Is that the correct behavior?
    

    We can do the validation in another isue, given that we did not applied any kind of validation in HEAD as well.

  4. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -82,16 +135,65 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +        // @todo It feels fragile for $options['target'] to be assumed to be
    +        //   two different things depending on whether we are in the form or
    +        //   not. It would also be potentially useful to know and store what
    +        //   the user originally entered. Add a separate key to store that, e.g.
    +        ///  entered_target or something?
    

    Follow up? :P

  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.

xjm’s picture

Assigned: Unassigned » xjm
Issue summary: View changes

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.

xjm’s picture

FileSize
33.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,306 pass(es), 1 fail(s), and 0 exception(s). View

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.

xjm’s picture

FileSize
10.56 KB

Status: Needs review » Needs work

The last submitted patch, 76: vdc-2341357-76.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -966,6 +966,27 @@ public function loadEntityByUuid($entity_type_id, $uuid) {
    +
    +    // For configuration entities, the config target is given by the entity ID.
    +    // @todo Should we allow entity types to return their target identifier key
    +    //   rather than hard-coding this check?
    +    if ($entity_type instanceof ConfigEntityTypeInterface) {
    +      $entity = $this->getStorage($entity_type_id)->load($target);
    +    }
    +
    

    Yeah, somehow ideally I think this should be part of the entity storage ... but given that this would be an API change for itself, moving along and not do that here, seems fine for me.

  2. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -82,16 +134,67 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +      if (isset($target_entity) && (!empty($this->options['bypass_access']) || $target_entity->access('view'))) {
    

    It would be great to file a follow up to get rid of that 'bypass_access' checking. This got introduced when we had an SA, which added access checking but still allow people to opt out, to not introduce regressions.

  3. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -112,4 +112,55 @@ public function tokenizeValue($value) {
    +  public function hasViewsToken($value, $only_token = FALSE) {
    ...
    +  public function hasGlobalToken($value, $only_token = FALSE) {
    ...
    +  public function hasToken($value, $only_token = FALSE) {
    

    Does those methods really have to be public?

  4. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
    @@ -112,4 +112,55 @@ public function tokenizeValue($value) {
    +    // @todo Do we need to check $this->options['tokenize']?
    +    return $this->view->style_plugin->hasViewsToken($value, $only_token);
    ...
    +    return $this->view->style_plugin->hasGlobalToken($value, $only_token);
    ...
    +    return $this->view->style_plugin->hasToken($value, $only_token);
    

    Can we use $this->view->getStyle() instead?

  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

  6. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -244,6 +244,63 @@ public function tokenizeValue($value, $row_index) {
    +      // @todo Should we use something more specific than .* for the contents
    +      //   of the [...]?
    ...
    +      return (preg_match('/^\[.*\]$/', $value) !== FALSE);
    

    Note: We could reuse the regex used by token itself:

        preg_match_all('/
          \[             # [ - pattern start
          ([^\s\[\]:]*)  # match $type not containing whitespace : [ or ]
          :              # : - separator
          ([^\[\]]*)     # match $name not containing [ or ]
          \]             # ] - pattern end
          /x', $text, $matches)
    
  7. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -244,6 +244,63 @@ public function tokenizeValue($value, $row_index) {
    +      // @todo Seems like the token system should actually provide this API.
    

    Absolutely!

  8. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -244,6 +244,63 @@ public function tokenizeValue($value, $row_index) {
    +    return (strpos($value, '[') !== FALSE);
    

    Its kinda odd that we return TRUE for strings like "you say hello, [ and we say goodbye"

  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() ... ?

    Btw. this should fail the ViewUI unit test automatically.

xjm’s picture

Status: Needs work » Needs review
FileSize
32.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,546 pass(es). View
5.89 KB

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.

dawehner’s picture

So what should we do with all these @todos ... We do alread have one follow up to have a broader test coverage for the itself.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Well anyway, let's just ship it :)

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

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

xjm’s picture

Status: Needs review » Needs work

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

xjm’s picture

xjm’s picture

xjm’s picture

Title: Views entity area config is not deployable » Views entity area config is not deployable and missing dependencies
Issue tags: +Triaged D8 critical

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).

xjm’s picture

Status: Needs work » Needs review
FileSize
28.37 KB
7.47 KB
39.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,197 pass(es). View

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.

xjm’s picture

FileSize
28.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,508 pass(es), 1 fail(s), and 1 exception(s). View
3.99 KB

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

Status: Needs review » Needs work

The last submitted patch, 89: vdc-entityarea-2341357-89.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
28.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,534 pass(es). View
1.88 KB

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

xjm’s picture

Assigned: xjm » Unassigned
FileSize
29.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,552 pass(es). View

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

xjm’s picture

FileSize
2.08 KB
xjm’s picture

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.)

.

larowlan’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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.

dawehner’s picture

FileSize
8.02 KB
35.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,895 pass(es), 0 fail(s), and 24 exception(s). View

Added a bit of a test coverage.

Status: Needs review » Needs work

The last submitted patch, 96: 2341357-96.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
35.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,914 pass(es). View
691 bytes

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

xjm’s picture

Status: Needs review » Needs work

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. ;)

xjm’s picture

Issue summary: View changes
xjm’s picture

Status: Needs work » Needs review
FileSize
35.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,940 pass(es). View
1.06 KB

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.

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -60,16 +101,33 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +
    +    // @todo This form appears to be missing some test coverage; no explicit
    +    // fail when the below was broken.
    +
    

    I think we can drop this todo now.

  2. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -82,16 +140,67 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +    // @todo No tests fail even if this never is false; add coverage.
    

    This todo can be dropped now as well

xjm’s picture

Assigned: Unassigned » xjm
FileSize
11.95 KB
29.22 KB
18.88 KB
28.56 KB

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.

dawehner’s picture

Didn't we totally agreed that we don't want to think about the UI representation in this issue?

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

xjm’s picture

Assigned: xjm » Unassigned
Issue summary: View changes
Issue tags: -security, -Needs tests
FileSize
37.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,033 pass(es). View
6.65 KB

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.

xjm’s picture

@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

xjm’s picture

  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’s picture

Issue summary: View changes
larowlan’s picture

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

xjm’s picture

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

xjm’s picture

FileSize
38.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,071 pass(es). View
6.49 KB

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. ??

xjm’s picture

Issue summary: View changes
alexpott’s picture

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.

alexpott’s picture

xjm’s picture

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

xjm’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'm certainly fine with the patch as it is ...

amateescu’s picture

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.

jibran’s picture

I can't speak for the views part

+1 for views changes :)

dawehner’s picture

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

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

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

Well, the idea was more to distinct between config and content entities, not about UUID vs. not UUID. At least this is how I can explain it.

webchick’s picture

Assigned: Unassigned » alexpott

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 9a5eb86 on 8.0.x
    Issue #2341357 by xjm, dawehner, larowlan, Wim Leers: Views entity area...
xjm’s picture

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?

amateescu’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.