Fatal error on admin/content

Views was moved into core on Drupal 8, so I'm moving this to the core issue queue.

Just hit this on a Production site, running Drupal 8.2.8

Providing a patch. I'm thinking we need to ensure that the returned entities implement the required method, not just that they're not null.

This is the source of the views results containing a null value for an Entity.

In core/modules/views/src/Plugin/views/query/Sql.php:1602 we assign

$entity = NULL

and then a few lines later at core/modules/views/src/Plugin/views/query/Sql.php:1609

$results[$index]->_relationship_entities[$relationship_id] = $entity

See the complete code here:

  /**
   * Sets entities onto the view result row objects.
   *
   * This method takes into account the relationship in which the entity was
   * needed in the first place.
   *
   * @param mixed[][] $ids
   *   A two dimensional array of identifiers (entity ID / revision ID) keyed by
   *   relationship.
   * @param \Drupal\Core\Entity\EntityInterface[] $entities
   *   An array of entities keyed by their identified (entity ID / revision ID).
   * @param \Drupal\views\ResultRow[] $results
   *   The entire views result.
   *
   * @return \Drupal\views\ResultRow[]
   *   The changed views results.
   */
  protected function assignEntitiesToResult($ids, array $entities, array $results) {
    foreach ($ids as $index => $relationships) {
      foreach ($relationships as $relationship_id => $id) {
        if (isset($entities[$id])) {
          $entity = $entities[$id];
        }
        else {
          $entity = NULL;
        }

        if ($relationship_id == 'none') {
          $results[$index]->_entity = $entity;
        }
        else {
          $results[$index]->_relationship_entities[$relationship_id] = $entity;
        }
      }
    }
    return $results;
  }

No sooner had we fixed the problem in core/modules/views/src/Plugin/views/query/Sql.php it surfaced in the Cache Plugin core/modules/views/src/Plugin/views/cache/CachePluginBase.php method CachePluginBase::getRowCacheTags.

I'm rolling a new patch, that fixes it for all.

This addresses the issue now in both the affected methods, using similar pattern in both classes.

Also fixed a few of the phpcs violations, though there are many more in existing code.

Please review.

So, I'm looking at the tests, trying to see if we can write a test that replicates the problem and verifies our fix addresses it. That would prevent a regression and validate what we're doing.

I can see this test

core/modules/views/tests/src/Unit/Plugin/query/SqlTest.php:75

\Drupal\Tests\views\Unit\Plugin\query\SqlTest::testGetCacheMaxAge()

It might be the place to add an additional assertion, to replicate the issue.

My best guess as to the cause

I think we have relationships being created to entities, that are in a workflow state such that they cannot be in the Views result. So, maybe a content editor creates a new relationship to an entity, but the entity revision for the relationship is in a draft state, or some other state, that means it cannot be loaded on the admin/content page.

That results in a NULL entity being added to the results, and then we see the cache methods blow up because they're assuming that every entity is an Entity object.

That's my guess anyway. Writing that into a unit test, well, that is the next challenge.

Working on this issue with @tim.plunkett at DrupalconVienna

While installing through Simplytest.me I encountered the following error and could not continue.

Fatal error: Interface 'League\OAuth2\Server\Repositories\ClientRepositoryInterface' not found in /home/r27ne/www/profiles/lightning/modules/contrib/simple_oauth/src/Repositories/ClientRepository.php on line 10

The IS mentions Lightning, so let's move this into Lightning's queue until we can prove that it is reproducible on vanilla Drupal.

I think #10 is because Lightning's D.O tarball doesn't include required composer dependencies and that's where Simplytest.me gets its code for Lightning.

I'll try to isolate this and see if I can reproduce on vanilla Drupal.

@phenaproxima

While the IS mentions lightning, I encountered and patched this on a Drupal 8.x site without lightning code installed.

First hit on Drupal 8.2. Have upgraded to Drupal 8.4.0-rc2 and removed the patch, with same fatal error on Content page. Have reapplied patch to address.

I'm not sure what our Content team created that surfaced the issue, but I have my suspicions that it could be to do with Paragraphs and/or Workflow (aka. Workbench Moderation). Paragraphs because they involve entity relationships, which features in the Views problem. Workbench Moderation because it may explain why certain relationship entities are not available in the Views query result.

I am using Drupal 8.3.7 with Deploy 1.0.0-beta1 and Lightning 2.1.8

@chOP Testing your patch results in the following:

