Problem/Motivation

In D8.7 and D8.8, Drupal\Core\Entity\ContentEntityStorageBase::cleanIds switched to loading field definitions with getActiveFieldStorageDefinitions, which fails to return all definitions in the event an external entity is not cached locally. See #3 below for more details.

Proposed resolution

Override ContentEntityStorageBase::cleanIds and use getFieldStorageDefinitions instead.

Remaining tasks

Submit patch.
Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

Original report by ericras

I'm following the examples and also building my own storage client.

In each case I get a fatal error:
Error: Call to a member function getType() on null in /core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php on line 990

which comes from the call to cleanIds() in ExternalEntityStorage:

  protected function getFromExternalStorage(array $ids) {
    $entities = [];

    if (!empty($ids)) {
      // Sanitize IDs. Before feeding ID array into buildQuery, check whether
      // it is empty as this would load all entities.
      $ids = $this->cleanIds($ids);
    }
    ....

Taking that out 'fixes' the problem. I'm not sure if sanitizing is necessary there, and I'm also unsure of how to fix it if its left in.

CommentFileSizeAuthor
#7 external_entities-override_cleanids-3053465-7.patch1.07 KBclemens.tolboom
#5 external_entities-override_cleanids-3053465-5.patch1.08 KBchris burge

Comments

ericras created an issue. See original summary.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

I got the same error while trying #3055615: Self reflect to Drupal using REST API.

Does it help removing fields?

chris burge’s picture

chris burge commented
Assigned: Unassigned » chris burge
Priority: Major » Critical

This bug is caused by #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code, which was committed to D8.7 and D8.8 on 22 March, 2019.

The following change was made to core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:

@@ -985,7 +985,7 @@ protected function populateAffectedRevisionTranslations(ContentEntityInterface $
   *   The sanitized list of entity key values.
   */
  protected function cleanIds(array $ids, $entity_key = 'id') {
-   $definitions = $this->entityFieldManager->getBaseFieldDefinitions($this->entityTypeId);
+   $definitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($this->entityTypeId);
    $field_name = $this->entityType->getKey($entity_key);
    if ($field_name && $definitions[$field_name]->getType() == 'integer') {
      $ids = array_filter($ids, function ($id) {

Per the API docs for EntityFieldManager::getActiveFieldStorageDefinitions, this method return "the active field storage definitions for a content entity type." This causes an issue in External Entities.

If the external entity is cached locally, then no problem. If the external entity is not cached locally, then definitions are missing. Specifically, we need the 'id' definition.

See comments in code below to step through cleanIds():

protected function cleanIds(array $ids, $entity_key = 'id') {
    $definitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($this->entityTypeId);
    // In the $definitions array, 'id' is not returned
    $field_name = $this->entityType->getKey($entity_key);
    // $field name is returned as 'id'
    if ($field_name && $definitions[$field_name]->getType() == 'integer') {
      // Since $definitions['id'] doesn't exist, you get 'Error: Call to a member function getType() on null in /core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php on line 990'
      $ids = array_filter($ids, function ($id) {
        return is_numeric($id) && $id == (int) $id;
      });
      $ids = array_map('intval', $ids);
    }

    return $ids;

If we use getFieldStorageDefinitions instead, then we get all of the definitions and no error. I'm going to be submitting a patch to override the cleanIds method.

I've changed the priority to critical because this bug renders the module unusable in D8.7+ and there are no workarounds.

chris burge’s picture

chris burge commented
Title: Sanitizing IDs errors out in getFromExternalStorage() » Override ContentEntityStorageBase::cleanIds to prevent fatal error introduced in D8.7
Issue summary: View changes
chris burge’s picture

chris burge commented
Assigned: chris burge » Unassigned
Status: Active » Needs review
StatusFileSize
new external_entities-override_cleanids-3053465-5.patch1.08 KB
chris burge’s picture

chris burge commented
Issue summary: View changes
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
StatusFileSize
new external_entities-override_cleanids-3053465-7.patch1.07 KB

I had to change $entityKey in 'id' + $field_name = $this->entityType->getKey('id'); and can confirm this fixes my wikipedia test.

So will commit soon

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Needs review » Fixed

@Chris Burge thanks for fixing.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Strange as 'author' is not remembered.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

Status: Fixed » Closed (fixed)

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