Problem/Motivation

Currently, a lot of content entities that want to support contextual links have a custom EntityViewBuilder that looks like this:

/**
 * View builder handler for taxonomy terms.
 */
class TermViewBuilder extends EntityViewBuilder {

  /**
   * {@inheritdoc}
   */
  protected function alterBuild(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
    parent::alterBuild($build, $entity, $display, $view_mode);
    $build['#contextual_links']['taxonomy_term'] = array(
      'route_parameters' => array('taxonomy_term' => $entity->id()),
      'metadata' => array('changed' => $entity->getChangedTime()),
    );
  }

}

Why don't we remove these sort-of-duplicate classes and instead support contextual links out of the box in EntityViewBuilder?

Proposed resolution

Introduce contextual links support for entities in EntityViewBuilder. Then BlockContent, Node and Term can use this new functionality. Also Media can benefit from it in #2775131: Media entities should support contextual links

Remaining tasks

  • Implement
  • Review
  • Commit
CommentFileSizeAuthor
#66 interdiff-2791571-65-66.txt4.17 KBchr.fritsch
#66 2791571-66.patch10.01 KBchr.fritsch
#65 2791571-65.patch5.38 KBchr.fritsch
#49 interdiff-2791571-46-49.txt1.08 KBchr.fritsch
#49 2791571-49.patch10.03 KBchr.fritsch
#46 interdiff-2791571-44-46.txt1.18 KBchr.fritsch
#46 2791571-46.patch10.02 KBchr.fritsch
#44 interdiff-2791571-38-44.txt1.46 KBchr.fritsch
#44 2791571-44.patch10.02 KBchr.fritsch
#38 interdiff-2791571-32-38.txt3.39 KBchr.fritsch
#38 2791571-38.patch8.86 KBchr.fritsch
#33 interdiff-2791571-24-32.txt1.99 KBchr.fritsch
#33 2791571-32.patch8.82 KBchr.fritsch
#24 interdiff-2791571-22-24.txt2.31 KBchr.fritsch
#24 2791571-24.patch6.45 KBchr.fritsch
#22 interdiff-2791571-18-22.txt2.79 KBchr.fritsch
#22 2791571-22.patch6.47 KBchr.fritsch
#18 interdiff-2791571-15-18.txt998 byteschr.fritsch
#18 2791571-18.patch5.41 KBchr.fritsch
#15 interdiff-2791571-10-15.txt4.02 KBchr.fritsch
#15 2791571-15.patch5.35 KBchr.fritsch
#10 interdiff-2791571-7-10.txt1.2 KBchr.fritsch
#10 2791571-10.patch4.89 KBchr.fritsch
#7 interdiff-2791571-6-7.txt1.6 KBchr.fritsch
#7 2791571-7.patch4.85 KBchr.fritsch
#6 2791571-6.patch4.79 KBchr.fritsch
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

kristiaanvandeneynde created an issue. See original summary.

dawehner’s picture

Let's play the devil's advocate. Could we not use $entity->toUrl()->getRouteParameters()?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Status: Active » Postponed