$ patch -p1 < fatal_error_on-2903942-7.patch
patching file core/modules/views/src/Plugin/views/cache/CachePluginBase.php
patching file core/modules/views/src/Plugin/views/query/Sql.php
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 1642 (offset -1 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/views/src/Plugin/views/query/Sql.php.rej

Rej file

$ cat core/modules/views/src/Plugin/views/query/Sql.php.rej
--- core/modules/views/src/Plugin/views/query/Sql.php
+++ core/modules/views/src/Plugin/views/query/Sql.php
@@ -6,6 +6,7 @@
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Database\Database;
 use Drupal\Core\Database\Query\Condition;
+use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\views\Plugin\views\display\DisplayPluginBase;

After the patch, I am able to access /admin/content

Further, I don't know if this is related but I cannot generate content and receive the following error after "saving" a node through the UI

Fatal error: Call to a member function getTranslation() on null in /var/www/html/docroot/core/modules/node/src/NodeViewBuilder.php on line 91

Note: The above error I'm receiving is due to a dependency of Deploy module. Removing Deploy does not fix this.

@brandonratz

The patch was rolled against the 8.5.x-dev branch, so perhaps won't apply seamlessly to Drupal 8.3.x

I can re-roll a patch against the appropriate 8.3 branch if you like. May help others who see the error in earlier Drupal versions.

Your Deploy related entity save error looks very similar to the one we're trying to address in Views, but looks like it's coming from elsewhere. Could be worth opening a new issue, and linking as related.

Drupal 8.3 version, may help someone.

Re-rolled against the latest 8.5.x branch for review.

To be clear, we're seeing this Fatal error still when the patch is not applied and our site does not make use of either Deploy module or the Lightning modules.

IMHO this issue could surface in any module where a developer expects the return type for getAllEntities() to match the Docblock, which says @return \Drupal\Core\Entity\EntityInterface[]. Without this patch, the Docblock comment is completely wrong, as the returned array could include NULL values. Hence the Fatal error.

My patch above attempts to fix this by explicitly returning values that match the Docblock assertion.

Another approach though could be to stop the NULL values being stored at their source, in the method assignEntitiesToResult() - see #2903942-5: Fatal error on admin/content above. This however would also involve finding out why they're needed in the result and addressing that too.

Without a test or steps to reproduce on a clean Drupal install we still have no idea if we are fixing the root cause of this problem or just treating the symptoms.

While I agree that enforcing getAllEntities() to adhere to its promise of returning \Drupal\Core\Entity\EntityInterface[] is a nice step, its more a task then a bug and I'm not sure we should ever allow the row entity to be empty. With the current proposed fix we just silently skip over any missing entities, without knowing the cause, we might just be silently allowing bad/malformed/bugged configuration. Maybe we should be throwing an Exception. Without knowing the situations where this occurs, we don't know the right solution.

+1 @Lendude

Agree totally. If I can steal the time to prepare some tests that reproduce the core problem, I will.

patch added for version #9.3.3

@ pviquez@pabloviquez.com, Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Is this issue still a problem?

There has been no activity here for 5 years (not counting the reroll) when a maintainer asked for steps to reproduce. See #20.

If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core"). Thanks.

Since we need more information to move forward with this issue, I am setting the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

I just came across this issue with 9.5.x. so it is still active.

@Lendude The root cause for this bug is that the object is left in an invalid internal state. Everything else depends on this bug. Either the triggering code is not fixed because the error is accepted by assignEntitiesToResult or several functions in core/modules/views/src/Plugin/views/query/Sql.php are broken and shoudl be fixed that the invalid state is correct.

A short scan over my Drupal installations reveals that it's nearly impossible to decide what is intended.
Here are the results:

$ grep -e '_relationship_entities' -r .|grep -v test
./core/modules/views/src/ResultRow.php:  public $_relationship_entities = [];
./core/modules/views/src/ResultRow.php:   * Resets the _entity and _relationship_entities properties.
./core/modules/views/src/ResultRow.php:    $this->_relationship_entities = [];
./core/modules/views/src/Plugin/views/cache/CachePluginBase.php:    if (!empty($row->_relationship_entities)) {
./core/modules/views/src/Plugin/views/cache/CachePluginBase.php:	    foreach ($row->_relationship_entities as $key => $entity) {
./core/modules/views/src/Plugin/views/cache/CachePluginBase.php:    $row_data = array_diff_key((array) $row, array_flip(['index', '_entity', '_relationship_entities'])) + $this->getRowCacheTags($row);
./core/modules/views/src/Plugin/views/field/FieldPluginBase.php:    elseif (isset($values->_relationship_entities[$relationship_id])) {
./core/modules/views/src/Plugin/views/field/FieldPluginBase.php:      return $values->_relationship_entities[$relationship_id];
./core/modules/views/src/Plugin/views/query/QueryPluginBase.php:   * $result->_relationship_entities[$relationship_id];
./core/modules/views/src/Plugin/views/query/Sql.php:   * $result->_relationship_entities[$relationship_id];
./core/modules/views/src/Plugin/views/query/Sql.php:          $results[$index]->_relationship_entities[$relationship_id] = $entity;
./core/modules/views/src/Plugin/views/query/Sql.php:      foreach ($row->_relationship_entities as $entity) {

As you can see _relationship_entities is a public member. So CachePluginBase Sql.php cannot rely on non-null entries in _relationship_entities. So my suggestion is:

• In the next major release: Throw an exception if an entity is not valid in assignEntitiesToResult
• In all other releases issue a warning instead and silently accept the invalid state without crashing in getAllEntities

Currently code must always check that a certain key contains no null value. With the fix in place people must check that the key exists. What you prefer is a question of coding style and which one is faster depends on the actual application.