Problem/Motivation

The views EntityField field plugin used for displaying data from entity fields does not support computed fields. Adding support for this is really easy and would be useful in core and beyond, so lets do it.

Proposed resolution

Update the EntityField plugin to support computed fields.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#63 interdiff-63.txt750 bytesamateescu
#63 2852067-63.patch13.32 KBamateescu
#58 interdiff.txt1.76 KBamateescu
#58 2852067-58.patch13.32 KBamateescu
#58 2852067-58-test-only.patch11.11 KBamateescu
#47 interdiff.txt2.65 KBSam152
#47 2852067-computed-field-support-47.patch13.35 KBSam152
#45 interdiff-38-45.txt4.3 KBseanB
#45 2852067-computed-field-support-45.patch15.43 KBseanB
#38 interdiff-36-38.txt3.36 KBseanB
#38 2852067-computed-field-support-38.patch15.54 KBseanB
#36 interdiff-32-36.txt4.16 KBseanB
#36 2852067-computed-field-support-36.patch14.67 KBseanB
#32 2852067-computed-field-support-32.patch12.87 KBSam152
#32 interdiff.txt767 bytesSam152
#24 2852067-computed-field-support-24.patch12.85 KBSam152
#24 interdiff.txt770 bytesSam152
#21 2852067-computed-field-support-21.patch12.81 KBSam152
#21 interdiff.txt2 KBSam152
#16 2852067-computed-field-support-15--test-only.patch11.1 KBSam152
#15 2852067-computed-field-support-15.patch12.81 KBSam152
#15 interdiff.txt1.59 KBSam152
#11 2852067-computed-field-support-11--test-only.patch10.64 KBSam152
#11 2852067-computed-field-support-11.patch12.34 KBSam152
#11 interdiff.txt6.02 KBSam152
#8 2852067-computed-handler-8.patch14.13 KBSam152
#8 interdiff.txt562 bytesSam152
#3 2852067-computed-handler-3.patch14.68 KBSam152
#2 2852067-computed-handler-2.patch3.1 KBSam152
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Here is the proof of concept used from the issue linked in the follow up. Needs stacks of work, but seemed to validate this was a possibility.

Sam152’s picture

Status: Active » Needs review
FileSize
14.68 KB

Before going too much deeper on the implementation front, here is an approach to testing this which should verify the existing basic handler is working.

Status: Needs review » Needs work

The last submitted patch, 3: 2852067-computed-handler-3.patch, failed testing.

dawehner’s picture

I would have guessed that all you need is to have a formatter which outputs the computed property and be done with it. Sadly I don't get why we need an additional handler here.

Sam152’s picture

You can create a computed field of any type, so a new formatter isn't what is required. The issue is that the EntityField handler makes assumptions about the field types it can display, making it impossible to use with computed fields.

The only way to achieve it right now is to create a new views handler for each type of computed field. node_path is an example of this in core.

Sam152’s picture

I'm not actually sure this is going to be possible in a generic way for ALL fields. I've found that:

  • To allow the user to configure a formatter + settings we need to know the type of field it is.
  • To figure out the type, we need some kind of field definition.
  • We can't get a storage definition because computed fields don't have normal field storage.
  • We can get the actual field definition, because that all depends on the bundle. Different bundles different field definitions and of course views show multiple bundles in the same list (could you actually have the same field ID on two bundles with a different field type if it was a computed field? Haven't checked, but it would almost seem possible from what I've seen).
  • We can't get simply grab it as a base field definition, because I think that's an assumption, it could be a bundle field.

So the two options are: make this something that works for computed base fields only or ditch this in favour of simple field plugins for the different computed fields that exist.

Personally I think the former could still be useful, it reduces the amount of work required when using a computed field with views for most cases.

Sam152’s picture

Status: Needs work » Needs review
FileSize
562 bytes
14.13 KB

I suspect this will fix the tests.

I think it still makes sense to proceed with this, but would be good to get some outside input.

dawehner’s picture