@dawehner (#2): That would also work in this case. Maybe we should postpone this until the other issue is resolved, though.

Version: 8.4.x-dev » 8.5.x-dev

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

chr.fritsch’s picture

Status: Postponed » Needs review
FileSize
4.79 KB

For media we also want to have contextual links in #2775131: Media entities should support contextual links. Solving this would make it bit easier.

So i moved the code from EntityViewBuilder in contribs entity module into core EntityViewBuilder and generalized it with the steps from the issue summary.

chr.fritsch’s picture

Title: [PP-1] Automatically supply contextual links for content entities » Automatically supply contextual links for content entities
FileSize
4.85 KB
1.6 KB

Checkin that entity is already saved

The last submitted patch, 6: 2791571-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 7: 2791571-7.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
4.89 KB
1.2 KB

Fix some more tests

chr.fritsch’s picture

Thinking about introducing a ContentEntityViewBuilder and move the new code there...

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Terrific work! What a nice enhancement this will be. Not sure if this needs additional tests, but I'm tagging it that way anyway. If there is already test coverage for contextual links of nodes, blocks, and taxonomy terms, then maybe all this needs is a few screenshots before it can go RTBC.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -337,7 +337,22 @@ public function buildComponents(array &$build, array $entities, array $displays,
    +      if (($entity->isDefaultRevision()) || !$entity->getEntityType()->isRevisionable()) {
    +        $key = $entity->getEntityTypeId();
    +      }
    +      else {
    +        $key = $entity->getEntityTypeId() . '_revision';
    +      }
    

    Two things. One, we don't need the extra set of parentheses around $entity->isDefaultRevision(). Two, it seems like this could be a little clearer if we did it something like this:

    $key = $entity->getEntityTypeId();
    if ($entity->isDefaultRevision() || $entity->getEntityType()->isRevisionable()) {
      $key .= '_revision';
    }
    
  2. +++ b/core/modules/taxonomy/src/TermViewBuilder.php
    @@ -2,24 +2,9 @@
    +class TermViewBuilder extends EntityViewBuilder {}
    

    Maybe this should be marked deprecated?

phenaproxima’s picture

On second thought: yes, let us add moar tests. We can enable one of the multitude of test entity types that ship with System and see if contextual links exist for it. And we can also add test coverage for contextual links on nodes, blocks, and terms, if we don't have any already.

seanB’s picture

Also had some time to look at this.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -337,7 +337,22 @@ public function buildComponents(array &$build, array $entities, array $displays,
    +        'route_parameters' => $entity->toUrl()->getRouteParameters(),
    

    Don't we need toUrl('revision') for non-default revisions?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -337,7 +337,22 @@ public function buildComponents(array &$build, array $entities, array $displays,
    +        $build['#contextual_links'][$key]['metadata'] = ['changed' => $entity->getChangedTime()];
    

    Nit, the array should be on a new line.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
4.02 KB

Still needs test, just introduced a ContentEntityViewBuilder and fixed the stuff from #14.

I'm not sure about marking TermViewBuilder, maybe it's still useful for future functionalities.

Status: Needs review » Needs work

The last submitted patch, 15: 2791571-15.patch, failed testing. View results

seanB’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
@@ -0,0 +1,37 @@
+      else {
...
+          'route_parameters' => $entity->toUrl('revision')->getRouteParameters(),

This should probably check if the entity type is revisionable.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
998 bytes

Fixed the patch

Regarding #17: It's already checked internally in Entity::urlRouteParameters()

seanB’s picture

I agree checking is the entity type is revisionable is probably not needed. Not sure if revisionable entity types must implement a revision link template though? I remember media not having it for a while, but I guess contrib could have cases as well.

It could lead to a UndefinedLinkTemplateException in Entity::toUrl(). Maybe a check for the revision link template is a good idea?
$entity->hasLinkTemplate('revision')

Status: Needs review » Needs work

The last submitted patch, 18: 2791571-18.patch, failed testing. View results

tstoeckler’s picture

I really like this a lot. I agree on the needed tests, but otherwise this looks pretty close. Some comments, but nothing major:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    + * Base class for content entity view builders.
    

    I think this reads like this is an abstract base class, which it is not. Also, IIRC class documentation should also start with a verb. I would suggest "Provides a view builder for content entities."

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +      if ($entity->isDefaultRevision() || !$entity->getEntityType()->isRevisionable()) {
    

    Not sure the isRevisionable() check is needed. I think it is implied that isDefaultRevision() will always return TRUE for non-revisionable entities.

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +          'route_parameters' => $entity->toUrl()->getRouteParameters(),
    

    Nice trick using the URL's route parameters. Sweet!

    However, I do think we should check $entity->hasLinkTemplate('canonical') before trying to generate the URL, just to be sure. Generally this will only be called on the canonical route, but let's be sure.

    Also, but this is more a personal preference, than an actual rule, I always explicitly call toUrl('canonical') instead of falling back to the default, because it makes the code more seld-documenting in my opinion.

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +          'route_parameters' => $entity->toUrl('revision')->getRouteParameters(),
    

    Again, I think we should check whether the 'revision' link template exists.

    Having written this, it might even be more readable to simply do $rel = ... in the if-else and then have a common code-path (including the hasLinkTemplate() check) afterwards. Not sure, though.

  5. +++ b/core/modules/taxonomy/src/TermViewBuilder.php
    @@ -2,24 +2,9 @@
    -  protected function alterBuild(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
    -    parent::alterBuild($build, $entity, $display, $view_mode);
    -    $build['#contextual_links']['taxonomy_term'] = [
    -      'route_parameters' => ['taxonomy_term' => $entity->id()],
    -      'metadata' => ['changed' => $entity->getChangedTime()],
    -    ];
    -  }
    

    Awesome that we can remove all this code. The term one is especially nice, as - even though we are removing code here - with #2880149: Convert taxonomy terms to be revisionable and publishable we will get revision contextual links for free.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
6.47 KB
2.79 KB

Thanks for reviewing @seanB and @tstoeckler

I think a implemented all the suggestions in #19 and #21

seanB’s picture

Status: Needs review » Needs work

Really tried to find something. Some small super nits only.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,36 @@
    +  protected function alterBuild(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
    

    I think the nesting of if statements in the method makes it a little hard to read. Can we do something like this:

    if ($entity->isNew() || !$entity instanceof ContentEntityInterface) {
      return;
    }
    
    $key = $entity->getEntityTypeId();
    $rel = 'canonical';
    if (!$entity->isDefaultRevision()) {
      $key .= '_revision';
      $rel = 'revision';
    }
    
    if (!$entity->hasLinkTemplate($rel)) {
      return;
    }
    
    $build['#contextual_links'][$key] = [
      'route_parameters' => $entity->toUrl($rel)->getRouteParameters(),
    ];
    if ($entity instanceof EntityChangedInterface) {
      $build['#contextual_links'][$key]['metadata'] = [
        'changed' => $entity->getChangedTime(),
      ];
    }
    
  2. +++ b/core/modules/node/tests/src/Functional/NodeContextualLinksTest.php
    @@ -0,0 +1,46 @@
    +  public function testNodeContextualLinks() {
    +
    

    Maybe just remove this empty line.

chr.fritsch’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.45 KB
2.31 KB

Fixed the nits

kristiaanvandeneynde’s picture

Just chiming in to say thanks for working on this. I agree with dawehner's solution in #2 now and as explained in #2651974-36: field_ui_entity_operation() cannot respect route parameters because of incorrectly named routes, the real issue with that approach in the other issue is not daniel's solution but rather how Field UI defines its routes. I will try to fix that problem in the related issue instead.

Good work!

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

Sorry to put a bit of a stop on this, but #24 removes the "Needs tests" tag, but #13 was asking for a more expansive set of tests. I don't have a strong opinion on adding tests for custom blocks or taxonomy terms, as that's kind of "their fault" to not have existing tests, but at least an entity_test entity type should be tested in my opinion.

bdimaggio’s picture

@tstoeckler @phenaproxima @chr.fritsch I'm actually planning to write those tests today. I can certainly do the entity_test you suggest, and would like to add the term and custom block ones unless someone thinks that would step on toes/be outside of the scope of this issue.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

@bdimaggio, we'll never say no to more tests! Kicking back to NW and re-tagging for that. :)

Since the tests all follow a similar structure, it shouldn't be too hard to test contextual links on:

1) Nodes (done)
2) Terms
3) Block content
4) Media items
5) One of the entity types in entity_test -- we'll want to test any of the revisionable content entities that it defines

If we have all of that, I think it'd be a pretty bulletproof suite of tests!

chr.fritsch’s picture

I am currently working on the Terms and BlockContent tests.

One thing about the entity_test: I have no idea how to render title_suffix for an entity_test. Because title_suffix contains the contextual links.

phenaproxima’s picture

One thing about the entity_test: I have no idea how to render title_suffix for an entity_test. Because title_suffix contains the contextual links.

Let's skip entity_test for now, then, and add it if the committers ask for it.

tstoeckler’s picture

Oh, that's absolutely right. So adding generic test coverage is blocked on a generic entity template á la #2808481: [PP-1] Introduce generic entity template with standard preprocess and theme suggestions.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
1.99 KB

Ok, here are tests for Terms and BlockContent. Media will follow in #2775131: Media entities should support contextual links

phenaproxima’s picture

@chr.fritsch: Since this issue makes contextual links generic for content entities, why not simply add test coverage for media items here, and close #2775131: Media entities should support contextual links?

chr.fritsch’s picture

Because we have to change the media.html.twig in classy and stable as well. And i think it's out of scope for this issue.

phenaproxima’s picture

Okay, that's fair enough. I think we might be all set here -- will review the patch shortly.

phenaproxima’s picture

Status: Needs review » Needs work

Man, this looks so very good. I found nothing serious -- mostly just nitpicks and slight hardening of the (beautiful) tests.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +    if ($entity->isNew() || !$entity instanceof ContentEntityInterface) {
    

    Nit: Can the instanceof check be wrapped in parentheses -- !($entity instanceof ContentEntityInterface)?

  2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentContextualLinksTest.php
    @@ -0,0 +1,39 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    Should be {@inheritdoc}.

  3. +++ b/core/modules/block_content/tests/src/Functional/BlockContentContextualLinksTest.php
    @@ -0,0 +1,39 @@
    +    $this->assertSession()->responseContains('data-contextual-id="block:block=' . $block->id() . ':langcode=en|block_content:block_content=' . $block_content->id() . ':');
    

    Nit: It's probably marginally better to assert an element, rather than random text in the response (which could be part of an HTML comment, or something else that is not actually functional). To wit, can we do this:

    $this->assertSession()->elementExists('css', '[data-contextual-id="all that good stuff here"]')

  4. +++ b/core/modules/node/tests/src/Functional/NodeContextualLinksTest.php
    @@ -0,0 +1,44 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    +    'contextual',
    +  ];
    

    Should be {@inheritdoc}.

  5. +++ b/core/modules/node/tests/src/Functional/NodeContextualLinksTest.php
    @@ -0,0 +1,44 @@
    +    $this->assertSession()->responseContains('data-contextual-id="node:node=' . $node->id() . ':');
    

    Can this also assert an element, rather than response text?

  6. +++ b/core/modules/taxonomy/tests/src/Functional/TermContextualLinksTest.php
    @@ -0,0 +1,38 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    +    'contextual',
    +  ];
    

    Should be {@inheritdoc}.

  7. +++ b/core/modules/taxonomy/tests/src/Functional/TermContextualLinksTest.php
    @@ -0,0 +1,38 @@
    +    $this->assertSession()->responseContains('data-contextual-id="taxonomy_term:taxonomy_term=' . $term->id() . ':');
    

    Let's change the assertion here too.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
