EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface

It gets messier: further down the code, we stop assuming that we have a FSDI, and go off to load them all:

  protected function mapFieldDefinition($table, $field_name, FieldDefinitionInterface $field_definition, TableMappingInterface $table_mapping, &$table_data) {
    // Create a dummy instance to retrieve property definitions.
    $field_column_mapping = $table_mapping->getColumnNames($field_name);
    $field_schema = $this->getFieldStorageDefinitions()[$field_name]->getSchema();

We have a $field_definition at that point, so why not just call getFieldStorageDefinition() on it?

Here's a go at a patch.

Given an FDI, we can get its FSDI with getFieldStorageDefinition().

For BaseFieldDefinitions, since they implement both, that method trivially returns the same object, but that's ok.

Invalid argument supplied for foreach()

/var/www/html/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:105
/var/www/html/core/modules/views/src/EntityViewsData.php:267
/var/www/html/core/modules/views/tests/src/Unit/EntityViewsDataTest.php:194

How can we have something that's not an array here????

    $field_definitions = $this->entityManager->getBaseFieldDefinitions($this->entityType->id());

    $field_storage_definitions = [];
    foreach ($field_definitions as $field_name => $field_definition) {

Trying a quick fix in the entity field manager -- should be a separate issue if it's indeed the problem.

Figured it out -- the mocked entity manager should be returning an empty array for baseFieldDefinitions().

I went down the rest of the class and changed all the calls to FDI that actually are methods on FSDI.

Tests are still failing. I am pretty sure it's due to the mocking in the test now being insufficient, however, I am finding the test code extremely hard to read and figure out!

I think the test failures actually helped uncover a bug.

it is failing here

  /**
   * {@inheritdoc}
   */
  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $settings = $field_definition->getSettings();
    $target_type_info = \Drupal::entityManager()->getDefinition($settings['target_type']);

    $target_id_data_type = 'string';
    if ($target_type_info->entityClassImplements(FieldableEntityInterface::class)) {
      $id_definition = \Drupal::entityManager()->getBaseFieldDefinitions($settings['target_type'])[$target_type_info->getKey('id')];
      if ($id_definition->getType() === 'integer') {
        $target_id_data_type = 'integer';
      }
    }

The undefined is the `uid` field on the user entity as a base field.

Confirmed. When working in\Drupal\Tests\views\Unit\EntityViewsDataTest::testBaseTableFields the base fields are never actually able to be retrieved from the mocked entity manager.

The problem is due to the fact the method is already mocked in setUp

    // This should return at least an array.
    $this->entityManager->method('getBaseFieldDefinitions')
      ->willReturn([]);

Rerolling to get tests passing.

Here is a patch, worked on getting passing tests. It was mostly due to mocked data in the test itself. That, to me, shows this fixed some kind of bug if the mocking was incorrect in the test.

I don't know if we can make this change as it currently stands. I tried to install Commerce and it broke \Drupal\commerce\CommerceEntityViewsData.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -524,7 +538,7 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
     $process_method = 'processViewsDataFor' . Container::camelize($field_type);
     if (method_exists($this, $process_method)) {
-      $this->{$process_method}($table, $field_definition, $views_field, $column_name);
+      $this->{$process_method}($table, $field_definition, $field_storage_definition, $views_field, $column_name);
     }

Anything implementing this method will have a breaking change. I don't think we need to explicitly pass the storage definition, here, since it can be retrieved from the field definition?

Simplifying the patch. This is all we cared about from the initial summary

+
+    $field_storage_definitions = array_map(function (FieldDefinitionInterface $definition) {
+      return $definition->getFieldStorageDefinition();
+    }, $field_definitions);
+
     /** @var \Drupal\Core\Entity\Sql\DefaultTableMapping $table_mapping */
-    if ($table_mapping = $this->storage->getTableMapping($field_definitions)) {
+    if ($table_mapping = $this->storage->getTableMapping($field_storage_definitions)) {

\Drupal\Core\Entity\EntityFieldManagerInterface::getBaseFieldDefinitions is expected to return FieldDefinitionInterface. Currently it's returning \Drupal\Core\Field\BaseFieldDefinition which satisfies both interfaces, so as original reporter mentioned: it just happened to work.

This should, hopefully, be a more concise patch.

+++ b/core/modules/views/src/EntityViewsData.php
@@ -53,6 +54,8 @@ class EntityViewsData implements EntityHandlerInterface, EntityViewsDataInterfac
+   * @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.x.
+   *

@@ -104,6 +107,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+   *
+   * @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.x.

I wonder if this needs explaination that it is no longer used, and that is why it is deprecated? That anyone extending this class should retrieve the field storage definition from the field definition itself?

> I wonder if this needs explaination that it is no longer used, and that is why it is deprecated?

IIRC a class such as this is considered internal, so anyone extending this should expect changes.

But from a core maintenance POV, it's a good idea to mark this, so that anyone making changes to this class in future knows they should avoid using these variables.

IIRC a class such as this is considered internal, so anyone extending this should expect changes.

It is not flagged @internal and is a handler "views_data" = "Drupal\views\EntityViewsData",

I'm assuming the message should be "get the field storage definition from the field definition, not this method." But I feel the case is small on anyone having used that method outside of core. But that is a maintainer call.

Turns out there's guidance at https://www.drupal.org/core/d8-bc-policy (though that document itself is not definitive...)

Entity handlers
Particular entity handlers should not be considered part of the public API. The interfaces which define those entity handlers though are part of the supported API.

EntityViewsData is not a particular entity handler, but a default base class to be used generically. Therefore, this applies:

Anything not marked @internal or covered by the below should not break BC without a specific reason.

I wonder if this needs explaination that it is no longer used, and that is why it is deprecated?

As far as I know, 'Yes, it does'.
Internal or not, anything marked @deprecated still needs the explanation about what to do when you currently use this and a trigger_error(). It also needs a CR and the trigger error and the deprecation need a reference to that CR.

https://www.drupal.org/core/deprecation

That, to me, shows this fixed some kind of bug if the mocking was incorrect in the test.

While its a nice way to prove it's a bug, don't we want so more explicit test for this?

for 8.8.x deprecation format

Only patch reroll

About deprecation message:
I review this issue 3024461

The text should be:
@deprecated in drupal:8.8.0 and is removed from drupal:9.0.0

But @andypost suggest only "@deprecated" ? why?

Re-rolled, with fixed deprecation messages, and s/entityManager/entityTypeManager/g.

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

In general we would not postpone a major bug fix on a test refactoring unless it was impossible to fix without it, but also I don't think there's any risk of this getting committed before that one. Trying to clean up the deprecation messages a bit more, and added a change record.

Re-rolled now that patch is in.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Made a MR on 10.1.

Back to NW for the update to the deprecation messages

Updated deprecation messages.

Per discussion around #26 if we should expand the deprecation message, I don't think we need it in the deprecation message, but I expanded the CR to at least provide some sort of example on how to replace this.

Looks nice and minimal otherwise.

This looks like good clean-up.

My only question is that both this and #2898635: bundleFieldDefinitions() are not added in EntityViewsData which depends on it don't come with any test coverage. This issue is a good net reduction in code, and I'm sure the code that's executed is covered by existing tests, but at some point do we need something to show that bundle field definitions work so it doesn't eventually get regressed. Maybe that's for #2898635: bundleFieldDefinitions() are not added in EntityViewsData though?

The problem is that Drupal doesn't have a bundle field paradigm yet, so I'm not sure how to test without adding that (like BundleFieldDefinition from Entity API module.)

Can we add the test coverage be added once Drupal adds that support? IMO #2898635: bundleFieldDefinitions() are not added in EntityViewsData needs another blocker – bringing BundleFieldDefinition into core which verifes this feature. Maybe that issue can handle that.

Added some related issues on #2898635: bundleFieldDefinitions() are not added in EntityViewsData which are all heading in that direction, which also confirms we have plenty of other issues to add test coverage meaning I think it's fine for this to go in without and unblock them.

@mglaman, the BundleFieldDefinition problem is tackled in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()

Left a comment on the MR

Only moving to NW for the open thread in the MR.

DeprecatedServicePropertyTrait is used for when there is a \Drupal::service() replacement call
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Dependenc...

So deprecation with no replacement I think is the appropriate thing to do here.

I rebased against 11.x and created a new MR.
Hopefully this can be merged for Drupal 11.2.0

11.2 is very feasible.

Probably should add deprecation test coverage though

Isn't a bit too much adding deprecation tests for a protected method?

I'm curious about how to mark them so they will be removed along the deprecated code.

I tried to look for examples but couldn't find much. I guess it would be about setting up an artificial code path with a class that extends from EntityViewsData that calls the deprecated method.

I'm not sure about needing deprecations for protected methods.

However to answer your second question, the nature of the test will necessitate its removal when the method is removed. The test will no longer see the deprecation so it will fail when you remove the function.

You then have to remove the deprecation test, it takes care of itself.

Added test for the deprecation so back to NR

No unanswered questions here, updated credit and hid files.

The suggested changes make sense to me, and if anything it makes the code more readable anyway.

Committed 42fc80c and pushed to 11.x. Thanks!

Also published the change record.

Is there a backport for 10.6 for this? None of the patches work and upgrading from D9 is broken.