`You can create a computed field of any type, so a new formatter isn't what is required. The issue is that the EntityField handler makes assumptions about the field types it can display, making it impossible to use with computed fields.

Do you mind elaborating this? The field handler though also has a lot of performance optimizations etc. so it might be worth fixing the handler instead of creating a new one.

Sam152’s picture

I'll write up a summary of what breaks in the EntityField handler. I went down the path of fixing it a few times, then backed out each time because it didn't seem feasible.

Sam152’s picture

My issue I faced in the past was that computed fields didn't have a storage definition, so everything which relied on that broke. As it turns out BaseFieldDefintions also implement EntityStorageDefintionInterface, so it can fit comfortably in the place of normal field storage for the purposes of the handler. The result is: we can support computed fields with a minor change to \Drupal\views\Plugin\views\field\EntityField. Awesome!

Attached are these changes.

Sam152’s picture

Title: Add a "computed" views field handler for rendering computed fields » Add support for rendering computed fields to the "field" views field handler

The last submitted patch, 11: 2852067-computed-field-support-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2852067-computed-field-support-11--test-only.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
12.81 KB

The view needed a resave after the handler was updated.

Sam152’s picture

Status: Needs review » Needs work

The last submitted patch, 16: 2852067-computed-field-support-15--test-only.patch, failed testing.

Sam152’s picture

Status: Needs work » Needs review
jibran’s picture

Category: Feature request » Task

I don't think this a feature request. I think this is something which we missed while writing EntityField plugin. It is a task if not a bug imo. Overall patch look good to me with complete test coverage. Just some document improvements needed imo.

  1. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
    @@ -321,17 +321,24 @@ public function clickSort($order) {
    +    if (isset($this->definition['field_name']) && isset($field_storage_definitions[$this->definition['field_name']])) {
    ...
    +    elseif (isset($this->definition['entity field']) && isset($field_storage_definitions[$this->definition['entity field']])) {
    ...
    +    elseif (isset($this->definition['field_name']) && isset($base_fields[$this->definition['field_name']])) {
    

    Can we please explain all these conditions?

  2. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
    @@ -321,17 +321,24 @@ public function clickSort($order) {
    +      // Add support for base field definitions which are compatible with
    +      // storage definitions, for fields which are computed and do not have
    +      // storage.
    

    Can you please expand comment by adding the examples for a computed field? Something like "e.g. path field". I can understand the fix because there is a test but it is difficult to understand when you are only reading the code.

  3. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.computed_field_view.yml
    @@ -0,0 +1,171 @@
    \ No newline at end of file
    

    EOF missing.

  4. +++ b/core/modules/views/tests/src/Kernel/Handler/ComputedFieldTest.php
    @@ -0,0 +1,56 @@
    +namespace Drupal\Tests\views\Kernel\Handler;
    +use Drupal\entity_test\Entity\EntityTestComputedField;
    

    Space missing between namespace and use statement.

Sam152’s picture

Status: Needs review » Needs work

Thanks for the review, will look at these.

Sam152’s picture

Status: Needs work » Needs review
FileSize
2 KB
12.81 KB

Feedback addressed. Re: point 1, the linked @todo followup actually explains why those are there. Legacy reasons only, should be removed in the next release.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

dawehner’s picture

  1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
    @@ -0,0 +1,44 @@
    +/**
    + * Test entity class.
    

    It'd be nice to have some quick helpful comment here :)

  2. +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestViewsData.php
    @@ -16,6 +16,17 @@ class EntityTestViewsData extends EntityViewsData {
    +    if ($this->entityType->id() === 'entity_test_computed_field') {
    +      $views_data['entity_test_computed_field']['computed_string_field'] = [
    +        'title' => t('Computed String Field'),
    +        'field' => [
    +          'id' => 'field',
    +          'default_formatter' => 'string',
    +          'field_name' => 'computed_string_field',
    +        ],
    +      ];
    +    }
    

    Here is a question: Is there a reason views could not expose those fields automatically? All the information should be available to expose those fields, I guess?

Sam152’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
770 bytes
12.85 KB

I think exposing it automatically is probably follow-up territory, but I think it would be possible. Maybe a BC break for people who expect computed fields not to be exposed by views?

Added a better comment to the entity class.

dawehner’s picture

Maybe a BC break for people who expect computed fields not to be exposed by views?

What exactly is the BC break there? What wasn't there can't break. Maybe I'm missing something obvious.

Sam152’s picture

Yeah, I can only come up with contrived examples. People with custom entity types, with computed fields which should never be exposed in a UI? If you set displayConfigurable => false, it would still show up in the fields list. Perhaps the expectation is that all fields would exist in the views fields list, but that hasn't been the case.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

> Is there a reason views could not expose those fields automatically?

I don't think we should do that. The 'entity' is a computed property in ER field and it doesn't make sense to add entity property automatically because we can render traget_id as an entity. We have also seen with path field that there is a blurry line between computed and custom storage fields.

Maybe, a good option would be to make it configurable by adding it to basefield definition which definitely comes in a follow-up territory.

Also, we still have to figure out argument, filter, expose filter, and sorting for computed fields.

I think we should add a change notice for this so that people will now about this.

Patch still looks good to me so back to RTBC.

jibran’s picture

Add good feature would be to add the views support for extra fields i.e https://www.drupal.org/project/extrafield_views_integration.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2852067-computed-field-support-24.patch, failed testing.

Sam152’s picture

Status: Needs work » Reviewed & tested by the community
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -321,17 +321,24 @@ public function clickSort($order) {
+    elseif (isset($this->definition['field_name']) && isset($base_fields[$this->definition['field_name']])) {
+      // Add support for base field definitions which are compatible with
+      // storage definitions, for fields which are computed (such as
+      // moderation_state) and do not have storage.
+      $field_storage = $base_fields[$this->definition['field_name']];
+    }

Since getBaseFieldDefinitions() returns FieldDefinitionInterface[] and $field_storage presumably implements FieldStorageDefinitionInterface we should instead do $field_storage = $base_fields[...]->getFieldStorageDefinition()

Sam152’s picture

Status: Needs work » Needs review
FileSize
767 bytes
12.87 KB

Good catch. BaseFieldDefinition implements FieldStorageDefinitionInterface directly, but I think this makes more sense.

Sam152’s picture

@tstoeckler, any chance of another review?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, missed this one. Looks good to me now, thanks!

seanB’s picture

It seems the same problem can occur in \Drupal\views\FieldAPIHandlerTrait for getFieldStorageDefinition(). I ran in to this issue using a computed entity reference field in a Search API view.

It seems related to this issue. Since \Drupal\views\Plugin\views\field\EntityField uses the FieldAPIHandlerTrait, can't we just move the changes to getFieldStorageDefinition() to the trait and remove the method from EntityField?

seanB’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.67 KB
4.16 KB

Added change mentioned in #35.

Sam152’s picture

I don't think we should do that. The implementation in EntityField contains a backwards compatibility fix that accepts both "entity field" and "field_name". We should keep that BC as narrowly scoped as possible to prevent the inconsistency from propagating to other classes.

seanB’s picture

Removed the BC fix in getFieldStorageDefinition() from \Drupal\views\FieldAPIHandlerTrait.

Sam152’s picture

So, this does make sense, but I wonder about the scope. It's an untested part of the patch now.

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to go back to RTBC with the caveat that there is an option between #32 and #38. I think it makes sense for #38 to be a follow up, but happy for either to land.

larowlan’s picture

+++ b/core/modules/views/src/FieldAPIHandlerTrait.php
@@ -45,16 +45,31 @@ protected function getFieldDefinition() {
       $this->fieldStorageDefinition = $field_storage_definitions[$this->definition['field_name']];

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -316,24 +316,37 @@ public function clickSort($order) {
+      $this->fieldStorageDefinition = $field_storage_definitions[$this->definition['field_name']];
...
+      $this->fieldStorageDefinition = $field_storage_definitions[$this->definition['entity field']];

lets return in all of these and avoid the three elseifs - aiding readability

dawehner’s picture

+++ b/core/modules/views/src/FieldAPIHandlerTrait.php
@@ -45,16 +45,31 @@ protected function getFieldDefinition() {
+    $base_fields = $this->getEntityManager()->getBaseFieldDefinitions($entity_type_id);

I'm wondering whether its worth to rewrite the code to not fetch the basefields, unless needed, by moving it into the if statement.

Sam152’s picture

Status: Reviewed & tested by the community » Needs work

NW for those two points.

seanB’s picture

Status: Needs work » Needs review
FileSize
15.43 KB
4.3 KB

Addressed #42 and #43. Since we now use separate if statements, the code to fetch the base fields is now only executed when we need it. It is not in the actual if statement since this would make the if statement really long and harder to read.

Sam152’s picture

I still think the changes in FieldAPIHandlerTrait are out of scope and should be added in another issue.

Sam152’s picture

@seanB, thanks for working on this, but if this issue has a chance, it has to have a clear scope. I've removed the changes to the trait and the changes introduced to EntityField which don't make sense to me (not sure where "$this->fieldStorageDefinition" from). If they don't fail the tests, they are out of scope.

Please do open a follow up to describes what the changes to trait actually does and why.

Sam152’s picture

Issue summary: View changes
seanB’s picture

\Drupal\views\Plugin\views\field\EntityField overrides getFieldStorageDefinition() defined in \Drupal\views\FieldAPIHandlerTrait. The issue is that getFieldStorageDefinition() doesn't support computed fields. Solving this only for \Drupal\views\Plugin\views\field\EntityField feels like a partial solution, since there are a bunch of other views plugins that use the trait.

I've noticed the issue in \Drupal\views\Plugin\views\filter\EntityReference, but I guess the other plugins using \Drupal\views\FieldAPIHandlerTrait probably have the same issue (this is an assumption which I haven't tested yet!).

Still prefer to solve this in a generic way, but if the only way to get this committed is to create an issue for each plugin that has the issue, then I guess that is what we need to do.

Sam152’s picture

I think if you can put forward a case for the change to the trait in a follow-up as well as a test to prove why it's necessary, then no reason not to change it. Lets just not put forward any false pretense that it's required to fix this issue or is covered by the tests this issue introduces.

Please read Issue scope guidelines for Drupal core issues

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rjacobs’s picture

Adding link to related issue #2392845: Add a trait to standardize handling of computed item lists. Issues with views support and computed fields are referenced multiple times in that issue, but punted for follow-up... so this issue might serve, at least partially, as that follow-up.

Sam152’s picture

Both issues seem to introduce a computed field for the purposes of testing. I think if that one gets in first, this will need to be rerolled against it, to make use of the same testing classes.

Sam152’s picture

Anyone want to pick this up again for review? The actual chunk of code introduced is very small. The rest of the patch is centered around testing, which has no api impact and can be updated if computed fields undergo api improvements.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good to me. Do you think we need a change notice? Just a nit.

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -316,24 +316,32 @@ public function clickSort($order) {
+    $field_storage_definitions = $this->getEntityManager()->getFieldStorageDefinitions($entity_type_id);
...
+    $base_fields = $this->getEntityManager()->getBaseFieldDefinitions($entity_type_id);

Entity manger can be a local variable now.

timmillwood’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
    @@ -316,24 +316,32 @@ public function clickSort($order) {
    -    $field_storage_definitions = $this->entityManager->getFieldStorageDefinitions($entity_type_id);
    +    $field_storage_definitions = $this->getEntityManager()->getFieldStorageDefinitions($entity_type_id);
    

    Why the change?

  2. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
    @@ -316,24 +316,32 @@ public function clickSort($order) {
    -    if (isset($this->definition['field_name'])) {
    ...
    +    if (isset($this->definition['field_name']) && isset($field_storage_definitions[$this->definition['field_name']])) {
    ...
    -    elseif (isset($this->definition['entity field'])) {
    ...
    +    if (isset($this->definition['entity field']) && isset($field_storage_definitions[$this->definition['entity field']])) {
    

    These look reasonable, but it seems like the old code should have been causing notices for undefined indexes in some cases. Is there coverage for that too?

  3. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
    @@ -316,24 +316,32 @@ public function clickSort($order) {
    +    // definitions, for fields which are computed (such as moderation_state)
    

    I really don't think moderation_state needs to get a shoutout here. Also, the lack of oxford comma combined with the parenthetical comment makes the whole sentence unclear.

For the next patch, can someone also post an expected fail patch (all the test changes, without the actual fix)? Thanks!

This should be easily re-RTBC-able, I will keep an eye on it.

amateescu’s picture

Title: Add support for rendering computed fields to the "field" views field handler » Add support for rendering computed fields to the "field" views field handler
Version: 8.5.x-dev » 8.4.x-dev
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
11.11 KB
13.32 KB
1.76 KB

Re #57:

  1. At first I thought this change was made for consistency, but AFAIS we don't use this method for getting the entity manager anywhere else in this class so I changed it back.
  2. I don't know if there are any pre-existing tests for that code, but the added safety checks surely can't break them :)
  3. That comment was pretty hard to understand so I rewrote it.

Here's an updated patch with a test-only one as well.

The last submitted patch, 58: 2852067-58-test-only.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks for the changes! And the clear red/green patch, what a sight :)

Sam152’s picture

Changes look great, thanks everyone.

larowlan’s picture

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestViewsData.php
@@ -16,6 +16,17 @@ class EntityTestViewsData extends EntityViewsData {
+        'title' => t('Computed String Field'),

this should be $this->t() (thanks vimeo/psalm for finding that one)

leaving at rtbc

amateescu’s picture

Sam152’s picture

LGTM +1 RTBC

Wim Leers’s picture

Does this need a change record? And a release notes mention? This seems a pretty significant improvement?

timmillwood’s picture

Issue tags: +8.4.0 release notes

Yes, I think this is worth a release notes mention, and here's a Change Notice https://www.drupal.org/node/2904410 (feel free to update @Sam152, @seanB or @amateescu).

larowlan’s picture

Updating credit to add reviewers that shaped final patch

  • larowlan committed 1fa12fb on 8.5.x
    Issue #2852067 by Sam152, seanB, amateescu, jibran, dawehner, larowlan,...
larowlan’s picture

Committed 1fa12fb and pushed to 8.5.x.

Have not published the change record yet - as a decision needs to be made with regards to backport.

The expectation is that in order for this to qualify for backport to 8.4.x, both #2862041: Provide useful Views filters for Content Moderation State fields and #2902187: Provide a way for users to moderate content need to be in 8.5.x too.

So to clarify - has not been cherrypicked to 8.4.x.

So leaving this at 8.4.x and at RTBC.

jibran’s picture

Any word on the backport yet?

timmillwood’s picture

Assigned: Unassigned » xjm

Other than being past the alpha/beta stages, I can't see any reason not to cherry pick this into 8.4.x.

Assigning to xjm to make a decision on this.

xjm’s picture

What I'd prefer to do is to get all three issues in to 8.5.x first (once they've had complete reviews etc.), and then consider backporting them together. So let's leave this at RTBC for now. Thanks!

xjm’s picture

Assigned: xjm » Unassigned

Looks like @larowlan also already said exactly that. Anyway it doesn't need to be assigned to me. :) The committer team will decide together when we decide whether to backport the view itself.

  • webchick committed d366b86 on 8.4.x authored by larowlan
    Issue #2852067 by Sam152, seanB, amateescu, jibran, dawehner, larowlan,...

webchick credited webchick.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, we had a meeting with the other core committers where we discussed the current state of Content Moderation. The general agreement was that we're really close to beta-level stability; we just need the trio of this issue, #2862041: Provide useful Views filters for Content Moderation State fields, and then #2902187: Provide a way for users to moderate content.

In order to give Content Moderation room to get to beta for 8.4.0 (which would be a huge win), there was agreement to backport this issue to 8.4.x, despite the fact that technically this type of patch would've been cut off at 8.4.0-beta1. Hopefully we can also get the other two issues put away in time for 8.4.0's release, but at least this one we don't have to stress about anymore. :)

Soooo! Cherry-picked 1fa12fb to 8.4.x. Thanks so much for all of your hard work on this, all! <3

Status: Fixed » Closed (fixed)

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

Sam152’s picture

Published the CR now that this is backported.

seanB’s picture

Ran into the computed entity reference filter issue again. Created a followup as suggested in #50.
#2936737: Return computed fields in FieldAPIHandlerTrait::getFieldStorageDefinition()