8.86 KB
3.39 KB

Fixed all the nits

chr.fritsch’s picture

Issue tags: +Media Initiative
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks great! RTBC once Drupal CI passes it.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Nice! This helps Media, but also Quick Edit and Outside-In.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +    if ($entity->isNew() || !($entity instanceof ContentEntityInterface)) {
    

    Extraneous parentheses.

  2. +++ b/core/modules/block_content/src/BlockContentViewBuilder.php
    @@ -2,14 +2,13 @@
    -class BlockContentViewBuilder extends EntityViewBuilder {
    +class BlockContentViewBuilder extends ContentEntityViewBuilder {
    

    👍

  3. +++ b/core/modules/node/src/NodeViewBuilder.php
    @@ -2,15 +2,14 @@
    -class NodeViewBuilder extends EntityViewBuilder {
    +class NodeViewBuilder extends ContentEntityViewBuilder {
    

    👍

  4. +++ b/core/modules/taxonomy/src/TermViewBuilder.php
    @@ -2,24 +2,9 @@
    -class TermViewBuilder extends EntityViewBuilder {
    ...
    +class TermViewBuilder extends ContentEntityViewBuilder {}
    

    👍

    But we should @deprecate this class and remove it in 9. Because it is empty now.

  5. Does this mean Quick Edit starts working for more entity types?
phenaproxima’s picture

Extraneous parentheses.

They're not extraneous -- the point is to keep it obvious that we're negating the result of the expression $entity instanceof ContentEntityInterface, rather than negating $entity first, then running the instanceof check. Obviously it will work without the parentheses, since that's how PHP works...but it decreases the clarity and intent of the code. I won't fight hard on this, but I think we should keep them.

I agree, though, about the deprecation. :)

chr.fritsch’s picture

Here is the change record for marking TermViewBuilder as deprecated: https://www.drupal.org/node/2924233

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
10.02 KB
1.46 KB

TermViewBuilder is now deprecated.

Regarding #41.5: Currently the new ContentEntityViewBuilder is only used by Terms, Nodes and Blocks which already had this support. So i would say no, but now it's much easier for content entities to get this support.

kristiaanvandeneynde’s picture

Status: Needs review » Needs work

Not a fan of deprecating TermViewBuilder. If we decide to add something else there, then we need to "un-deprecate" the class. View builders are used for more than just contextual links, you know :)

Also minor nitpick but you have three tests with a testNodeContextualLinks method even though only one of them actually covers nodes.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
10.02 KB
1.18 KB

I am also not 100% convinced about marking the TermViewBuilder deprecated, because of the point @kristiaanvandeneynde mentioned.

Let's receive more feedback from committers.

xjm’s picture

Status: Needs review » Needs work

Thanks for your work on this!

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
    @@ -0,0 +1,37 @@
    +    $rel = 'canonical';
    ...
    +      $rel = 'revision';
    

    $rel is not a super great variable name. In general, we don't use abbreviations in variable names in core. I guess it's by analogy with the link tag but that's not a perfect match. Let's use a more descriptive variable name.

  2. +++ b/core/modules/taxonomy/src/TermViewBuilder.php
    @@ -2,24 +2,17 @@
    +@trigger_error(__NAMESPACE__ . '\TermViewBuilder is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\ContentEntityViewBuilder instead. See https://www.drupal.org/node/2924233.', E_USER_DEPRECATED);
    ...
    + *
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.0.
    + *   Use \Drupal\Core\Entity\ContentEntityViewBuilder instead.
    + *
    + * @see \Drupal\Core\Entity\ContentEntityViewBuilder
    + * @see https://www.drupal.org/node/2924233
      */
    -class TermViewBuilder extends EntityViewBuilder {
    -
    -  /**
    -   * {@inheritdoc}
    -   */
    -  protected function alterBuild(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
    -    parent::alterBuild($build, $entity, $display, $view_mode);
    -    $build['#contextual_links']['taxonomy_term'] = [
    -      'route_parameters' => ['taxonomy_term' => $entity->id()],
    -      'metadata' => ['changed' => $entity->getChangedTime()],
    -    ];
    -  }
    -
    -}
    +class TermViewBuilder extends ContentEntityViewBuilder {}
    

    I think deprecating something and then un-deprecating it later if we have cause to override the base class is fine. The deprecation will ensure we get rid of the empty implementation before D9 if we don't have a need to override it.

    Just a note that the trigger error has a comma splice (grammatical error in English). We can fix this by replacing the comma with the word "and". Edit: we can also make the @deprecated match it exactly.

tstoeckler’s picture

Re #47.1: While your general is point is of course correct, in this particular case we do use "rel" as an abbreviation all over core. In particular, the method which we pass this variable to documents it as such, as well. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

So in this case, the abbreviation actually makes things clearer because of the identity of the local variable name and the method variable name. Not to mention all the various other places $rel is used in core in this context.. Therefore, I would vote to keep it as is.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
1.08 KB

Okay, I adjusted the deprecation message.

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

Looks good given the above comments and the way they were answered or dealt with.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: 2791571-49.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Reviewed & tested by the community

Testbot hiccup

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: 2791571-49.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Reviewed & tested by the community

Pretty sure it's the testbot again. Re-queuing patch for testing.

Wim Leers’s picture

This issue is to <EntityType>ViewBuilder as #2282029: Automate link relationship headers for all entity types, not just nodes is to <EntityType>ViewController. Together, they make entity rendering much more consistent!

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityViewBuilder.php
@@ -0,0 +1,37 @@
+/**
+ * Provides a view builder for content entities.
+ */
+class ContentEntityViewBuilder extends EntityViewBuilder {

Not 100% sure about the naming of the subclass here.

1. The thing is that EntityViewBuilder already *is* content entity specific, calling it on a config entity is going to result in a fatal error. They can't be viewed without a completely custom implement like e.g. BlockViewBuilder.

2. Assuming config entities could be viewed then contextual links should also work for those, this is conceptually not really content entity specific?

Having a new subclass has the advantage that it is something that entity types can opt-in to, if they define this already then it would proably be weird if we'd somehow define this stuff twice. So it does make sense to have one I guess, although I'm not a huge fan of adding more and more base classes because it gets hard to pick the right now and lots of documentation/tutorials would need to be updated.

Just thinking out loud...

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Good point, I don't seen an immediate reason why we could not update EntityViewBuilder instead of adding ContentEntityViewBuilder.

Also noticed the IS is very out of date.

Berdir’s picture

Well, the reason we can't just add it to the base class is that then all entity types would get it automatically, which could be a problem if things are duplicated or should not be there for a reason.

What we've done before is add some kind of entity type flag to opt-in for that new behavior and then plan on making that the default in Drupal 9. Not sure if that's worth it here or if we should just do it and mention it in a change record.

phenaproxima’s picture

If EntityViewBuilder is content entity-specific, then it was poorly named to begin with. I would prefer that we kept ContentEntityViewBuilder in place -- it's far more clear about what it actually does.

kristiaanvandeneynde’s picture

+1 to phenaproxima's comment in #59

But it's too late for that now so I see two options:

  1. Add a flag like Berdir suggested in #58 and remove/change it in D9
  2. Still go ahead with a subclass and merge the two in D9

Either way, modules will have to opt in to this new behavior.

P.s.: Nothing stops us from renaming EntityViewBuilder to ContentEntityViewBuilder right now and introducing a deprecated class called EntityViewBuilder that extends ContentEntityViewBuilder without adding to it.

chr.fritsch’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
phenaproxima’s picture

Nothing stops us from renaming EntityViewBuilder to ContentEntityViewBuilder right now and introducing a deprecated class called EntityViewBuilder that extends ContentEntityViewBuilder without adding to it.

I like this idea. Let's fix the bad naming as swiftly as we can.

chr.fritsch’s picture

I agree with @kristiaanvandeneynde and @phenaproxima. Let's open a separate issue for that

phenaproxima’s picture

Issue tags: +Needs followup

Okay, tagging for a follow-up.

chr.fritsch’s picture

Discussed this with @Berdir on IRC.

First thing: We would like to keep the clean-up of EntityViewBuilder, because it has content entity specific stuff, out of the scope of this issue.

Second thing: He mentioned again, that contextual links are not really content entity specific.

So we came to the conclusion to add the contextual link support to the EntityViewBuilder and introduce a new protected method there which will be called right before alterBuild.

PS: No interdiff, because of a new approach.

chr.fritsch’s picture

#65 proves that the new solution would also work with existing alterBuild implementations.

So now lets remove the old implementations.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -324,6 +325,36 @@ public function buildComponents(array &$build, array $entities, array $displays,
+    if ($entity instanceof ContentEntityInterface && !$entity->isDefaultRevision()) {
+      $rel = 'revision';
+      $key .= '_revision';
+    }

We should check that the entity type has a revision link template/relationship, might not exist and then toUrl() will throw an exception.

I can't read.

kristiaanvandeneynde’s picture

Second thing: He mentioned again, that contextual links are not really content entity specific.

So we came to the conclusion to add the contextual link support to the EntityViewBuilder and introduce a new protected method there which will be called right before alterBuild.

I'm confused. So they're not specific to content entities, but we are adding a protected method to add them to the view builder for content entities. Doesn't the above statement indicate that we need to add that logic to a common place that both content and config entities can use? Or am I misreading the quoted text?

chr.fritsch’s picture

@kristiaanvandeneynde I think EntityViewBuilder is used currently by content and config entities. For example, Tour uses is as well. But there is content entity specific stuff in EntityViewBuilder. For example ::getSingleFieldDisplay() expects to get a ContentEntityBase.

Berdir’s picture

There is no dedicated view builder for config entities. EntityViewBuilder has lots of content entity specific code but there is no alternative for it, BlockViewBuilder and TourViewBuilder are examples, but BlockViewBuilder overrides basically every method in the parent and TourViewBuilder just overwrote viewMultiple and doesn't care about anyone calling any of the other methods which would then fail for it.

My point is we should do this in a way that avoids that problem by adding it to the existing code, then we can discuss in another issue what we should do about EntityViewBuilder that's mostly content entity specific.
Patch looks good to me. The only thing we need to decide now is whether we need a flag so that entity types can opt in/opt out of this (they could override the method if they don't want this). I'd like to avoid adding even more special keys to the entity type annotation if we can avoid it.

We should also have a change record that explains that we do this now including a snippet for how it can be overridden if someone really doesn't want it. And i guess we can open an issue to deprecate \Drupal\entity\EntityViewBuilder :)

kristiaanvandeneynde’s picture

Okay gotcha, the wording had me confused a bit.

If that's the case, then the follow-up should not mark EVB as deprecated as suggested in #60. The right approach would be to split off the content entity specific stuff into a ContentEVB, which would mean a backwards compatibility break and thus should only be done in 9.0.0. It's still possible to create a follow-up for that, though.

kristiaanvandeneynde’s picture

@Berdir in #70: How about we introduce a boolean returning method showsContextualLinks() that always returns TRUE? If people don't want them, they can extend the view builder and only need to override that method. It would also remove the need for yet another annotation key in the entity type definition.

chr.fritsch’s picture

Regarding #72: TBH I don't see the benefit of a showsContextualLinks() method. If you don't want contextual links, just overwrite addContextualLinks().

kristiaanvandeneynde’s picture

Good point

chr.fritsch’s picture

chr.fritsch’s picture

Title: Automatically supply contextual links for content entities » Automatically supply contextual links for entities
Issue summary: View changes
Related issues: +#2928552: Introduce a ContentEntityViewBuilder and move content entity specific logic from EntityViewBuilder into it
chr.fritsch’s picture

Also added a second CR: https://www.drupal.org/node/2928555

kristiaanvandeneynde’s picture

Followed/starred the follow-up. CR also looks nice. Thanks @chr.fritsch!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, maybe mention in the CR that it supports contextual links for the default revision as well as revision specific contextual links if the entity type defines the canonical/revision link templates?

Back to RTBC, then.

chr.fritsch’s picture

Thanks @Berdir, i added that.

alexpott’s picture

Issue tags: -Needs followup

We've got sign off from the entity maintainer (@Berdir), change records, good test coverage and the followup has been created. Ticking the issue credit box for all the reviews that have moved this issue forward.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 723712c and pushed to 8.5.x. Thanks!

  • alexpott committed 723712c on 8.5.x
    Issue #2791571 by chr.fritsch, kristiaanvandeneynde, phenaproxima,...

Status: Fixed » Closed (fixed)

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