Allow adding computed bundle fields in Views

Here is a reroll for Drupal 8.5.4

The patch works aaaawweeessoome for my problem, thanks Tobias!

Thanks for the patch! Great work!

btw: the 8.5.4 patch applies, but not to 8.5.x...

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -337,11 +337,22 @@ protected function getFieldStorageDefinition() {
+    if (isset($this->definition['field_name'])) {
+      $base_fields = $this->entityManager->getBaseFieldDefinitions($entity_type_id);
+      if (isset($base_fields[$this->definition['field_name']])) {
+        return $base_fields[$this->definition['field_name']]->getFieldStorageDefinition();
+      }
+
+      if (isset($this->definition['bundle'])) {
+        $fields = $this->entityManager->getFieldDefinitions($entity_type_id, $this->definition['bundle']);
+        if (isset($fields[$this->definition['field_name']])) {
+          return $fields[$this->definition['field_name']]->getFieldStorageDefinition();
+        }
+      }

Just a thought but would it be more efficient to do it this way as getting the field list with a bundle also includes base fields?
Can't bundles also override some details so checking with a bundle first would include that case?

    if (isset($this->definition['field_name'])) {
      if (isset($this->definition['bundle'])) {
        $fields = $this->entityManager->getFieldDefinitions($entity_type_id, $this->definition['bundle']);
      }
      else {
        $fields = $this->entityManager->getBaseFieldDefinitions($entity_type_id);
      }
      if (isset($fields[$this->definition['field_name']])) {
        return $fields[$this->definition['field_name']]->getFieldStorageDefinition();
      }
    }

I took the liberty to tweak the comment a bit as well, what do you think?

This patch has a very deja vu feeling to it because of #2852067: Add support for rendering computed fields to the "field" views field handler. Changes look good just a doc nit other than that it is RTBC.

1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
   @@ -49,4 +50,20 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      $fields['computed_bundle_field'] = BaseFieldDefinition::create('string')
   

   I'm sure @amateescu doesn't approve of this but there is no correct API exist to create computed bundle field.

2. +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
   @@ -336,12 +336,20 @@ protected function getFieldStorageDefinition() {
   +    // support them. If a bundle was specified as part of the definition, check
   +    // those field definitions, else only base fields can be checked.
   

   s/check those/check the

I think we should add @todo to #21.1 linking to #2986634: Discuss the role of field storage definitions for computed fields..

Updated with the changes from #21 & #22.

EDIT: Ugh, I was thinking of #2935932 when wording the comment but the linked issue lists it at the top so maybe it's ok?

I was thinking of #2935932 when wording the comment but the linked issue lists it at the top so maybe it's ok?

It is perfect.

This should have a changelog entry describing the new views data structure, right?

I think we can just update https://www.drupal.org/node/2904410 once the issue is fixed.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
@@ -49,4 +50,22 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      // @todo Bundle fields should not need to use BaseFieldDefinition
+      //   https://www.drupal.org/node/2986634
+      $fields['computed_bundle_field'] = BaseFieldDefinition::create('string')
+        ->setLabel('Computed Bundle Field Test')
+        ->setComputed(TRUE)
+        ->setClass(ComputedTestBundleFieldItemList::class);

+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
+++ b/core/modules/views/src/Plugin/views/field/EntityField.php
@@ -336,12 +336,20 @@ protected function getFieldStorageDefinition() {

@@ -336,12 +336,20 @@ protected function getFieldStorageDefinition() {
+      if (isset($this->definition['bundle'])) {
+        $fields = $this->entityManager->getFieldDefinitions($entity_type_id, $this->definition['bundle']);
+      }

This isn't quite right. The method expects a storage definition to be returned and the only reason a storage definition is returned in the test case is because BaseFieldDefinition implements both the storage and definition interfaces.

I believe if the computed bundle field had an associated storage definition (which @amateescu and @tstoeckler agreed should be the case in #2935932: Add a FieldDefinition class for defining bundle fields in code.), this would already just work.

I think the test coverage is still valuable, but this should be reevaluated once we have the capabilities to properly test this.

@Sam152 I'm not following your comment, what would already just work without the changes here?

I personally don't agree having storage definitions for computed bundle or base fields makes sense at all but the interface already dictates that they do so I guess we have no choice without breaking the API.

I don't think we can fix #28 in core atm without #2935932: Add a FieldDefinition class for defining bundle fields in code.. It makes sense to postpone this on #2935932.

Parent issue fixed.

This should address #28.

This is kind of blocked on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() as well but we can just add a todo and then replace the use statement in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

This should be green.

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
@@ -49,4 +52,27 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        ->setComputed(TRUE)
+        ->setClass(ComputedTestBundleFieldItemList::class);
+      $fields['computed_bundle_field'] = FieldDefinition::createFromFieldStorageDefinition($storageDefinition)
...
+        ->setComputed(TRUE)
+        ->setClass(ComputedTestBundleFieldItemList::class);

This shows FieldDefinition::createFromFieldStorageDefinition() should at least run setComputed and setClass. Seems like #2935932: Add a FieldDefinition class for defining bundle fields in code. missed it.

#32 shows the error when setComputed and setClass are not called on FieldStorageDefinition
#34 shows the error when setComputed and setClass are not called on FieldDefinition

Maybe #2986836: Support computed bundle fields by adding the notion of computed field storage definitions will fix it by adding a new class.

Rerolled after #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods.

Reroll.

Thanks, @jibran for pointing me to this issue. While I'm not yet familiar with all changes to field/storage defs, I'm only warming up :)

1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
   @@ -49,4 +52,27 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    if ($bundle === $entity_type->id()) {
   
   +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestViewsData.php
   @@ -25,6 +25,15 @@ public function getViewsData() {
   +          'bundle' => 'entity_test_computed_field',
   
   +++ b/core/modules/views/src/Plugin/views/field/EntityField.php
   @@ -363,12 +363,20 @@ protected function getFieldStorageDefinition() {
   +      if (isset($this->definition['bundle'])) {
   +        $fields = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $this->definition['bundle']);
   +      }
   

   What happens if the field is attached to more than one bundle within the same entity type, which is really used in the wild? Should we set the views data bundle key as array, instead of string?

2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
   @@ -49,4 +52,27 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      // @todo Use the proper FieldStorageDefinition class instead
   +      //  https://www.drupal.org/node/2280639.
   

   It seems that we have to postpone on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()? But reading https://www.drupal.org/node/2982512 it looks like we'll have to use the new FieldDefinition? Not very clear to me.

Thanks, for having a look at the patch

1. This is a really good question and a valid point. We could change it to
   

         $fields = [];
         if (isset($this->definition['bundles'])) {
           foreach ($this->definition['bundles'] as $bundle) {
             $fields[] += $this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle); 
           }
         }

   , thoughts?

2. We don't have to postpone this on #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() as it is not a direct dependency here whichever issue lands first the other can accommdate the changes.

Tests were conflicting with some other patch so here is a fix only patch. #42.1 is still pending.

I've been working on a contrib module that makes use of computed bundle fields and applying the fix only patch #44 seemed to resolve the issue for me. It would be super to get this into D8.9 and not have to wait until 9.1 because it cripples the module I'm working on's functionality.

So if there is anything I can do to get this over the line please do let me know.

I've tried my best to wrap my head around what should actually happen in the instance that a computed bundle basefield exists on multiple bundles. For example, the question arises, which of those definitions should we use? I've assumed that it should be the first one we come across with the appropriate field_name. I'm not sure if that assumption is correct or not.

I'd love to get this moving more and happy to help out however possible.

Attaching a new patch to fix the failing tests.

Go tests go

The patch looks good now. Let's update the IS.

+++ b/core/modules/system/tests/modules/entity_test/src/EntityTestViewsData.php
@@ -31,7 +31,7 @@ public function getViewsData() {
-          'bundle' => 'entity_test_computed_field',
+          'bundles' => ['entity_test_computed_field'],

Let's add test for multiple bundles as well to cover all the bases.

I guess I am slightly confused as to what to do for multiple bundles. I could update that test views data to something like

'bundles' => ['entity_test_computed_field', 'someother_bundle'],

But if that bundle does not actually exist in the test data, then it is not an accurate test. Do we need to mock up a second bundle on that entity with the shared computed field? And then set that value to something different and assert the value?

Sorry, I haven't done a lot of core unit tests so I'm not exactly sure of the correct approach.

Besides testing for two bundles that the field is on, I would also like to see a test where there are two bundles rendered in the View and the computed field isn't on one of them (so it would show up empty once).

I have put together a merge request that includes all the work from previous patch #50 and additional changes to add more tests.

If you look at the second commit on the MR you can see the changes made since the initial branch creation using patch #50.

What I did was enabled the EntityTestComputedField entity to have bundles, then I create 3 different bundles and an entity for each. The first two bundles will have the bundle computed basefield, the third will not. This way I can check the following:

1) Do all the bundles have the computed basefield as you would expect?
2) Do the first two bundles have the bundle computed basefield?
3) Does the third bundle not have the bundle computed basefield?

Tests seem to be passing so I think I got things covered, but please let me know if there is more to do here.

@Lendude is there any chance you could take another look at this issue, the MR in #56 has added a number of new tests which should cover the use-cases you requested in #54

Thanks, and I'm happy to make any necessary changes to get this issue moving.

Hiding the old patches now that work is taking place in an MR.

I filled in the rest of the issue summary except the steps to reproduce. Those are always good, but since this already has automated test coverage, it isn't necessary.

Assigning to @Lendude for final sign-off.

Also there was another commit adding something to the end of entity_test.schema.yml, so this needs a reroll. My approach to side-stepping these conflicts: don't add to the end of the file. Find some other nice place in the middle to add the definition.

Ugh. Well that went horribly wrong. I'm looking at resolving this now.

My rebase destroyed the previous MR, so I've sorted it out by opening a new MR, merged back in the latest changes to 9.3.x which included the change to entity_test.schema.yml mentioned by Tim.

Looks like we're green again and the MR is mergeable.

Originally, the idea with this issue was to specify a specific bundle for which a bundle field can exist, but during the process of refining the problem and implementing the solution it was decided that specifying multiple bundles was a more flexible approach. As such I have updated the issue's proposed solution to reflect the change from `bundle` to `bundles` as the views data key name.

The tests look good.

One last thing I'm concerned about is docs. We are introducing a new key for Views data, but we are not providing any docs for it, how are people going to know how this works? A quick search didn't reveal much in the way of docs for views data for computed fields anyway, none were added in #2852067: Add support for rendering computed fields to the "field" views field handler. So maybe we could do that in a follow up, but here might be a good place to rectify this.

Maybe adding an example to hook_views_data in views.api.php? There doesn't seem like another place where the structure of Views data is explained better.

@lendude thanks for the follow-up, I really appreciate you helping to move this along.

I've added some documentation to views.api.php here is the commit that adds the documentation.

https://git.drupalcode.org/project/drupal/-/merge_requests/1019/diffs?co...

Does this satisfy the documentation requirements? If not, I'm happy to take direction and add something more.

It would be nice to wait until #3116481: Convert EntityViewsDataTest from a unit test to a kernel test is committed, and then we could add tests for the ViewsData handler?

Documentation looks good to me (was looking for this functionality myself).

This was no longer applying to 9.3.x so I merged and fixed a small conflict, but now it looks like tests are failing due to some changes from #2997123: Cacheability of normalized computed fields' properties is not captured during serialization.

I've rebased the MR against the latest 9.3.x branch but I guess we're still waiting on #3116481: Convert EntityViewsDataTest from a unit test to a kernel test to land so we can rework the tests.

Setting it back to needs works as it's not green, thanks.

We're finally back to green. Still have to wait on #3116481: Convert EntityViewsDataTest from a unit test to a kernel test making the cut.

These rebases to keep up with core are getting ... fun.

Status update

The previous MR got way out of control with rebasing, and each time core changed I was losing at least an hour trying to rebase and deal with conflicts. So, I resolved the conflicts, exported a patch, and applied the patch to a new MR. I'll hide the previous one as it is no longer applicable.

At this point nothing is changing in the MR for this work, I am just keeping it up to date with the latest in 9.3.x by rebasing, so if we could get a review that would be absolutely great.

I created a new PR against 9.4.x as switching the target branch did not work out well at all. So we have an MR for 9.3.x (which is now obsolete) and a new MR against 9.4.x

The changes look good. @Lendude's last feedback is addressed so it RTBC for me but I can't RTBC it as I worked on the patch before.

Sorry, this fell off my radar in a big way it seems.

This now looks ready to me too, thanks for the great work!

This looks good to me too, however it would be good to see a green run on 10.0.x too.

I'm going to change the target of the MR to 10.0.x and run a test run.

Unfortunately this doesn't apply cleanly to 10.0.x and 9.4.x

Can we get a second MR that applies cleanly against 10.0.x

Thanks folks

Using the Needs reroll tag to surface contributors who may be able to create a 10.0.x version of the MR

To be clear, this doesn't need reroll, it needs a second MR created that applies to 10.0.x

Here is a new MR that applies to 10.0.x, thanks!

I've rebased the 9.4.x MR, and we now also have a 10.0.x MR that's green. Can we get this reviewed again, please?

I encountered this issue for a view used in the recurrent events module.

Within these patches there is none for 9.3.7 which is the version the site being worked on is being upgraded to,

I modified the patch given for 9.2 so that it worked with 9.3.

Tests have shown the site is working as intended.

Patch for 9.3.7

All credits go to owenbush since I just modified for it to work in 9.3.7

Fixed CS error

I think the issue summary could use an update to identify the steps to reproduce, which are currently N/A and the Remaining tasks which is TBD.

Since we're adding an option, do we need a change record?

I have altered the original description to add the steps to recreate, and the remaining tasks.

In response to the question about needing a change record, I do not know what to say on that front and would need a Views maintainer to confirm. This did make it to RTBC before the request for the 10.x patch was made, so I would assume not at this point given that the option is now documented in the views api.

Can we try and get this moving again? Nothing has changed in the patches for quite some time, we're just rebasing over and over. I am at DrupalCon if anyone wants to walk through this.

Closed MR for 9.3.x, updated MR for 9.4.x and 10.0.x, and added new 9.5.x MR.

It would be super lovely/marvelous to try and get this reviewed and committed.

Restoring RTBC, nothing seems to have changed since #87, and the request from #89 seems to have been addressed, AFAICT.

Just patched on 9.3.x after having this problem with one of my views and the patch worked flawlessly. I was able to save my work immediately after installing #96.

Saving issue credits

crediting @claudiu.cristea and @Sam152 for reviews
crediting @tim.plunkett for issue summary updates

Left a couple of nice-to-have comments about nesting/complexity, one personal comment about in_array vs array_search which can be taken/left but one question about what to do if two bundles have the same computed field name with a different computed field class which I think we at least need to discuss.

> I think the only thing we can do is get the definition for all bundles and make sure they all have the same computed class and if not raise an exception or warning or similar? There's just not enough context to do anything else.

That doesn't seem to be ideal functionality.

What if you mean to have different classes?

We possibly need to add each bundle field as a separate field declaration in views?

We possibly need to add each bundle field as a separate field declaration in views?

Yeah that's a great idea

So how do we move forward? There's nothing to do right, the onus is on the developer, we just need a documentation update in the views.api.php change?

I've tried to address all the comments, tests are running and hopefully they will pass and I'll move back to Needs Review

Looks like PHPStan did not actually like the new PHP8 NULL safe operator, so I've reverted that change.

Running PHPStan on *all* files.
------ -----------------------------------------------------------------------
Line core/modules/views/src/Plugin/views/field/EntityField.php
------ -----------------------------------------------------------------------
Ignored error pattern #^Method
Drupal\\views\\Plugin\\views\\field\\EntityField\:\:getFieldStorageDe
finition\(\) should return
Drupal\\Core\\Field\\FieldStorageDefinitionInterface but return
statement is missing\.$# in path
/var/www/html/core/modules/views/src/Plugin/views/field/EntityField.p
hp was not matched in reported errors.
------ -----------------------------------------------------------------------

Back to needs review.

Can the MR be rebased onto 10.1.x

Left some comments on the MR, I think we still need to handle #107/ #108

Also, I'm not sure if this is a bug or a task or a feature

I'd say it is a task as we already support non-bundle fields so this is not a new feature. Also, nothing is broken as we don't support them yet so it is not a bug.

We have 3 MRs open which is very confusing and not necessary. Feedback is scattered throughout. Can we please close them all except the one against 10.1.x (which I think will need to be created, or 10.0.x repurposed)

Add The Patch Re-roll For 10.1.

Are any of the patches workable for a 9.5.x site?

@wxman we're using patch from MR 2511, works fine.

@vlad.dancer @joachim I don't know what I could be doing wrong, but still don't have access in views. I applied the patch successfully to 9.5.7, cleared the caches twice, but still I don't see the one computed field in Views. This is just a simple test site with the only content being a test of the computed field version 4. The field is working as I wanted it to, and it shows in the field list as a computed field.

> cleared the caches twice, but still I don't see the one computed field in Views.

The patch *allows* bundle fields to be used. It doesn't automatically register them.

I commented on this further up, saying the patch should do this, but I'm no longer sure, as it may make it a fair bit more complex. For base fields, it's been done in two steps -- #2852067: Add support for rendering computed fields to the "field" views field handler and #3349739: Automatically declare computed base fields to Views

With 9.5.8, using the MR 2511 patch and the example from the issue summary, I can successfully add the computed field to a view of profile entities, referencing the entity base table in views data as:

function MYMODULE_views_data_alter(array &$data) {
  $data['profile']['computed_bundle_string_field'] = [
    'title' => 'Computed Bundle String Field',
    'field' => [
      'title' => 'Computed Bundle String Field',
      'id' => 'field',
      'field_name' => 'computed_bundle_string_field',
      'bundles' => ['my_bundle'],
    ],
  ];
}

But I get no output from it. The error is

Drupal\Core\Entity\Sql\SqlContentEntityStorageException: Column information not available for the 'computed_bundle_string_field' field. in Drupal\Core\Entity\Sql\DefaultTableMapping->getFieldColumnName() (line 440 of /var/www/html/public/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

Should this be able to work, and I am missing some crucial mapping in the views data definition?

this issues is resolved. MR has been created and merged for Drupal 10.x.1MR !1864 mergeable Please review it.

@124: There are various issues on the MR to resolve.

@123: I tried the diff from the 9.5 branch, with a bundle computed field made with Computed Field module (https://www.drupal.org/project/computed_field). It worked fine (apart from warning that was caused by a bug in Computed Field module, which I've now fixed).

I suspect it's a problem with the definition of your field.

Tagging as a contrib module blocker, since this affects https://www.drupal.org/project/computed_field.

Added a reference from Add Views data automatically as the Computed Field Plugin module also relies on this issue.

Tagging for IS update, as I don't think the proposed 'bundles' key is actually necessary -- see my prior comment.

I'm working on this in Pittsburgh. Hoping to have all the requested changes in to a new MR for 11.x before the end of the conference.

Definitely this:

> I add the EntityTypeBundleInfo service with dependency injection to the EntityField views class.

There's no point to having the 'bundles' key - the information is mostly ignored. It's bad DX to make developers specify it.

Updating issue summary.

I've spent a long time trying to figure this out without specifying the 'bundles' key in EntityTestViewsData::getViewsData or in entity_test_views_data_alter().

The bundles are configured using 'entity_test_create_bundle()' within ComputedBundleFieldTest::setUp(), however, using xdebug and setting breakpoints where the bundles are configured, the code never actually gets to that point because it dies with the following before it ever gets there:

1) Drupal\Tests\views\Kernel\Handler\ComputedBundleFieldTest::testComputedFieldHandler
Error: Call to a member function getType() on null

/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/field/EntityField.php:397
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/PluginBase.php:143
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/HandlerBase.php:109
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/field/FieldPluginBase.php:136
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/field/EntityField.php:211
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:906
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:934
/var/www/html/repos/drupal/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:970
/var/www/html/repos/drupal/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:71
/var/www/html/repos/drupal/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:89
/var/www/html/repos/drupal/core/modules/views/src/Entity/View.php:282
/var/www/html/repos/drupal/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:321
/var/www/html/repos/drupal/core/modules/views/src/Entity/View.php:292
/var/www/html/repos/drupal/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528
/var/www/html/repos/drupal/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483
/var/www/html/repos/drupal/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/var/www/html/repos/drupal/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/repos/drupal/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:609
/var/www/html/repos/drupal/core/modules/views/src/Tests/ViewTestData.php:49
/var/www/html/repos/drupal/core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php:53
/var/www/html/repos/drupal/core/modules/views/tests/src/Kernel/Handler/ComputedBundleFieldTest.php:35
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

So looking through the backtrace and adding breakpoints it appears that the test view is installed and EntityTestViewsData::getViewsData executes as part of setting up the parent of the test (parent::setUp()) and then EntityField attempts to look for the field definition of the bundle fields. Using the EntityTypeBundleInfo service to find the bundles that have the computed bundle fields but the bundles do not (yet) exist, because the rest of the test's ::setUp() function have not yet executed. This blows up as above.

If I use the EntityTypeBundleInfo service to find the bundles (there is only the default bundle at execution time) it doesn't work, but if I execute the EntityFieldManager::getFieldDefinition() on hardcoded bundle names, it does work. So it appears the bundles are not yet initialized, or in cache, but executing getFieldDefinition() seems to work.

The field map also does not contain the bundles at this point, for the same reason - the setUp code has not finished executing.

The only way I can reliably use EntityFieldManager::getFieldDefinition() is if I already know the machine name of the bundles, and the only way that I can see for that to work is to define the 'bundles' key in the views data.

At this point I'm out of ideas and if anyone else has any ideas I'd appreciate some help. I've pushed a lot of code to this issue and have hit a brick wall.

Does it also have this problem outside of tests?

The code as-is works for me outside of the context of the tests.

I'm a bit confused about the need for the 'bundles' property or even the logic surrounding it, as I've just made a custom module that defines a computed bundle field and it shows up fine in Views with this in hook_views_data():

  $data['node_field_data']['test_2981047_base'] = [
    'title' => t('Test computed base field 2981047'),
    'entity field' => 'test_2981047_base',
    'field' => [
      'id' => 'field',
    ],
  ];

  $data['node_field_data']['test_2981047_bundle'] = [
    'title' => t('Test computed bundle field 2981047'),
    'entity field' => 'test_2981047_bundle',
    'field' => [
      'id' => 'field',
    ],
  ];

I'm attaching the module -- I've written it on D9 as I don't have a local D10 yet.

Just install it & create a node of the supplied bundle and then go to the view at /test-2981047.

EDIT: Ah, I see my mistake -- hook_entity_field_storage_info() is necessary when defining normal bundle fields, but with a computed field, you don't need it. If I comment out that hook in my demo module, the node loads and displays properly, but the view crashes.

I've figured out the problem with the test:

    parent::setUp($import_test_views);

that tries to import the test view, but it's run BEFORE the bundles are defined, and that's why EntityTypeBundleInfo has nothing.

Working on it.

Also, BTW, if we're using entity_test_create_bundle() then I don't think we need a bundle entity type, as that function stores dummy bundle info in state.

Got it working on 9.5.x.

I've cherry-picked to the D10 branch, but the cherry-pick to the D11 branch is conflicting - not sure why? Could someone take a look at it?

Ah, D11 has this:

    // Check for computed bundle base fields too.
    $bundles = $this->entityTypeBundleInfo->getBundleInfo($entity_type_id);
    foreach ($bundles as $bundle_id => $bundle) {
      $bundle_fields = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle_id);
      if (isset($bundle_fields[$this->definition['field_name']])) {
        return $bundle_fields[$this->definition['field_name']]->getFieldStorageDefinition();
      }
    }

which actually does what we want, but the comment only mentions base fields! And what business is it of base fields to check every bundle???

The reason we check every bundle is because we need to find the first definition of the computed bundle field, which could be shared across one or more bundles. So EntityField is unaware of which bundles define the field, so we loop through all bundles until we find a definition for the computed field. When I was testing I wasn't able to use the Field Map, but that was likely due to the same issue you solved with the view being imported before the bundles were defined.

So we may be able to use the field map instead?

The D10 branch is working - works manually, and test passes.

I'm not sure what to do on the D11 branch as it looks like that's got a lot of your experimental commits on it. Could you have a look at it? There's two new commits I've made on the D10 branch which need to be brought into D11.

> The reason we check every bundle is because we need to find the first definition of the computed bundle field, which could be shared across one or more bundles

Yup, that's what I've done on D10 too.

> // Check for computed bundle base fields too.

I see why I was confused now -- it's just this comment that doesn't make sense. The fields can't be BOTH 'bundle base'. They're one or the other!

I see that comment is confusing. I'll get the D11 branch figured out with what you've done in D10. Some other changes I made in the D11 branch were to make these computed bundle field tests completely separate from computed base field tests. So there was some refactoring and renaming going on.

I have updated the 11.x version of the MR to factor in your changes. I am now seeing this working locally manually and in tests, so hopefully this goes green.

The 11.x MR and the 10.x MR now are quite different because of the refactoring for separating out the tests and creating a different entity and bundle name to test with.

Edit: So if the 11.x goes green and seems acceptable, maybe it can be backported for 9.5 and 10?

I'm not sure what to do about the custom commands failing on the 11.x branch, EntityField::getFieldStorageDefinition() even without my changes (so as it exists currently in core: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/views...) may not actually return anything.

I tried setting the return type of the function to FieldStorageDefinitionInterface or NULL, and then returning NULL at the end if no definitions are found, but it seems like the check is still failing. I don't know how the change made to lookup the bundle fields does anything different to what is already happening in that function. Is this also a backwards incompatible API change?

There's a line in phpstan-baseline.neon that can be removed.

Comparing 10.x MR and 11.x MR though and see core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php is updated in 11.x is that intended?

Thanks for pointing out the phpstan config I could remove.

As for the difference between 10.x and 11.x MR I dependency inject the entity_type.bundle.info service into the EntityField class in the 11.x MR. I don't think the 10.x MR does that. The FieldTest class instantiates an EntityField object several times, so I had to pass through the service as well.

One option to get around that would be to make the entity_type.bundle.info service optional in EntityField, and if it is not passed, instantiate it in the EntityField::__construct(). I feel like I've seen this approach elsewhere in core. That would mean we wouldn't need to update the FieldTest class, but it felt like the right thing to do, to pass all the required services.

The failures were kind of expected in the jsonapi tests as I didn't modify any of those. I'll get those looked at tomorrow.

> Some other changes I made in the D11 branch were to make these computed bundle field tests completely separate from computed base field tests

That's probably a good thing to do in D10 as well.

Another thing about the tests - if you're using entity_test_create_bundle() to mock bundles, I don't think you need to define a bundle entity at all.

> As for the difference between 10.x and 11.x MR I dependency inject the entity_type.bundle.info service into the EntityField class in the 11.x MR

Ah yes, because I was experimenting I used \Drupal::service() to get the bundle info service. It should be injected - there's a pattern to add additional services to a class in a backwards-compatible way.

11.x is green which is great.

So a couple of questions are outstanding:

1. The 10.x branch is missing a couple of changes from the 11.x branch such as the dependency injection of the service in EntityField, and the separation of the computed base field tests and the computed bundle field tests. So, how should we approach that? Backport a new version from 11.x to a new 10.x MR, or attempt to recreate the changes in the current 10.x MR?

2. Do we need to ensure that the service dependency injection is backwards compatible in the 10.x branch? What about the 11.x branch? My thoughts are that the service should be optional on the 10.x branch, but maybe deprecate it being optional in the 11.x branch and make it required, and trigger an error if the service is not passed in as an argument.

3. Anything else we are missing to get this through?

May help you decide but the code has to be merged into 11.x first and backported.

11.x is the main branch currently with D10 releases being cut from that.

So if something needs to be deprecated it will only go into 11.x branch. Don’t think policy is to backport deprecations. But if this were merged in soon it would be deprecated in 10.2 but required in D11.

As 10.2 will be tagged off 11.x

Hope that helps.

Thats helpful thank you so much. I had no idea about that process. So, to me this feels ready to review. If someone disagrees then it can be changed back. But for now I'll move this to Needs Review.

> 1. The 10.x branch is missing a couple of changes from the 11.x branch such as the dependency injection of the service in EntityField, and the separation of the computed base field tests and the computed bundle field tests. So, how should we approach that? Backport a new version from 11.x to a new 10.x MR, or attempt to recreate the changes in the current 10.x MR?

We should make the D10 branch as similar as possible to the D11 branch:

- Make the tests the same.
- Use the BC-compatible pattern for DI in EntityField

Thanks for the review, joachim. I think I've addressed the issues you pointed out in the 11.x MR

1. You were right, no bundle class was necessary, and as a result a bunch of other things changed - including no longer needing (maybe we never needed one anyway) the JSONAPI test, which I added to ensure the bundle info was represented appropriately with jsonapi, but now we don't have a test bundle we definitely do not need that.

2. I addressed the comment in the views api you mentioned.

Once the 11.x MR is looking good, I'll go ahead and work on the 10.x MR to get it in sync.

Due to the number of changes in the 11.x MR it made more sense to backport the 11.x changes to a new branch rather than try and update the 10.x MR and have to force push, so I guess that can be closed (although, I couldn't do it myself).

I also closed out the 9.5 MR. So ideally we would just have the 11.x MR and the new backported 10.x MR.

Ok looks like we have green 11.x and 10.x MRs.

The 10.x branch differs from the 11.x branch in FieldTest and EntityField.

EntityField has an optional argument of the entity_type.bundle.info service, if the service is not passed the constructor will retrieve it from the container and triggers a deprecation error (suppressed) to warn that in 11.0.0 the service is no longer optional.

This also affected the FieldTest class which in the 10.x MR had to mock the entity_type.bundle.info service and pass it into the instantiations of EntityField.

Other than those differences the 10.x MR is a direct backport of the 11.x MR.

Marking this as Needs Review.

Appear to be some open threads on MR 4139

MR 4139 (11.x) has been updated after the review from Joachim.

MR 4224 (10.x backport) has been updated with the same changes.

Moving back to Needs Review.

Lets get this in front of the committer eyes.

@smustgrave, It seems you have confidence that a committer should look at this. But your comment is brief and makes me wonder if there is a specific part of this a committer should look at. If not, then the comments should include the steps you have taken to determine this is ready for a committer's time. For reference there is a list of items to check in Triage the Drupal core RTBC queue. And then stating what you have done, or not done, greatly helps a committer.

I'd agree this is RTBC. The tests are green and I've reviewed the MR several times.

@joachim, thanks for the reply. It looks like I didn't triage this properly and I started at the bottom and not the top. Sorry about that.

I read the issue summary and then through the comments, mostly looking for questions unanswered. I saw more than one request for an Issue Summary update. Thank you! That makes it easier for committers and reviewers. (though, why I didn't start with that in July I don't know). In reading the comments I think everything has been answered. I wasn't sure about #144. But on reading the relevant comments I think that settled on adding an example to views.api.php. And that has been done. So, I didn't find any thing missed in the comments.

This is a challenging issue to triage! It is long and 3 MRs with comments!

I did not update credit or review the code.

I forgot to respond to the question about a CR. #3, #97, #98

Yes, it is worth adding a change record, adding tag. There were two comments about amending an existing change record, https://www.drupal.org/node/2904410. That is not an option because a change record is tied to a release. And thus each issue should have it's own change record so that the version field is correct.

Because this needs a change record, I am setting it to needs work.

Done a CR, but feels a bit pointless to me as there is no API change, and to me this is a bug fix -- thing that used to cause a crash doesn't cause a crash any more.

CR reads well, this should be ready now.

On Friday I tried MR !1864 but I was not able to apply that one as patch. For me it looks like that is an older MR which will be discarded in favor of !4224?

MR !4224 on 10.1.2 with Commerce 2.36.0 works for me for a computed bundle field on the commerce order item entity.

Nevertheless I found an error in the example in `views.api.php` in both MRs, as following the example in !4224 I got the error

The "computed" plugin does not exist. Valid plugin IDs for Drupal\views\Plugin\ViewsHandlerManager are: [list of found plugin ids]

See my review comment in !4224 for details.

> On Friday I tried MR !1864 but I was not able to apply that one as patch. For me it looks like that is an older MR which will be discarded in favor of !4224?

Yes, MR !1864 is older. It's currently marked as not applying, so should be ignored.

> Nevertheless I found an error in the example in `views.api.php` in both MRs

Yup - the 'bundle' bit should be removed.

@joachim, yes the 'bundle' bit should be removed too. But the field (plugin) id should be changed from 'computed' to 'field' as no computed plugin exists.

I have updated both MR 4139 (11.x) and MR 4224 (10.x backport) to change the field ID from 'computed' to 'field'.

I had previously removed the 'bundle' part though, so I assume that was seen in MR 1864 which is old and not used (I wish I could close it, but I do not have the permission to do so).

Now I've addressed the issue with the views.api.php doc change I'm moving this back to Needs Review.

MR !4139 says it's not mergeable.

(Which is why I got confused about the 'bundles' key -- I assumed the unmergeable MR was the old one.)

MR !4139 was just behind 11.x a little, I've updated the branch to the latest 11.x, it is now mergeable again.

Was previously RTBC in 167 and seems just needed a rebase so restoring status.

Added some small review nits to the MR. Saving issue credits.

Hiding all the files are closing the out-of-date MRs.

Thanks @alexpott! Reviewed your changes and they fix your own feedback :) Back to RTBC!

Small docs nitpic, sorry!

@joachim actually I think here the better thing would be to blow up with a more helpful exception. Because all the usages of this method assume that it going to return a field storage. Which I think is the correct behaviour - if you are able to configure an entity field in views this thing should be able to get its field storage - if it can't then that is a error rather than something just to continue on from. I'd rather not add documentation - I think we should open a follow-up to trigger an exception. Created #3404304: EntityField::getFieldStorageDefinition() should throw an exception when it can't return a field storage to address this.

I removed the |null since there's no need to add this here as we're going to address this in #3404304: EntityField::getFieldStorageDefinition() should throw an exception when it can't return a field storage

Committed cc1bc30 and pushed to 11.x. Thanks!

Can this be backported to 10.3?

Filed the obvious follow-up: #3404369: Automatically declare computed bundle fields to Views.

@joachim 11.x will (currently) become 10.3, we haven't branched 10.3 yet.

I see that this issue is being addressed via changes to Drupal core.

Is there any workaround for using computed fields with Drupal 10 views in the meantime, or should I take a different approach? I can insert my field into my view just using my custom module, but then I have to do a lot of template formatting to emulate the output of other table-based fields.

(Edited for clarity.)