bundleFieldDefinitions() are not added in EntityViewsData

You could try the following to account for bundles properly:

    // Add any bundle fields defined in code.
    if (isset($this->entityType->getKeys()['bundle'])) {
      $bundle_field_definitions = [];
      foreach ($this->entityManager->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle_info) {
        $bundle_field_definitions += $this->entityManager->getFieldDefinitions($this->entityType->id(), $bundle_id);
      }

      $field_definitions += $bundle_field_definitions;
    }

Problem is this causes a crash because EntityViewsData claims it's using field definitions on line 252 whereas it's using that variable for both FieldDefinitionInterface[] and FieldStorageDefinitionInterface[] interchangeably, which causes the above code to crash because it may end up (correctly) providing FieldConfig instances, which implement FDI but not FSDI.

Also keep in mind we might start seeing duplicate entries because configurable fields (FieldStorageConfig/FieldConfig) are already being dealt with in views_field_default_views_data().

Guess this is blocked by #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface

> Also keep in mind we might start seeing duplicate entries because configurable fields (FieldStorageConfig/FieldConfig) are already being dealt with in views_field_default_views_data().

We definitely will see something, as Field module declares config fields via hook_entity_bundle_field_info(): https://api.drupal.org/api/drupal/core%21modules%21field%21field.module/...
Not sure if that'll mess up Views data, or just be a case of doing the work twice.

Either way, we need to do one of:

- remove views_field_default_views_data(), and consider that config fields are now handled in EntityViewsData
- add a filter to this patch so that config fields aren't handled here. In which case, I think we possibly *aren't* blocked by #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface

Here is a patch.

I've implemented @kristiaanvandeneynde's suggestion from #6

I've also added a filter to the bundle fields to skip those which don't implement both FieldDefinitionInterface and FieldStorageDefinitionInterface.

The benefits of this:

- sidesteps the problem of #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface as an interim measure.
- config fields get skipped, so we also don't need to figure out that problem just yet.
- Entity module's BundleFieldDefinitions are added (#2922784: Expose bundle fields from bundle plugin types to views)
- bundle fields from #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields when it lands should work.

