Allow adding computed bundle fields in Views

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 📌 Automatically declare computed base fields to Views Needs work

The example code in the issue summary makes use of the node entity, which is a bit of a special case with its node_field_data table. Should this work for any generic entity bundle without a [entity_type]_field_data mapping table?

With 9.5.8, using the example code, 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).

Which makes some sense, but I'm unsure whether this is a limitation, or if it should be able to work and I am missing some crucial mapping in the views data definition?

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 ✨ Add Views data automatically RTBC as the Computed Field Plugin → module also relies on this issue.

owenbush → closed merge request !1578

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.

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.

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.

owenbush → closed merge request !2511

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 📌 Allow adding computed bundle fields in Views Fixed . 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.

alexpott → closed merge request !4224

alexpott → closed merge request !1864

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 📌 EntityField::getDefinition() should throw an exception when it can't return a field storage Active to address this.

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

Committed cc1bc30 and pushed to 11.x. Thanks!

Can this be backported to 10.3?

Filed the obvious follow-up: 📌 Automatically declare computed bundle fields to Views Active .

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

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

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

However in the meantime, I was hoping for a workaround for use with Drupal 10. I created the test module shown at the top of this issue and the computed field became available in my Views. However, at the point of trying to add it to a View, I got the following error:

Error: Call to a member function getType() on null in Drupal\views\Plugin\views\field\EntityField->defineOptions() (line 375 of /var/www/html/web/core/modules/views/src/Plugin/views/field/EntityField.php).

Is this expected, or have I missed something? Do I have to wait until the changes above have been merged to Drupal core before I can use computed fields in Views?

MR4224 doesn't apply to 10.2.6

Here is a patch that applies to 10.2.6. Removing this hunk from the phpstan-baseline.neon allowed it to apply cleanly:

diff --git a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon
index b99733aed6aee10cb47823c0774d656d228f1db6..099a9a17e8e23cc7ec79a9176a3f610bfb67e15a 100644
--- a/core/phpstan-baseline.neon
+++ b/core/phpstan-baseline.neon
@@ -2500,11 +2500,6 @@ parameters:
 			count: 9
 			path: modules/views/src/Plugin/views/field/Date.php

-		-
-			message: "#^Method Drupal\\\\views\\\\Plugin\\\\views\\\\field\\\\EntityField\\:\\:getFieldStorageDefinition\\(\\) should return Drupal\\\\Core\\\\Field\\\\FieldStorageDefinitionInterface but return statement is missing\\.$#"
-			count: 1
-			path: modules/views/src/Plugin/views/field/EntityField.php
-
 		-
 			message: "#^Method Drupal\\\\views\\\\Plugin\\\\views\\\\field\\\\EntityField\\:\\:renderItems\\(\\) should return string but return statement is missing\\.$#"
 			count: 1