Problem/Motivation

The EntityViewBuilder::getBuildDefaults has an $entity parameter that should respect the EntityInterface.

Yet in the getBuildDefaults() method there's a call on the $entity object to a method that's not on the EntityInterface:

$entity->isDefaultRevision();

isDefaultRevision() lives on the ContentEntityBase but EntityViewBuilder might be used by a ConfigEntityBase.

This may result in an error Error: Call to undefined method Drupal\my_module\Entity\MyConfigEntity::isDefaultRevision() in EntityViewBuilder.php on line 181

Proposed resolution

We should check if the entity is an instance of RevisionableInterface first:

$entity instanceof RevisionableInterface
CommentFileSizeAuthor
#15 2951487_15_no-tests.patch915 bytesJaesin
#12 2951487_12.patch1.45 KBducktape
#2 2951487_2.patch925 bytesmpp

Issue fork drupal-2951487

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mpp created an issue. See original summary.

mpp’s picture

mpp commented
Assigned: mpp » Unassigned
Status: Active » Needs review
StatusFileSize
new 2951487_2.patch925 bytes

Added a check for the RevisionableInterface and also fixed some coding standards.

filsterjisah’s picture

filsterjisah commented

I wonder why the defaults should only set the cache keys when rendering a revisional entity? Since there is no revision id being set.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We already have other issues, the fact is that EntityViewBuilder really is ContentEntityViewBuilder, nothing in there really works for config entities and needs to be completely overridden anyway, see BlockViewBuilder as an example in core.

mpp’s picture

mpp commented

@filsterjisah perhaps this makes more sense:

    if (
      $this->isViewModeCacheable($view_mode)
      && !$entity->isNew()
      && (
        !$entity instanceof RevisionableInterface
        || $entity->isDefaultRevision()
      )
      && $this->entityType->isRenderCacheable()
    ) {

@berdir, so we need to refactor everything to ContentEntityViewBuilder and change the arguments interface to ContentEntityInterface?
Ideally we'd have a static code analysis tool which checks if interfaces are respected but it would seem useful to have this patch to check for the interface anyhow?

There are similar cases in that class:
$entity instanceof TranslatableInterface
$entity instanceof FieldableEntityInterface

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

We already have other issues, the fact is that EntityViewBuilder really is ContentEntityViewBuilder, nothing in there really works for config entities and needs to be completely overridden anyway, see BlockViewBuilder as an example in core.

I'm not sure if that makes this issue irrelevant (closed duplicate) or not. I think this is a good improvement for the current situation.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'm also not sure. I guess in the example here, it worked because buildMultiple() was then overridden but not view, unlike BlockViewBuilder?

I'd avoid refactoring everything, at least here. Maybe we should just check the interface in the default buildMultiple() implementation and throw a clear exception that the default implementation only supports fieldable/content entities? Together with this fix you can then at least get the default placeholder/render-caching logic.

I'm not a fan of multi-line if conditions, especially not if we start to further nest it. What if we add a $this->isEntityCacheable($entity) and move the isNew and revision check in there? Then it should become much more readable and we can probably keep it in a single line? (we could in theory even move all existing conditions into that method, then we can do early returns or something like that to keep it readable?

That would also allow to customize the logic, e.g. if someone wants to cache non-default revisions?

mpp’s picture

mpp commented

@berdir, I agree that adding $this->isEntityCacheable($entity) with early returns will be an improvement.

While my suggestion won't break anything else, I'm not 100% sure that adding an exception won't break any existing config entity that extends EntityViewBuilder.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'm OK with not adding any exceptions here. Looking at \Drupal\Core\Entity\EntityViewBuilder::buildMultiple(), I guess it will simply do nothing now for config entities as it will never put them into $view_modes, so it will just silently do nothing.

mpp’s picture

mpp commented

ok, thanks.

Fyi, we ran into this issue as we had a custom config entity (a Map) extending EntityViewBuilder that we're viewing on a Node as an entity reference field.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Needs work for adding a wrapper method to make it more readable. Also maybe this needs a test, but I guess it won't be very useful as it won't do much except no longer failing.

ducktape’s picture

ducktape commented
Status: Needs work » Needs review
StatusFileSize
new 2951487_12.patch1.45 KB

Added a wrapper method with the checks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks.

So one remaining question is #3. Why not always cache non-revisionable entities? Basically turn around the conditions, check for isNew() first, return FALSE and then check for either not revisionable or being the default revision?

We also need tests. Unsure how that would work exactly, possibly with a custom view builder on a config entity that overrides buildMultiple() and does something simple in there like returning a property?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jaesin’s picture

Jaesin commented
StatusFileSize
new 2951487_15_no-tests.patch915 bytes

As far as I can see the view builder is just trying not to cache builds for non default revisions.

luismagr’s picture

luismagr commented

This patch works fine for me. I had the same problem. Thanks for the work!

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Can't this be fixed before 8.7.0 is out? If not, is there a chance to get this merged before 8.8.0? How can I help to speed things up?

It is a bug and in my opinion, its fix is a small change, it should not break anything.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue tags: +not every entity is content or config

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mykola dolynskyi’s picture

mykola dolynskyi
Primary language Ukrainian
Location Poltava
commented

Still facing this in Drupal 9.5.2 in context of https://www.drupal.org/project/dashboards/issues/3294986

kalpanajaiswal made their first commit to this issue’s fork.

kalpanajaiswal opened merge request !8299

kalpanajaiswal’s picture

kalpanajaiswal commented

Created a MR for Drupal 11.
MR: https://git.drupalcode.org/project/drupal/-/merge_requests/8299
Branch: 2951487-check-instance-type

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Fails on code style check

prabha1997’s picture

prabha1997 commented
Assigned: Unassigned » prabha1997

Working on code style check

prabha1997’s picture

prabha1997 commented

Updated MR please check
https://git.drupalcode.org/project/drupal/-/merge_requests/8299

kalpanajaiswal’s picture

kalpanajaiswal commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Was previously tagged for tests. Did not review

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.