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
Once #1503464: Automatically add theme hook suggestion for node view mode is in, would be great to have a generic mechanism that adds the view mode based suggestions to all entity types, not only for nodes.
Proposed resolution
Create a generic system that generates theme hook suggestions for entities based on entity type, bundle and view mode.
Remaining tasks
Create the patch.
Testing.
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#58 | automatically_add_theme-2270883-58.patch | 12.94 KB | joelpittet |
Comments
Comment #1
plopescHello
Attaching first approach for this issue, creating a functions that includes theme_suggestions based on the entity view modes in case we are theming an entity with view modes.
There could be another better approach. Any feedback is welcomed here.
Thank you!
Comment #2
sunAdding an extremely closely related issue.
Comment #4
plopesc1: entity_suggestions_view_mode-2270883-1.patch queued for re-testing.
Comment #5
Manuel Garcia CreditAttribution: Manuel Garcia commentedThe patch applies cleanly, however upon trying to do a fresh install with this patch applied I see this:
I run
rm -rf files/* && rm settings.php && cp default.settings.php settings.php && chmod 777 settings.php
before attempting the installation, just in case its me that causing the problem...I realy dont see why the patch would trigger this error, however if i undo the patch changes the installer shows up fine. Strange...
Comment #6
plopescHello Manuel
I found the bug, it was my mistake because getDefinition() threw an exception because I didn't add the correct parameter.
New patch including this change:
Changed to
getDefinition($entity_type_id, FALSE)
to avoid exceptions.And changing file structure after #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4
Regards.
Comment #7
plopescPatch in #6 was only the re-roll of #1
Here is the patch including the bug fixed.
Comment #9
Schnitzel CreditAttribution: Schnitzel commentedI really like that, but not sure if
theme_get_entity_suggestions()
should be in theme.inc, as it is specific to entities and not drupal in general. Maybe this should be in entity Module? And probably there with entity_theme_suggestions_alter() ?Comment #10
plopescAddressing comments in #9
Regards
Comment #11
andypostRelated #2301245-13: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."
Comment #15
lauriiiI moved
hook_theme_suggestions_alter()
to system module since there's no entity-module anymore.Comment #16
andypost+1
Comment #18
joelpittetYes yes!
Gave this a test:
Still good:
And moar.
Comment #19
star-szrCouple questions, leaving at RTBC though. They are pretty minor.
1. Shouldn't this be \Drupal?
2. Would checking
isset($variables['elements']['#view_mode'])
before doing any of the other checks make more sense from a performance perspective?Comment #20
joelpittet#re 20.2
I do think it may have a bit of a performance improvement but not a whole lot I'd hazard a guess. The first check is a boolean/truthy/falsey check which IIRC would be fastest, but the second function call would be slower than the isset() so it kind of depends on how often view_mode is not set. Wouldn't hurt to put it first though.
You could also store local variables for $entity->getEntityTypeId() and $entity->bundle(), etc to prevent multiple method calls, though I don't know how much we gain from those changes.
#re 20.1 Yes, it should be, nice catch @Cottser.
That gets me thinking though. Before we had a view_mode with no check. Now some entities may not have one.
Maybe that check for #view_mode should be around the view_mode specific suggestions? Otherwise entity's without view_modes won't get bundle suggestions. What do you think?
Comment #21
star-szrI just want us to keep in mind that every theme/template (that is not render cached) is going to be going through this function. So there is potentially a big impact.
Comment #22
joelpittetWas talking out loud, not meant to dissuade :), I agree, let's do all the things. Good call, thank you for sober second thought.
Comment #23
plopescHello
New version that I think improves performance a bit.
Firstly checks if
$variables['elements']['#view_mode']
is set, given that this property is set by EntityViewBuilder for Content Entities and that's what we are looking for here.Then it checks the other variables and uses local variables for entity_type, bundle and entity_id.
I know this hook approach can hit the performance, other option could be do something more "special" that could convert something inserted in EntityViewBuilder automatically into theme suggestions.
Thank you for your reviews.
Comment #24
joelpittet@plopesc thanks that should be better:)
Wondering your thoughts re #20, if the view mode doesn't exist to still have the other bundle/id suggestions?
Comment #25
plopescHello
All content entity types provide at least "default" view mode, so there is no possibility to get one without
['#view_mode']
.On the other hand, config entities don't allow bundles,
bundle()
method returns theentityTypeId
.So, I think we are OK now...
New patch adding support for custom blocks, given that they are embedded into regular blocks and needs specific support.
Anyway, maybe would be a good idea to get more feedback from the entity team.
Regards
Comment #26
joelpittetThanks, yeah good call @plopesc maybe @Berdir or @catch could have a quick look, they my have some suggestions on these suggestions:)
Comment #27
BerdirWe had exactly the same discussion (how to properly identify an entity template) in #2023571: Support preprocessing in EntityViewBuilder. There this was considered as too less specific.
You should probably at least check if #$hook is set as well as you use it unconditionally later on.
I think it would be a good idea to add #entity_type to buildDefaults() in the entity view builder, that would make it a more reliable to identify the entity type. Because it is not mandatory to be the same, just the default.
If you look at the other issue, the approach there was to redirect the call back to the view builder. Doing that here would allow to a) avod duplicting the logic for block_content and give entities an easy way to change those suggestions. so we would add getTemplateSuggestions(EntityInterface $entity) there and use it here.
All you would do then here I think is check #view_mode and #entity_type, and when provided, check if $entity_manager->hasViewBuilder(). we might actually want to pass in $variables instead of $entity, so that we don't have to hardcode #$hook?
Comment #28
joelpittetThank you very much for the feedback. We can take a stab at your recommendations.
Comment #29
plopescHello
Here is a new approach for this patch following some of the directions provided by Berdir (Thank you for your time).
This uses a specific implementation for blocks, given that its architecture is different.
Let' see if that approach is better than previous :)
Regards
Comment #30
Manuel Garcia CreditAttribution: Manuel Garcia commentedThis looks good to me, here's what I did to test:
I'd set it to RTBC, but I don't understand the patch well enough for that call =)
Comment #31
Manuel Garcia CreditAttribution: Manuel Garcia commentedIf it helps, without the patch applied, these are the template suggestions for that same page (taxonomy/term/1)
Comment #32
star-szrExtra blank line can be removed ;)
I don't think this is preprocessing variables - summary line should be updated.
Other than that looks good overall to me! Having this on the interface seems quite sensible.
Comment #33
plopescThank you for your review @manuel and @Cottser
Here is a new patch addressing comments in #32
Also includes a new test file.
Regards
Comment #34
SteffenR@plopesc: The comment looks more descriptive to me - the suggestions are working fine - tested it for node entities and term entities.
What do you think cottser?
Comment #35
BerdirNot all entity types have bundles. User for example doesn't, so we would get template suggestions for user__user and so on.
I would suggest you add a check for $entity->getEntityType()->getKey('bundle') to see if the entity type supports different bundles and then only add those if it makes sense.
We have view modes with "." ?
I guess this makes this an API change that needs to be documented and accepted to still be possible after beta...
Comment #36
joelpittetSetting as needs work as per @Berdir's notes in #35
Comment #37
lauriii#35.2: isn't it possible to add view_mode with "."?
Comment #39
BerdirI don't think that is valid (anymore?), view modes are part of the entity display entity ID, that can not possibly contain ".".
Comment #42
plopescIn the past, it was possible, but since #2170289: Entity Display modes form allows to create machine names including dots is in, it should not be possible.
It comes from #1503464: Automatically add theme hook suggestion for node view mode
Comment #45
star-szrTagging for reroll.
Comment #46
Manjit.Singhre-rolling #37
Comment #47
Manjit.SinghComment #51
SteffenRI'll reroll the patch against latest core version.
Comment #52
SteffenRComment #53
SteffenRAttached rerolled patch against latest core.
Comment #56
SteffenRLooks like the array structure of some parts changed - i'll fix it.
Comment #57
SteffenRI'm not sure why the tests are breaking here - maybe some one else can help out fixing the issue.
Comment #58
joelpittetre-rolling based on #37 there was only one failure in the patch which was
EntityViewBuilderInterface.php additions. So I added those in. Let's see if we're failing still.
Bumping to 8.1.x as it's a feature request.
Comment #60
joelpittetSo one error is that not all blocks have a 'block_content' key
block.module:183:
Comment #61
BerdirSeems like this should live in block_content.module, not here? Possibly just remove the check and always do the else here?
Adding a new method to an interface is an API change. To be backwards compatible, we need to add a new interface for this..
Comment #63
andypostSuppose before making that better to fix #2023571: Support preprocessing in EntityViewBuilder
Comment #64
andypostany reason to inject theme implementation details into entity layer?
Comment #66
BerdirNew approach that has a bigger chance of getting in I hope: #2808481: Introduce generic entity template with standard preprocess and theme suggestions
Comment #68
jibranIs this a duplicate of #2808481: Introduce generic entity template with standard preprocess and theme suggestions or postpone on that?
Comment #81
andypostDuplicate of #2808481: Introduce generic entity template with standard preprocess and theme suggestions