Multiple usages of FieldPluginBase::getEntity do not check for NULL, leading to WSOD

This does fix an issue for me where a View gets an empty reference field from a node. Just hard to tell if the fixes are all related to this issue since they are fixed in a different way in some cases:

1. +++ b/core/modules/media_library/media_library.module
   @@ -141,9 +141,10 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   -        $form['media_bulk_form'][$key]['#title'] = t('Select @label', [
   +        $title = $media ? t('Select @label', [
              '@label' => $media->label(),
   -        ]);
   +        ]) : '';
   +        $form['media_bulk_form'][$key]['#title'] = $title;
   

   I'm not sure how this resolves to a WSOD

2. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -124,7 +124,9 @@ public function query() {
   -    BubbleableMetadata::createFromObject($access)->applyTo($build);
   +    if ($access) {
   +      BubbleableMetadata::createFromObject($access)->applyTo($build);
   +    }
   

   This also looks unrelated to the problem.

In my condition, #2 & #3 still get TypeError:
TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\RenderedEntity::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given

I fix this with below:

--- core/modules/views/src/Plugin/views/field/RenderedEntity.php
+++ core/modules/views/src/Plugin/views/field/RenderedEntity.php
@@ -105,7 +105,7 @@
    * {@inheritdoc}
    */
   public function render(ResultRow $values) {
-    $entity = $this->getEntityTranslation($this->getEntity($values), $values);
+    $entity = $this->getEntity($values)?$this->getEntityTranslation($this->getEntity($values), $values):NULL;
     $build = [];
     if (isset($entity)) {
       $access = $entity->access('view', NULL, TRUE);

In this patch:

• reroll of #2
• removed the ternary statements, which i found confusing, and replaced with if statements. I find this more readable, and makes for a cleaner patch.
• added type-checking before calls to getEntityTranslation(), per #6
• reverted the API change to getEntityTranslation() - I didn't see a good reason for this, and changing a public API would prevent this from getting into 7.x
• a few additional adjustments around calls to getUrlInfo() and checkUrlAccess() to accommodate NULL return values

re @joelpittet comments from #5,
1) hopefully this is clearer now that it's using if statements instead of the long ternary
2) the changes in LinkBase are necessary because of calls to checkUrlAccess() and getUrlInfo(). Hopefully the new patch is also clearer in this regard

This should have been updated in 2016 when this issue landed:
#2657826: FieldHandlerInterface::getEntity() can also return NULL

Not sure how this is currently evading all test coverage, nor the best way to add tests around this.
If someone wants to point me in the right direction, I can take a stab at writing some tests.

Generally, I don't think it's feasible to write tests for all the cases where callers may fail to type-check return values - that's pretty basic, could be caught e.g. by static analysis.

The patch in #7 successfully addressed this particular issue in my environment.

In my instance, I have a view with a relationship based upon an entity reference; aka Content using field_name: Relate each Content with a field_name set to the content. I also have a "Content: Rendered entity" field using that relationship.

Pre-patch, in the cases where that relationship did not exist (the node was NOT referenced and had no relationship), I got the "TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\RenderedEntity::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in..." error.

Post-patch, no errors.

Patch #7 fixed my issues as well.

Can we set this to RTBC to get into 8.7? Deadline for core patches is March 8.

Some of these changes already made it into 8.7. This issue needs another look and a preroll.

This is a straight re-roll of #7

@douggreen, i didn't see anything which had already been committed. Did you have something specific in mind?

The patch looks good!

1. +++ b/core/modules/comment/src/Plugin/views/field/LinkApprove.php
   @@ -19,7 +19,11 @@ class LinkApprove extends LinkBase {
   +    $entity = $this->getEntity($row);
   +    if (!$entity) {
   +      return NULL;
   +    }
   +    return Url::fromRoute('comment.approve', ['comment' => $entity->id()]);
   

   This can be rewritten to:

       if ($entity = $this->getEntity($row)) {
         return Url::fromRoute('comment.approve', ['comment' => $entity->id()]);
       }
   

   The default return value is NULL. Use this solution in multiple places for this patch.

2. +++ b/core/modules/comment/src/Plugin/views/field/LinkReply.php
   @@ -21,6 +21,9 @@ class LinkReply extends LinkBase {
   +    if (!$comment) {
   +      return NULL;
   +    }
   

   Better is to test if $comment is an instanceof Drupal\Core\Entity\EntityInterface. Use this solution in multiple places for this patch.

3. All these changes need automatic tests. And yes, that is some work.

All these changes need automatic tests. And yes, that is some work.

Can you expand on this feedback please?
Like I said previously, I don't see how it's feasible to write tests for all the cases where callers may fail to type-check return values.

@AaronBauman: For each change/fix that your patch has, can you create a view which results into a WSOD and with your change/fix it does not. You can then write an automated test with your view and test that you do not get a WSOD. You create 2 patches, 1 with only the test and that one should fail. The second patch is the one with the test and the fix and that one should pass. I can help you with making the tests. Post for one change/fix the view with the problem and together we shall work out how to make that into an automated test. If you want to of course.

Here are some tests that should fail.

Will post the patch which fixes them after testbot finishes.

These tests seem very overblown, given that the patch is a set of simple type checks.

And here are the tests, along with fixes.

Omitting the interdiff, because #15 no longer applies so it's not particularly helpful.

Patch in #7 solved my problem on 8.7.5 (although in a couple of places the offset was too great and I had to patch those manually, if anyone else is patching 8.7) so thanks for the work on this; it will be great when this gets committed to 8.8.

I stumbled over this error too a few minutes ago.

Somehow a node was not completely deleted. The row in the "node" table is gone, but the row in "node_field_data" is still there plus rows in some of the field tables and revision tables.

The /admin/content view created a WSOD.

I couldn't delete or edit or view this node via the Drupal interface.

I deleted the user (cancel user with deletion of all nodes), who owned the node. After that the admin/content view was ok again.
But the broken node was still not removed from the database.

What are the steps to remove all orphaned entries? Is it possible to just insert a row in the "node" table to make the node loadable/deletable again?

I don't want to manually delete rows in the database.

I am having the same issue but only on the /admin/commerce/products view.

Still reviewing, but so far patch #15 is working well.

Project: Drupal Core 8.7.4 and Drupal Commerce 2.13.

Encountered this problem today when I tried to render the latest comment of a node.

Since some nodes didn't have a comment, I got a the TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\RenderedEntity::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in....

The relevant part of patch #15 worked well for me:

diff --git a/core/modules/views/src/Plugin/views/field/RenderedEntity.php b/core/modules/views/src/Plugin/views/field/RenderedEntity.php
index f70b03f5d9..4539866605 100644
--- a/core/modules/views/src/Plugin/views/field/RenderedEntity.php
+++ b/core/modules/views/src/Plugin/views/field/RenderedEntity.php
@@ -146,15 +146,16 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    * {@inheritdoc}
    */
   public function render(ResultRow $values) {
-    $entity = $this->getEntityTranslation($this->getEntity($values), $values);
+    if (!$entity = $this->getEntity($values)) {
+      return [];
+    }
+    $entity = $this->getEntityTranslation($entity, $values);
     $build = [];
-    if (isset($entity)) {
-      $access = $entity->access('view', NULL, TRUE);
-      $build['#access'] = $access;
-      if ($access->isAllowed()) {
-        $view_builder = $this->entityTypeManager->getViewBuilder($this->getEntityTypeId());
-        $build += $view_builder->view($entity, $this->options['view_mode']);
-      }
+    $access = $entity->access('view', NULL, TRUE);
+    $build['#access'] = $access;
+    if ($access->isAllowed()) {
+      $view_builder = $this->entityTypeManager->getViewBuilder($this->getEntityTypeId());
+      $build += $view_builder->view($entity, $this->options['view_mode']);
     }
     return $build;
   }

Patch #21 applies cleanly(8.7.x) and removes WSOD.
I had a problem with media overview page.

Still, need to find out how data got deleted in the main media table and data is present in media_field_data & other fields table. Possible due to script broke abnormally.

Reroll of #21 for D8.7

So the current tests in #19 and #21 all work with Mock views ($view = $this->createMock(ViewExecutable::class))
Those pass the tests for the actual issue here, but then fail later on because they're not "real" views with all their "real" properties.

Let's see if that's fixable by adding some "real test" views (if that makes even any sense to someone...)

#21 with (trying to) addres #16.1

Right...let's pretend that never happened...

#21 with trying to addres #16.1

Uploaded the same patch twice instead of patch + interdiff

phpcs fixes and (hopefully) one less failure.

btw It does change return value of the actions - maybe instead of that just properly catch exceptions in calling code and document exceptions for actions?

Tests changed

@YurkinPark Thank you!

- Fixed 2 Coding Standard Messages
- Removed changes in core/modules/views/tests/src/Kernel/Handler/FieldEntityLinkTest.php. That only contained code formatting changes which I've inadvertently introduced in #33
- Seperated the tests into a Test-Only.patch

If #34 needs to be addressed, at least we now should have a set of valid tests, I hope.
Otherwise: Review away! :)

Reroll after #2925816: Views plugin "Rendered Entity" must add langcode in render function

Removing the 'Needs tests' tag after #36.

Reroll for 8.7.x

I also found this error when building a search result view for all content types:

TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\RenderedEntity::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in

The view in question has a product relationship and a product variation relationship. The only field that threw the error in the view was the rendered entity for the add to cart form. This view also has product variation price field, as well as a taxonomy reference field from the product which haven't caused any noticeable issues.

The view would render fine when being filtered to only the content referencing products, but when any other content types without the product reference were trying to be rendered, I got the white screen. Hope this helps with troubleshooting.

Which patch is best to use right now for 8.7.8?
Thanks!

Which patch is best to use right now for 8.7.8?

#42

I took the patch from #38 and removed all the fixes. I want to check if the new tests will fail if the fixes are removed. Please do not give me commit credits for this creating this patch.

Jibran, isn't #42 failing tests right now? I'm new to this stuff.

@ToneLoc: The 2 test failures are not related to the patch. So the testbot is green for the patch. You can do a review. See: https://www.drupal.org/patch/review.

Update: The failure is for by this patch added test : Drupal\Tests\media_library\Unit\MediaLibrarySelectFormTest. So back to needs work.

Let's see if this gets us closer to green

Added #3058853: Drupal 8.x on Postgresql 12.

The patch look good. Some (minor) points:

1. +++ b/core/modules/media_library/media_library.module
   @@ -271,7 +271,10 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +        if (!$media = $view->field['media_bulk_form']->getEntity($view->result[$key])) {
   +          $form['media_bulk_form'][$key]['#title'] = '';
   +          continue;
   +        }
   

   Could we, for better readability, rewrite this to:

     if ($media = $view->field['media_bulk_form']->getEntity($view->result[$key])) {
       $form['media_bulk_form'][$key]['#title'] = t('Select @label', [
         '@label' => $media->label(),
       ]);
     }
     else {
       $form['media_bulk_form'][$key]['#title'] = '';
     }
   

2. +++ b/core/modules/media_library/media_library.module
   @@ -271,7 +271,10 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   diff --git a/core/modules/media_library/media_library.module.orig b/core/modules/media_library/media_library.module.orig
   
   diff --git a/core/modules/media_library/media_library.module.orig b/core/modules/media_library/media_library.module.orig
   new file mode 100644
   
   new file mode 100644
   index 0000000000..84c0f67732
   
   index 0000000000..84c0f67732
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/media_library/media_library.module.orig
   
   +++ b/core/modules/media_library/media_library.module.orig
   +++ b/core/modules/media_library/media_library.module.orig
   @@ -0,0 +1,540 @@
   

   and

   +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -55,7 +55,10 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   diff --git a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig
   
   diff --git a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig
   new file mode 100644
   
   new file mode 100644
   index 0000000000..2d0702bd4d
   
   index 0000000000..2d0702bd4d
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig
   
   +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig
   +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php.orig
   @@ -0,0 +1,153 @@
   

   Why are these files added? Wrong patch?!

3. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -55,7 +55,10 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +      if (!$entity = $this->getEntity($row)) {
   +        $form[$this->options['id']][$row_index] = [];
   +        continue;
   +      }
   

   Could we, for better readability, rewrite this to:

       if ($entity = $this->getEntity($row)) {
         $form[$this->options['id']][$row_index] = [
           '#type' => 'checkbox',
           '#title' => $this->t('Select @label', [
             '@label' => $entity->label(),
           ]),
           '#title_display' => 'invisible',
           '#return_value' => $entity->id(),
         ];
       }
       else {
         $form[$this->options['id']][$row_index] = [];
       }
   

4. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,74 @@
   +class MediaLibrarySelectFormTest extends UnitTestCase {
   

   Should this test class not have a teardown method, because the container is being set?

5. +++ b/core/modules/user/src/Plugin/views/field/Permissions.php
   @@ -87,7 +87,10 @@ public function preRender(&$values) {
   +      if (!$user = $this->getEntity($result)) {
   +        continue;
   +      }
   +      $user_rids = $user->getRoles();
   

   Could we, for better readability, rewrite this to:

     if ($user = $this->getEntity($result)) {
       $user_rids = $user->getRoles();
       $uid = $this->getValue($result);
   
       foreach ($user_rids as $rid) {
         $rids[$rid][] = $uid;
       }
     }
   

6. +++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
   @@ -308,7 +308,11 @@ public function viewsForm(&$form, FormStateInterface $form_state) {
   +        if (!$entity = $this->getEntity($row)) {
   +          $form[$this->options['id']][$row_index] = [];
   +          continue;
   +        }
   +        $entity = $this->getEntityTranslation($entity, $row);
   

   Could we, for better readability, rewrite this to:

     if ($entity = $this->getEntity($row)) {
       $entity = $this->getEntityTranslation($entity, $row);
   
       $form[$this->options['id']][$row_index] = [
         '#type' => 'checkbox',
         // We are not able to determine a main "title" for each row, so we can
         // only output a generic label.
         '#title' => $this->t('Update this item'),
         '#title_display' => 'invisible',
         '#default_value' => !empty($form_state->getValue($this->options['id'])[$row_index]) ? 1 : NULL,
         '#return_value' => $this->calculateEntityBulkFormKey($entity, $use_revision),
       ];
     }
     else {
       $form[$this->options['id']][$row_index] = [];
     }
   

7. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -26,6 +26,9 @@ public function render(ResultRow $row) {
   +      if (!$urlInfo = $this->getUrlInfo($row)) {
   +        return '';
   +      }
          return $this->getUrlInfo($row)->toString();
   

   Could we, for better readability, rewrite this to:

     if ($urlInfo = $this->getUrlInfo($row)) {
       return $urlInfo->toString();
     }
     return '';
   

8. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -37,6 +40,9 @@ protected function renderLink(ResultRow $row) {
   +    if (!$entity) {
   +      return NULL;
   +    }
   

   Could we, for better readability, rewrite this to:

     if ($entity = $this->getEntity($row)) {
       if ($this->languageManager->isMultilingual()) {
         $entity = $this->getEntityTranslation($entity, $row);
       }
       return $entity->toUrl($template)->setAbsolute($this->options['absolute']);
     }
   

9. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -177,6 +177,9 @@ public function query() {
   +    if ($access === NULL) {
   +      return '';
   +    }
   

   Could we, for better readability, rewrite this to:

     if ($access = $this->checkUrlAccess($row)) {
       $build = ['#markup' => $access->isAllowed() ? $this->renderLink($row) : ''];
       BubbleableMetadata::createFromObject($access)->applyTo($build);
       return $build;
     }
     return '';
   

@Spokje: Thank you adding the tests.

Patch from #28 fixed my issue from comment #43. Patch #48 would not install via composer, not sure if that is because of the 2 fails?

@ToneLoc

Patch #48 would not install via composer

Any error messages?

I didn't copy it down, I think it was just the standard composer message could not apply patch...

Few improvements to the patch.

I found examples in Core that explicitly return NULL instead of not returning anything in such cases. I updated the patch accordingly.

Additionally, I updated the patch to assign a local $entity variable in one step instead of doing it in an if condition. There are both examples in Core and I am not sure if there is an official convention for these cases. I think the changed way results in fewer lines changed.

Also, addressed #51:

1. Fixed with a ternary operator
2. Removed
3. Fixed
4. Hm, left as it is for now?
5. Fixed
6. Fixed with a ternary operator
7. Fixed
8. Fixed
9. Fixed

@ToneLoc The patch in #42 applied cleanly for me with composer on 8.7.9, so it's possible updating core might solve your problem.

This patch should fix test fails in Drupal\Tests\media_library\Unit\MediaLibrarySelectFormTest appeared in #49 https://www.drupal.org/pift-ci-job/1467125.

#57 patch rerolled on top of Drupal 8.7.9.

This fixes the test fail in Drupal\Tests\views\Unit\Plugin\views\field\RenderedEntityTest I introduced in #55.

At first, I reverted changes from #49 made in Drupal\views\Plugin\views\field\RenderedEntity, but when I look into the documentation of ::render() it says:

   * @return string|\Drupal\Component\Render\MarkupInterface
   *   The rendered output. If the output is safe it will be wrapped in an
   *   object that implements MarkupInterface. If it is empty or unsafe it
   *   will be a string.

That said, the return value should be an empty string if there is no entity.

It all looks good to me. Just a couple of minor points left:

1. +++ b/core/modules/node/src/Plugin/views/field/RevisionLink.php
   @@ -21,10 +21,16 @@ class RevisionLink extends LinkBase {
   +    if ($node) {
   +      // Current revision uses the node view path.
   +      return !$node->isDefaultRevision() ?
   +        Url::fromRoute('entity.node.revision', [
   +          'node' => $node->id(),
   +          'node_revision' => $node->getRevisionId(),
   +        ]) :
   +        $node->toUrl();
   +    }
   +    return NULL;
   

   Can we combine the first lines to and remove the return null (returning null is the default action):

   +    if ($node && !$node->isDefaultRevision()) {
   +      // Current revision uses the node view path.
   +      return Url::fromRoute('entity.node.revision', [
   +          'node' => $node->id(),
   +          'node_revision' => $node->getRevisionId(),
   +        ]) :
   +        $node->toUrl();
   +    }
   

2. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -37,10 +38,13 @@ protected function renderLink(ResultRow $row) {
   +    return NULL;
   

   This line can be removed. It is the default action in php.

3. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -188,12 +191,14 @@ public function render(ResultRow $row) {
   +    return NULL;
   

   The same.

I have tried to make changes as suggested in #63. Please review it.

Almost there:

+++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
@@ -177,9 +177,11 @@ public function query() {
+    if ($access) {
+      $build = ['#markup' => $access->isAllowed() ? $this->renderLink($row) : ''];
+      BubbleableMetadata::createFromObject($access)->applyTo($build);
+      return $build;
+    }

The line return ''; should not have been removed.

Oh sorry I removed that also.

Here I have recreated patch, please review it.

I have recreated the patch.

@ravi.shankar: You found my mistake from comment #63.1. If the patch is green I shall give it a RTBC.

The patch looks good to me.
Tests have been added.
It is RTBC for me.

Great work everybody!

Wow, I take my eyes off the ball for a few days and stuff got done.

Thanks @mbovan and @ravi.shankar for patching away and special thanks for @daffie for the excellent review work! 🙏

Could not apply #68 with composer again for some reason. Patch from #28 is still applying cleanly via composer.

#68 neither with core 8.7.8 nor with 8.7.10. I did not try 8.7.9

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2019-11-13/3007424-64.patch

Maybe it's a conflict with this other core patch I have: https://www.drupal.org/files/issues/2019-06-27/use-text-editor-for-summa...

haha. oops that's patch 64. :)

Same for #68 tho.

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2019-11-14/3007424-68.patch

@ToneLoc Patch #68 (and #64 as well) are tested against D8.9 and might work on D8.8.
As you've found out, they won't apply on D8.7.x.

@Spokje thanks. I didn't even know 8.9 was a thing yet. 😂

Back to RTBC as to #70
Testbot Failure was a random fluke as the next test I ran on the same patch was all green.

Confirming that #58 applies on 8.7.11. Thanks, @mbovan!

#68 this works correctly for me in 8.6.1 Thanks!!

#68 works for me in 8.8.1. Thanks!

#68 ok for 8.8.5 php7.2 mySQL 5.6

Rerolled against 9.1.x-dev

Tried to fix failure tests

Next try

Nice job @aleevas!

+++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
@@ -0,0 +1,89 @@
+    $this->assertNotEmpty($form, print_r($field->view->result, 1));

It looks like this was left over debugging?

Patch applies for 9.1.x-dev in my local environment.

Thanks @narendra.rajwar27, can you please provide an interdiff?

sure @acbramley, interdiff for the patch in comment #94

There is Patch issue in comment #94, as applied patch files were not neatly checked out in my local setup. which caused test cases removal, so updating again. Changes added as suggested in comment #91

Patch updated for failed test cases.

Patch looks good.

1. +++ b/core/modules/comment/tests/src/Unit/Plugin/views/field/EntityLinkTest.php
   @@ -0,0 +1,31 @@
   +    $this->assertEquals('', $field->render($row));
   

   Can we change this to: "$this->assertNull($field->render($row));". On multiple places.

2. +++ b/core/modules/comment/tests/src/Unit/Plugin/views/field/LinkApproveTest.php
   @@ -0,0 +1,48 @@
   +use Drupal;
   ...
   +    Drupal::setContainer($container);
   

   You can remove the use-statement, when you change "Drupal::setContainer($container);" tot "\Drupal::setContainer($container);". On multiple places.

3. +++ b/core/modules/comment/tests/src/Unit/Plugin/views/field/LinkApproveTest.php
   @@ -0,0 +1,48 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function tearDown(): void {
   +    parent::tearDown();
   +    $container = new ContainerBuilder();
   +    Drupal::setContainer($container);
   +  }
   

   I think we need to do this in every unit test we are adding with setContainer() stuff. Missing it in the class MediaLibrarySelectFormTest.

4. +++ b/core/modules/contact/src/Plugin/views/field/ContactLink.php
   @@ -30,7 +30,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
      protected function getUrlInfo(ResultRow $row) {
   -    return Url::fromRoute('entity.user.contact_form', ['user' => $this->getEntity($row)->id()]);
   +    $entity = $this->getEntity($row);
   +    return $entity ? Url::fromRoute('entity.user.contact_form', ['user' => $entity->id()]) : NULL;
   

   Missing automated testing for this change. You can test a protected method by using reflection. On multiple places.

5. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,89 @@
   +    $form = [];
   +    $this->assertEmpty($form, $field->options['id']);
   

   The variable $form is not necessary. On multiple places.

6. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,89 @@
   +   * Tests the viewsForm method.
   

   This line can be removed. There is already "@covers ::viewsForm". On multiple places.

7. +++ b/core/modules/comment/tests/src/Unit/Plugin/views/field/LinkApproveTest.php
   @@ -0,0 +1,48 @@
   +   * Tests the render method.
   
   +++ b/core/modules/comment/tests/src/Unit/Plugin/views/field/LinkReplyTest.php
   @@ -0,0 +1,47 @@
   +   * Tests the render method.
   
   +++ b/core/modules/contact/tests/src/Unit/ContactLinkTest.php
   @@ -0,0 +1,47 @@
   +   * Tests the render method.
   

   Please change to "@covers ::render"

8. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,89 @@
   +    $view_entity = $this->getMockBuilder('Drupal\views\Entity\View')
   

   Please change to: "$view_entity = $this->getMockBuilder(View::class)". On multiple places.

9. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -189,7 +193,7 @@ protected function checkUrlAccess(ResultRow $row) {
   -   * @return \Drupal\Core\Url
   +   * @return \Drupal\Core\Url|null
       *   The URI elements of the link.
   

   We are adding NULL as a possible return value. Please add when that is the case. On multiple places.

@daffie thank you for the review.
About point 1:
I think, that the method $this->assertEmpty is more fit for this case.
I've fixed all notice from #104 except point 4.
Still needs to work on it

Patch looks better.

1. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,94 @@
   +    $this->assertEmpty([], $field->options['id']);
   

   Should this not be: $this->assertEmpty($field->options['id']);?

2. +++ b/core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php
   @@ -0,0 +1,94 @@
   +    $display_manager = $this->getMockBuilder('\Drupal\views\Plugin\ViewsPluginManager')
   ...
   +    $display = $this->getMockBuilder('Drupal\views\Plugin\views\display\DefaultDisplay')
   

   Can we change the class definition to "ViewsPluginManager::class" and "DefaultDisplay::class".

The point #104.4 is still open.

Updates:

• Addressed #105
• Reverted unrelated test changes
• Updated core/modules/media_library/tests/src/Unit/MediaLibrarySelectFormTest.php and core/modules/views/tests/src/Unit/Plugin/views/field/BulkFormTest.php tests. MediaLibrarySelectFormTest was updated in #101 but I think we were not testing what we should.

Fixed test fails in #106 in MediaLibrarySelectFormTest.

@mbovan I've tried your patch on 8.9.1 but it's failing. Should it work on this version as well?

And more general: does this patch solve the error:
"TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\EntityOperations::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /Applications/MAMP/htdocs/drupaluss/web/core/modules/views/src/Plugin/views/field/EntityOperations.php on line 142 in Drupal\views\Plugin\views\field\EntityOperations->getEntityTranslation() (line 69 of core/modules/views/src/Entity/Render/EntityTranslationRenderTrait.php). "

Edit: I found out I didn't apply the patch correctly. Big thanks to @Corn696 who (accidentally) gave an answer to my issue, about adding an extra section of your composer.json (https://www.drupal.org/project/social/issues/3093583)

Tested with 9.0.5 and it seemed to strip all values out of the view. I'm seeing the rows but they're empty.

Tested with 8.9.6 and it seems to work well

I was getting a WSOD when deleting Group posts. This appears to be resolved but like #110 I'm seeing blank rows in my content admin view.

@MrPaulDriver check this out: #3029355: Deleting a node deletes the linked group_content entities which then resaves the node

All my points are addressed.
The patch is green for 9.2.
The patch looks good to me.
For me it is RTBC.

I'm not convinced that this patch is the correct direction. I'm not sure that soldiering on when expectations are not met is the correct behaviour.

1. +++ b/core/modules/comment/src/Plugin/views/field/LinkApprove.php
   @@ -19,7 +19,8 @@ class LinkApprove extends LinkBase {
   -    return Url::fromRoute('comment.approve', ['comment' => $this->getEntity($row)->id()]);
   +    $entity = $this->getEntity($row);
   +    return $entity ? Url::fromRoute('comment.approve', ['comment' => $entity->id()]) : NULL;
   
   +++ b/core/modules/comment/src/Plugin/views/field/LinkReply.php
   @@ -21,12 +21,13 @@ class LinkReply extends LinkBase {
   -    return Url::fromRoute('comment.reply', [
   -      'entity_type' => $comment->getCommentedEntityTypeId(),
   -      'entity' => $comment->getCommentedEntityId(),
   -      'field_name' => $comment->getFieldName(),
   -      'pid' => $comment->id(),
   -    ]);
   +    return $comment ?
   +      Url::fromRoute('comment.reply', [
   +        'entity_type' => $comment->getCommentedEntityTypeId(),
   +        'entity' => $comment->getCommentedEntityId(),
   +        'field_name' => $comment->getFieldName(),
   +        'pid' => $comment->id(),
   +      ]) : NULL;
   

   I think this solution is incorrect. We're now breaking the interface of getUrlInfo() - this can't return NULL. It has to return a Url object.

   I think here we need to trigger an exception - our expectations are not met. Also is it really true that getEntity() can return NULL for these plugins. I'm not convinced.

2. +++ b/core/modules/contact/src/Plugin/views/field/ContactLink.php
   @@ -30,7 +30,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -    return Url::fromRoute('entity.user.contact_form', ['user' => $this->getEntity($row)->id()]);
   +    $entity = $this->getEntity($row);
   +    return $entity ? Url::fromRoute('entity.user.contact_form', ['user' => $entity->id()]) : NULL;
   

   As above... this is moving the problem.

3. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -189,7 +193,7 @@ protected function checkUrlAccess(ResultRow $row) {
   -   * @return \Drupal\Core\Url
   +   * @return \Drupal\Core\Url|null
   

   I think this change is not correct.

I think it is worth considering going the entirely opposite direction with the change.

i.e changing \Drupal\views\Plugin\views\field\FieldPluginBase::getEntity() to something like.

  public function getEntity(ResultRow $values, $require_entity = FALSE) {
    if (!$require_entity) {
      @trigger_error('This will be required in the future', E_USER_DEPRECATED);
    }
    $relationship_id = $this->options['relationship'];
    if ($relationship_id == 'none') {
      $entity = $values->_entity;
    }
    elseif (isset($values->_relationship_entities[$relationship_id])) {
      $entity = $values->_relationship_entities[$relationship_id];
    }
    if ($require_entity && !$entity instanceof EntityInterface) {
      throw new \RuntimeException('@todo');
    }
  }

For me the big question is why/how are we calling these plugins when there is no entity. How do we get a row without an entity?

> For me the big question is why/how are we calling these plugins when there is no entity. How do we get a row without an entity?

In my experience for one of three reasons:

a) Broken rows in the entity tables. There are some cases when a query can return a node ID but the node load for it is failing. For example if there is no default revision aka record in {node_revision}. I've seen this happen with migrate or so doing weird things. But we've added some safe guards for this a while ago. At least this is very hard to recover from when it does happen as you need to manually fix rows in your database.
b) Invalid cache invalidation, which shouldn't happen on its own but could result in the views data cache returning rows and then the load for that failing.
c) optional relationships that don't exist for a row. I might mix something up with another issue, but that should also be a possibility. So at least for that case, not being able to return an entity is an expected situation I think and not something we should. throw an exception for?

For a), maybe the sql query plugin could drop rows that fail to load an entity.

#117.c is a good point. What then feels odd is that we try and render the value in this case at all. Surely if the relationship required by the plugin is optional and missing then we should short circuit all the processing an render an empty field... like a missing entity exception could help us do that. We could catch it in advancedRender()... but doing this would require extensive re-architecture and probably better test coverage of views than we have.

So the solution in #108 is probably the most pragmatic. Going to re-instate the rtbc from #115 and sleep on it.

Agree with #117, in my case it was caused by bad cache invalidation, I added steps to reproduce on this issue marked as duplicate

https://www.drupal.org/project/drupal/issues/3105003#comment-13849916

Yeah but #120 is exactly what gives me pause. Your site has ended up in a situation it shouldn't have and this patch is masking the error. This patch will cause an incorrect version of the view to be cached because instead of triggering an error we've returned empty values and the view has been rendered. This can have serious consequences - imagine you have set up the view to add different information if the result is empty - for example - you have a list of limited stock items and if some column is empty you display that product is available. But the only reason it's empty is some cache error like in #120.

Re-uploading patch 3007424-108.patch so it gets re-tested every 2 days against 9.2.x-dev.

Currently is re-tested against 9.1.x-dev.

Are we able to distinguish between #3007424-117: Multiple usages of FieldPluginBase::getEntity do not check for NULL, leading to WSOD (a)/(b) and (c), and if so log something in the case of (a)/(b)?

Logging that should happen when "the node load for it is failing".

Logging when actually trying to load nodes would be a big change.

For example if I visit https://www.drupal.org/node/2 that's a 404. The only way Drupal knows it's a 404 is to try to load the node to see if it's there or not. Logging would potentially add a lot of noise.

If we don't log in entity load, then the next place up is the various callers, of which Views would be one.

Agreed. Not every failed load is a problem. But in views, it very much is because the entities it loads are based on a query that it did just run and expects those ID's to exist. So it's views that needs to do the logging.

When I was testing this (see comment #120) I was running a behat test that deletes the generated users at the end, while I was constantly refreshing the admin/people page. The results were correct, but at some point it breaks.
I think the deletion of users is not correctly clearing the cache tags used by the view.

I've added a complimentary issue and patch for Views Bulk Operations: https://www.drupal.org/project/views_bulk_operations/issues/3195468

Patch #108 worked for me on Drupal 8.9.13. Had the issue with /admin/commerce/products view. Not sure what caused it however as it was working fine until today. No changes were made to the site.

Been using patch #108 for some time now, what is left before this can be RTBC?

Browsing through this "Blast from the Past" it looks like @catch puts this back to NR to get more logging in #123 since this patch might probably only masking the root cause and was answered by @geek-merlin in #124 and finally @Berdir pointed out in #126 that this logging should take place in the views module.

So, in my tiny mind, this issue should be on Needs Work for that logging in views. If I misread anything, please correct me.

Adding related issue in the #117(c) category: #2916597: TypeError when result row doesn't have an entity

Marked #2916597: TypeError when result row doesn't have an entity as a duplicate

I tried patch on #122 to solve...

TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\EntityOperations::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given,

... on admin/content and I've found a regression (see regression_patch_122.png). Content field values doesn't render.

Notice I landed in current issue via #3105003: TypeError: Argument 1 passed to Drupal\views\Plugin\views\field\EntityOperations::getEntityTranslation() must implement interface Drupal\Core\Entity\EntityInterface, null given,

#2939442: Views 'Entity Operations' field crashes if on a non-required relationship made similar changes to the EntityOperations field so this is a reroll for 9.5.x

Conflicts with #2457999: Cannot use relationship for rendered entity on Views. Just posting to save others time as that has been committed to 10.1.

Closed #2715483: [regression] getEntity() doesn't always return entity, which results in add comment field not working in Views as a duplicate

I can confirm patch on #144 fixed the issue for me on Drupal 9.4.x

Rebased #146 onto an MR against 10.1.x. Setting to NR for core checks as they aren't running locally for me for some reason. Will try to address the last remaining piece of feedback (logging) after.

So in terms of logging, I'm not entirely sure where this is best placed. Putting it in FieldPluginBase::getEntity would make sense but it would have to be a very generic message since we don't have context of what the entity id or type is there.

I think the only place we can really add it is where _entity is set which looks like is in Sql::assignEntitiesToResult. That would mean adding a new dependency on the logger service and that would mean we'd need a deprecation dance for any classes extending that plugin.

Before I go down that route, it would be good to get confirmation from @catch and/or @Berdir that this sounds like the right approach.

FieldPluginBase::getEntity has the following available

• $this->configuration['entity_type'] for the entity-type
• $this->configuration['entity field'] for the field name
• $values->index for the row index
• $this->view->id() for the view machine name

so in theory we could use that information to construct a reasonably informative log message

Back to NW for logging

Logging added. Almost all tests needed updating to mock the logger factory so I've added a trait for that. Also removed the tearDowns which were unsetting the container as that happens in UnitTestCase::setUp anyway.

Almost certain there will be changes needed to the wording but this gets us into that convo :)

tested the MR !3529 , worked fell, all senerio covered, moving to RTBC done

@Lendude chance you could give this a +1 from a Views subsystem maintainer POV?

The test look great, and as I see it we are handling all the needed cases. The only thing I don't really like is that how the NULLs are handled, it feels very inconsistent case-by-case. Sometime we have a guard statement with an early return, sometimes we have a big tertiary operator on the return statement, sometimes we just let the method run to the end and trust that to give a NULL and maybe some other variations.

My personal preference is just to have a guard statement as early as possible and return whatever we need in case of a NULL, but like I said, that is personal pref.

Not setting too 'Needs work' because this is not something that is inherently wrong, just personal opinion/feeling. The code does what it needs to do, so from that perspective it is RTBC.

As this is a bug, can we also get a 9.5.x version? Thanks - it doesn't apply cleanly to 9.5.x.

Rolled for 9.5.x

Moving back to RTBC.

Compared patch #164 has all the changes in MR 3529 and they are all there except the change to phpstan-baseline.neon, (because it's not in 9.5)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Moving back to RTBC. Think the bot tried to apply the 9.5.x branch to 10.1.x

New comments on the MR from @mstrelan

Moving to NW for additional comments in MR.

I'll try and address this today

All comments addressed.

The failing tests are related to the Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest class, specifically to the testJsWebAssert method. The error message suggests that there was a problem with the WebDriver, specifically with a stale element reference. This could be caused by the web page changing before the test could complete, or by an error in the test script itself.

To correct this issue, the code for the JSWebAssertTest class needs to be reviewed and corrected. It is likely that the testJsWebAssert method needs to be updated to account for changes in the web page during testing. Additionally, the WebDriver configuration may need to be updated to ensure that it is properly interacting with the web page

These ChatGPT answers are not helpful.

How is it relevant to this issue? Restoring status.

There still appear to be some open threads in the MR. Know you probably can't resolve them but could you leave a comment on them saying if they've been addressed or not needed.

Weird, I thought I had applied all suggestions in the last push but I seem to have missed 5 of them...all applied now.

Have reviewed the MR and all feedback has been addressed with tests passing. API additions mentioned in the Issue Summary don't need a Change Record. This is good to go.

Updating issue credits

Committed to 10.1.x

Although there is a minor API change here (returning null instead of a Url) at present that code just blows up with a WSOD so I don't think this is disruptive (its a WSOD out of your hands now, it would be something you can do something about with this patch). So on that basis I think this is worth considering for 10.0.x and 9.5.x but I will get a second opinion.

Setting as patch to be ported in the meantime.

This is causing test fails in TMGMT :-/

I have a custom entity label field plugin there: \Drupal\tmgmt\Plugin\views\field\EntityLabel, this displays the label of an entity, and it's on an optional relationship (messages that might or might not have a reference to a job item).

Not only does this now log errors in a situation that is not unexpected, the logging also blindly asumes that there is an entity field in the configuration, which I think isn't necessary. It might be due to very old default views configuration but still, it shouldn't cause php warnings on this:

Warning: Undefined array key "entity field"
Drupal\views\Plugin\views\field\FieldPluginBase->getEntity()() (Line: 438)

I can work around it by checking if I have an ID, but http://grep.xnddx.ru/search?text=$this-%3EgetEntity($values) finds a bunch of similar use cases. I think what this should do is check for optional relationships and not log an error then.

Reverted in light of #182 - let's see if we can make the logging conditional if the relationship is optional

Taking a look

Was a bit tricky to figure out how to reproduce the failure but I was able to by:

1. Checking out this branch on core
2. Cloning tmgmt module
3. Reverting https://git.drupalcode.org/project/tmgmt/-/commit/aab4b531f7cd60bbb5e25e...
4. Running phpunit app/modules/contrib/tmgmt/translators/tmgmt_local/tests/src/Functional/LocalTranslatorTest.php --filter testBasicWorkflow
5. Which failed with Warning: Undefined array key "entity field" Drupal\views\Plugin\views\field\FieldPluginBase->getEntity()() (Line: 438)

I then applied a fix (latest commit on the MR) and ran the tests again and they passed.

It seems like we don't have any tests in core that have an optional relationship with an entity that doesn't load, otherwise that would've triggered the same error? If anyone has pointers on where to put a test like that, that'd be great.

Test pushed. It's pretty gross but does the job. Using a mock logger to save having to install dblog and query for logs.

The change looks good I think, didn't check the test. One part that's not covered yet is handling the case if there's no entity field configuration key, I think that's a valid situation.

Added a guard for #188

Last commit seems to cover #188.

Question is it possible on TMGMT to run tests with this as a patch? Not sure if that's possible in drupalci

Left some questions on the MR

Remarking for committer review

core/modules/views/src/Plugin/views/field/FieldPluginBase.php

The thread for this do we want to open a follow up?

The thread in the MR has some suggestions now

Ok, hopefully this is the one 🤞

Marking this for @acbramley. Checked the threads have been resolved and recommended change for $this->label() ?: $this->realField was added.

This applies and works.

I am placing this note here. It conflicts with #2457999: Cannot use relationship for rendered entity on Views on Drupal 9.5, which has been added to Drupal 10.1.

Unfortunately, this now needs a reroll, clash in phpstan-baseline.neon

Fine to self RTBC on resolution

Green after merge, back to RTBC

Committed to 10.1.x, moving to 10.0.x for backport

Thanks all 🎉

Here's a reroll for 10.0.x

Reroll looks good.

Committed to 10.0.x, moving to 9.5.x for reroll for that

Got hit by this issue on a 9.5.x Project I am working on .
Back ported Latest 10.x patch to 9.5.x

Seems to have CI errors.