Problem/Motivation

Getting the follow error while working on a new test in #2614720: Fatal errors while loading/building orphaned comments 

1) Drupal\Tests\comment\Functional\CommentOrphanedTest::testNoOwner
TypeError: Argument 1 passed to Drupal\Core\Entity\EntityViewBuilder::resetCache() must be of the type array or null, object given, called in /opt/sites/d8/core/tests/Drupal/Tests/EntityViewTrait.php on line 64

Line 64 is the resetCache in this block:

    if ($reset) {
      $render_controller->resetCache([$entity->id()]);
    }

which is passing an array of strings to resetCache which expects the first parameter to be an array of entities.

The failure will happen when buildEntityView(EntityInterface $entity, $view_mode = 'full', $langcode = NULL, $reset = FALSE) is called with reset set to TRUE. There are currently no instances of that in core but the new test in #2614720: Fatal errors while loading/building orphaned comments does set reset to TRUE.

Steps to reproduce

Proposed resolution

Change \Drupal\Tests\EntityViewTrait::buildEntityView from 

    if ($reset) {
      $render_controller->resetCache([$entity->id()]);
    }

to 

    if ($reset) {
      $render_controller->resetCache([$entity]);
    }

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3163924-13.patch1.4 KBquietone
#11 3163924-11.patch2.66 KBquietone
#11 interdiff-8-11.txt2.29 KBquietone
#8 3163924-8.patch1.74 KBquietone
#8 3163924-8-fail.patch1.07 KBquietone
#5 3163924-5.patch1.74 KBquietone
#5 3163924-5-fail.patch1.06 KBquietone
#3 3163924-3.patch694 bytesquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Title: buildEntityView call resetCache with an array of ids » buildEntityView calls resetCache with an array of ids not entities
Component: php.module » phpunit
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
3163924-3.patch694 bytes

A simple patch. I'd write a test for this but don't know where to start.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

I think a kernel test that loads an entity using this trait, and renders it with the reset flag set should be enough for testing sake

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3163924-5-fail.patch1.06 KB
3163924-5.patch1.74 KB

Thx, I woke up the next day with the same thought.

The last submitted patch, 5: 3163924-5-fail.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 3163924-5.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
3163924-8-fail.patch1.07 KB
3163924-8.patch1.74 KB

Wrong namespace.

The last submitted patch, 8: 3163924-8-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/tests/Drupal/Tests/EntityViewTrait.php
@@ -61,7 +61,7 @@ protected function buildEntityView(EntityInterface $entity, $view_mode = 'full',
     $render_controller = $this->container->get('entity_type.manager')->getViewBuilder($entity->getEntityTypeId());
     if ($reset) {
-      $render_controller->resetCache([$entity->id()]);
+      $render_controller->resetCache([$entity]);
     }

To be honest, seems pretty overkill to even offer this option. Clearly this wasn't ever used before and probably just added to have parity with other somewhat similar methods like \Drupal\Tests\node\Traits\NodeCreationTrait::getNodeByTitle() but the use case here is very different. That other one is mostly about the static cache and mismatch between web requests and the test runner and #3040878: Reset static entity cache on POST requests in tests would cleanly remove the need for that.

The render cache however is 99% transparent. saving the entity invalidates that, even across the web requests/test runner. The only case when you need this is when you do shady things like manually alter database tables. And it's not enough, you need to reset the storage cache as well.

I know it's kinda out of scope, but I do wonder if we shouldn't just deprecate this argument and remove it in D10. Trigger a deprecation mesage if you pass in TRUE.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
interdiff-8-11.txt2.29 KB
3163924-11.patch2.66 KB

Thanks. It does seem odd to fix this when it isn't used anywhere. I support deprecation.

I am not sure if this needs to be done in two issues or if it can all be done here. In any case, I started a patch with the deprecation. There isn't a change record yet.

So fix this and then a new issue or all in one?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

All in one sounds good to me

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
3163924-13.patch1.4 KB

From Slack discussion

berdir @larowlan @quietone thinking more about #3163924: buildEntityView calls resetCache with an array of ids not entities, do we even need to deprecate it considering that it's an immediate fatal error and nobody could have possibly been using it? :slightly_smiling_face:
larowlan yeah I was thinking the same - but because its in the function signature… is their a BC issue
larowlan I don’t think so, because you can’t extend traits
berdir even on a base clase, we could remove an optional extra argument at the end, child classes could still have it, only on an interface it would be an issue
larowlan ya
larowlan +1 to remove dead code now - agree it would die hard if you did use it
quietone I take that to remove the fourth parameter and related code. Right?
berdir yeah, I think it's fine to just remove it

So, new patch that removes the fourth parameter etc.

Participants:

larowlan, Berdir, quietone

edit: fix table formatting

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Yeah, lets do this :)

Feels like entity system would be a better component even though the code is technically in the generic test traits namespace, but doesn't matter too much.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3163924-13.patch, failed testing. View results

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Reviewed & tested by the community

Just that ckeditor random fail.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed fecfd28 and pushed to 9.1.x. Thanks!

Thanks folks

  • larowlan committed fecfd28 on 9.1.x 
    Issue #3163924 by quietone, Berdir: buildEntityView calls resetCache...

Status: Fixed » Closed (fixed)

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