Add support for rendering computed fields to the "field" views field handler

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.

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

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.

Thanks!

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?

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.

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

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

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

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

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?

Added change mentioned in #35.

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

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

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

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.

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

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.

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.

This is a dependency for #2902187: Provide a way for users to moderate content.

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.

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.

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

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

Here we go.

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

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

Updating credit to add reviewers that shaped final patch

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.

Any word on the backport yet?

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.

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!

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.

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

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

What about computed fields defined via hook_entity_bundle_field_info()?

@claudiu.cristea I'm glad you asked would you like to review #2981047: Allow adding computed bundle fields in Views?