Test failure is 'Invalid argument supplied for foreach()', coming from:

      foreach ($this->entityManager->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle_info) {

But AFAICT, getBundleInfo() always returns an array, even for config entities!

    if (isset($this->entityType->getKeys()['bundle'])) {
      $bundle_field_definitions = [];
      foreach ($this->entityManager->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle_info) {

I think there might be another problem with this approach: is it legitimate to define bundle fields on an entity that has no bundles? E.g. a module might use the hook_entity_bundle_field_info() to put bundle fields on everything, and that would include entity types like 'user'.

The code would skip that field, because the user entity has no bundle key, though getBundleInfo() will return a single bundle.

> But AFAICT, getBundleInfo() always returns an array, even for config entities!

As seen with the failures in #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface, it'll be methods on mocks that are returning NULL.

I'm trying to expose a code-defined bundle field of type date range and #11 does not work for me because it does not support hook_field_views_data() / hook_field_views_data_alter(). The attached patch seems to be working nicely.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -321,6 +324,19 @@ public function getViewsData() {
+    // Add any bundle fields defined in code.
+    // @todo Here we are assuming that hard-coded bundle field definitions
+    //   extend BaseFieldDefinition, which is a common case. Revisit this logic
+    //   in https://www.drupal.org/project/drupal/issues/2930736.
+    $storage_definitions = $this->entityManager->getFieldStorageDefinitions($this->entityType->id());

I think we should be fixing #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface first.

Marking postponed by related issue.

Found a bug in the patch - it's not carrying settings over, which causes problems for entity reference fields (you lose the entity type and bundle settings, which falls back to 'node' from EntityReferenceItem).

Leaving postponed but uploading the patch for reference. Also this is against 8.7.x - whole patch needs a re-roll for 8.8.x. We should add explicit test coverage for entity reference here.

Re-rolled #22 for 8.8.x.

Needed a little more refactoring.

Tests still outstanding.

I was getting this error:

Call to undefined method Drupal\Core\Entity\EntityTypeManager::getFieldStorageDefinitions() in core/modules/views/src/EntityViewsData.php:363

I had to update the patch

from this:
$storage_definitions = $this->entityManager->getFieldStorageDefinitions($this->entityType->id());

to this:
$storage_definitions = $this->entityFieldManager->getFieldStorageDefinitions($this->entityType->id());

Hope this helps.

+++ b/core/modules/field/src/BaseFieldStorageConfigWrapper.php
@@ -0,0 +1,111 @@
+class BaseFieldStorageConfigWrapper extends FieldStorageConfig {

Revisiting this issue after a while, and I'm struck by the introduction of this wrapper class. Why do we need it? Could we not just add the methods we need to FieldStorageConfig? The whole point of the unified field system is that base fields and config fields behave the same way and consumers of those fields can ignore the detail of their definition and storage. So it seems like a core smell that we need a wrapper like this!

I needed an update of this for 8.8, and I've not had time to delve into why patches after #11 add loads of extra stuff, so here's a reroll of #11.

I was about to write tests for this too, and then I hit what I now remember is one of the main reasons (IMO at least) that all the various issues around views data are stuck: the tests are a nightmare to change, because it's a unit test which therefore mocks EVERYTHING. Which means that any change in what EntityViewsData gets from services or entity storage means the mocking in the test needs to be changed, and it's a huge barrier, both because it's difficult, and because it's scary (changing both tests and SUT at the same time never feels good).

So I think actually we need a separate issue to replace the EntityViewsData unit test with a kernel test: create test entity classes (like system module has, or use those, or add to those), get the result. Test doesn't need to care about any of the plumbing in between the entity field definitions and the result.

Filed #3116481: Convert EntityViewsDataTest from a unit test to a kernel test.

While we are waiting for the tests I have made two patches, one including #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface and one post applying.

I have also made a minor change to use $this->entityType->hasKey('bundle')

@joachim agree about the Wrapper.

Re-roll on top of #2930736-40: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface since this fatals in Drupal 9.

This should be postponed on #3116481: Convert EntityViewsDataTest from a unit test to a kernel test, as that rewrites the tests.

Splitting so there's a combined patch with the blocking issue as well as the actual changes in this one.

Bundle info isn't on the entity type manager.

This should be a bit better.

Re-roll - patch from this issue, and also the stacked version alongside #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface.

Something went wrong with the re-roll, trying again.

Even more wrong.

Finding actual issues with the patch now.

Oof.

More real issues with the patch.

This patch prevents "List (text)" fields from working. They appear as a StringFilter instead of a ListField.

It can be fixed by removing this code:

// Add any bundle fields defined in code.
if ($this->entityType->hasKey('bundle')) {
  $bundle_field_definitions = [];
  foreach ($this->entityTypeBundleInfo->getBundleInfo($this->entityType->id()) as $bundle_id => $bundle_info) {
    $bundle_field_definitions += $this->entityFieldManager->getFieldDefinitions($this->entityType->id(), $bundle_id);
  }

  $field_definitions += $bundle_field_definitions;
}

$field_storage_definitions = array_map(function (FieldDefinitionInterface $definition) {
  return $definition->getFieldStorageDefinition();
}, $field_definitions);

What seems to be happening is fieldable entities such as nodes have duplicate fields being added. I'm not sure what the implication of removing those lines is, but this patch is currently breaking core functionality.

I wonder if this is because config fields get declared as bundle fields? (I think they do??)

This might be related to #2337515: Allow @FieldType to customize views data -- base fields don't get the right views data set up for them.

It's related in that EntityViewData provides hardcoded and limited support for base fields (which is what that issue is about) and views_views_data_alter() only supports field_storage_config config entities. Non-base and non-config fields are in a blindspot as far as Views is concerned, it only knows those about those two.

Adding some related issues.

This is a working version of #48 for Drupal 10. Please test it well before using it, I haven't tested it very thoroughly yet.

Updated version of patch #58, found a bug. This new patch works for Drupal 10.

Patch #60 does not conflict with Drupal 10.1.x, however, it generates an error when applying the database updates.

An MR has been created associated with Drupal 10.1.x version where the error is fixed.

For anyone looking, the patch for the above MR can be viewed here: https://git.drupalcode.org/project/drupal/-/merge_requests/4953.patch

Uploading it here for testing.

The patch #65 still breaks List (text) fields in views filters. Is nobody else having that problem?

I've had the same problem as #66. I've added some commits to the latest MR, to allow handlers to detect bundle fields. I've hidden the rest to prevent confusion to know which MR is canonical.

The only problem is that the changes are not compatible with Drupal 11.x because on the changes added at this issue, related with computed bundle field and views. So it is needed to update the MR to be compatible with 11.x

I added blob support after finding a regression in a filter that was working before the patch. The specific filter is the layout section field. The field was already added by reading field storage definitions, and now is being added in both field storage definitions and bundle field definitions.

#2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface landed.

Awesome, one step closer to better bundle field support in Views :)

@omarlopesino

I've had the same problem as #66. I've added some commits to the latest MR, to allow handlers to detect bundle fields.

So did you fix the problem from comment #66?

Opened MR !13189, which is a rebase of `2898635-10.1.x-views-data-bundle-field-definitions` branch against `11. x` branch, and added a deprecation message for the newly introduced constructor argument.

Hid 3x branches to leave visible only one 11.x branch, see #77.

Some feedback for this MR: It seems datetime fields filters get "reverted" to a String filter.

I tried to rebase the patch against main because conflicted against the latest 11.3.8.

However the patch doesn't work because the getFieldStorageDefinitions is duplicated, as I think as it moved inside a trait.

What is the correct approach in this case? Openining a new MR against 11.x?
Meanwhile I've attached the patch rebased against 11.x

#3045509: EntityFieldManager key/value field map gets out of sync, doesn't recognise bundle fields may help here.

@giuseppe87 as you can see, the MR adds an entirely new "getFieldStorageDefinitions()" method implementation. Even though one implementation already exists in "EntityViewsData" for "drupal/core-recommended" >= 11.3.9 (maybe even in lower versions, but "11.3.9" is the one I use).

We don't need to open a new MR, we just have to fix the existing one, since this is already incorrect for current 11.x.

@catch, what do you mean with "may help here"?

I created a new 11.x branch with a freshly rebased version of the old MR, since trying to rebase the old branch caused some very weird merge conflicts (furthermore the old version implemented a seperate "getFieldStorageDefinitions").

I also created a new main branch variant without the "getFieldStorageDefinitions" method changes. This version needs to be double checked, whether we need replacement code for the the "getFieldStorageDefinitions" additions.

MR https://git.drupalcode.org/project/drupal/-/merge_requests/13189 can be closed, as it is redundant now.