Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#66 | interdiff-2791571-65-66.txt | 4.17 KB | chr.fritsch |
#66 | 2791571-66.patch | 10.01 KB | chr.fritsch |
#65 | 2791571-65.patch | 5.38 KB | chr.fritsch |
#49 | interdiff-2791571-46-49.txt | 1.08 KB | chr.fritsch |
#49 | 2791571-49.patch | 10.03 KB | chr.fritsch |
Comments
Comment #2
dawehnerLet's play the devil's advocate. Could we not use
$entity->toUrl()->getRouteParameters()
?Comment #4
kristiaanvandeneynde@dawehner (#2): That would also work in this case. Maybe we should postpone this until the other issue is resolved, though.
Comment #6
chr.fritschFor 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.
Comment #7
chr.fritschCheckin that entity is already saved
Comment #10
chr.fritschFix some more tests
Comment #11
chr.fritschThinking about introducing a ContentEntityViewBuilder and move the new code there...
Comment #12
phenaproximaTerrific 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.
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:
Maybe this should be marked deprecated?
Comment #13
phenaproximaOn 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.
Comment #14
seanBAlso had some time to look at this.
Don't we need
toUrl('revision')
for non-default revisions?Nit, the array should be on a new line.
Comment #15
chr.fritschStill 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.Comment #17
seanBThis should probably check if the entity type is revisionable.
Comment #18
chr.fritschFixed the patch
Regarding #17: It's already checked internally in
Entity::urlRouteParameters()
Comment #19
seanBI 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
inEntity::toUrl()
. Maybe a check for the revision link template is a good idea?$entity->hasLinkTemplate('revision')
Comment #21
tstoecklerI really like this a lot. I agree on the needed tests, but otherwise this looks pretty close. Some comments, but nothing major:
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."
Not sure the
isRevisionable()
check is needed. I think it is implied thatisDefaultRevision()
will always return TRUE for non-revisionable entities.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.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 thehasLinkTemplate()
check) afterwards. Not sure, though.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 we will get revision contextual links for free.
Comment #22
chr.fritschThanks for reviewing @seanB and @tstoeckler
I think a implemented all the suggestions in #19 and #21
Comment #23
seanBReally tried to find something. Some small super nits only.
I think the nesting of if statements in the method makes it a little hard to read. Can we do something like this:
Maybe just remove this empty line.
Comment #24
chr.fritschFixed the nits
Comment #25
kristiaanvandeneyndeJust 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!
Comment #26
seanBLooks good to me. Thanks!
Comment #27
tstoecklerSorry 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.
Comment #28
bdimaggio@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.
Comment #29
phenaproxima@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!
Comment #30
chr.fritschI 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.
Comment #31
phenaproximaLet's skip entity_test for now, then, and add it if the committers ask for it.
Comment #32
tstoecklerOh, that's absolutely right. So adding generic test coverage is blocked on a generic entity template á la #2808481: Introduce generic entity template with standard preprocess and theme suggestions.
Comment #33
chr.fritschOk, here are tests for Terms and BlockContent. Media will follow in #2775131: Media entities should support contextual links
Comment #34
phenaproxima@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?
Comment #35
chr.fritschBecause 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.
Comment #36
phenaproximaOkay, that's fair enough. I think we might be all set here -- will review the patch shortly.
Comment #37
phenaproximaMan, this looks so very good. I found nothing serious -- mostly just nitpicks and slight hardening of the (beautiful) tests.
Nit: Can the instanceof check be wrapped in parentheses --
!($entity instanceof ContentEntityInterface)
?Should be {@inheritdoc}.
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"]')
Should be {@inheritdoc}.
Can this also assert an element, rather than response text?
Should be {@inheritdoc}.
Let's change the assertion here too.
Comment #38
chr.fritschFixed all the nits
Comment #39
chr.fritschComment #40
phenaproximaLooks great! RTBC once Drupal CI passes it.
Comment #41
Wim LeersNice! This helps Media, but also Quick Edit and Outside-In.
Extraneous parentheses.
👍
👍
👍
But we should
@deprecate
this class and remove it in 9. Because it is empty now.Comment #42
phenaproximaThey'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. :)
Comment #43
chr.fritschHere is the change record for marking TermViewBuilder as deprecated: https://www.drupal.org/node/2924233
Comment #44
chr.fritschTermViewBuilder 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.
Comment #45
kristiaanvandeneyndeNot 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.
Comment #46
chr.fritschI am also not 100% convinced about marking the TermViewBuilder deprecated, because of the point @kristiaanvandeneynde mentioned.
Let's receive more feedback from committers.
Comment #47
xjmThanks for your work on this!
$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 thelink
tag but that's not a perfect match. Let's use a more descriptive variable name.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.Comment #48
tstoecklerRe #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.Comment #49
chr.fritschOkay, I adjusted the deprecation message.
Comment #50
kristiaanvandeneyndeLooks good given the above comments and the way they were answered or dealt with.
Comment #52
kristiaanvandeneyndeTestbot hiccup
Comment #54
kristiaanvandeneyndePretty sure it's the testbot again. Re-queuing patch for testing.
Comment #55
Wim LeersThis 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!Comment #56
BerdirNot 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...
Comment #57
Wim LeersGood point, I don't seen an immediate reason why we could not update
EntityViewBuilder
instead of addingContentEntityViewBuilder
.Also noticed the IS is very out of date.
Comment #58
BerdirWell, 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.
Comment #59
phenaproximaIf 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.
Comment #60
kristiaanvandeneynde+1 to phenaproxima's comment in #59
But it's too late for that now so I see two options:
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.
Comment #61
chr.fritschComment #62
phenaproximaI like this idea. Let's fix the bad naming as swiftly as we can.
Comment #63
chr.fritschI agree with @kristiaanvandeneynde and @phenaproxima. Let's open a separate issue for that
Comment #64
phenaproximaOkay, tagging for a follow-up.
Comment #65
chr.fritschDiscussed 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.
Comment #66
chr.fritsch#65 proves that the new solution would also work with existing alterBuild implementations.
So now lets remove the old implementations.
Comment #67
BerdirWe 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.
Comment #68
kristiaanvandeneyndeI'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?
Comment #69
chr.fritsch@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.
Comment #70
BerdirThere 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 :)
Comment #71
kristiaanvandeneyndeOkay 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.
Comment #72
kristiaanvandeneynde@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.
Comment #73
chr.fritschRegarding #72: TBH I don't see the benefit of a showsContextualLinks() method. If you don't want contextual links, just overwrite addContextualLinks().
Comment #74
kristiaanvandeneyndeGood point
Comment #75
chr.fritschHere is the follow-up #2928552: Introduce a ContentEntityViewBuilder and move content entity specific logic from EntityViewBuilder into it
Comment #76
chr.fritschComment #77
chr.fritschAlso added a second CR: https://www.drupal.org/node/2928555
Comment #78
kristiaanvandeneyndeFollowed/starred the follow-up. CR also looks nice. Thanks @chr.fritsch!
Comment #79
BerdirAgreed, 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.
Comment #80
chr.fritschThanks @Berdir, i added that.
Comment #81
alexpottWe'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.
Comment #82
alexpottCommitted 723712c and pushed to 8.5.x. Thanks!
Comment #85
bgilhome CreditAttribution: bgilhome commentedPatch for backport to 8.4.x-dev at https://www.drupal.org/project/drupal/issues/2948838.