Updated: Comment #0
Problem/Motivation
Edit module adds needed attributes in edit_entity_view_alter()
but default view builder does not provide a wrapper element to attach this attributes so JS throws and exception and other behaviors are not run.
Proposed resolution
Add default #theme_wrappers
within edit module or default wrapper in EntityViewBuilder
so contrib entities will not require to implement entity view builder to allow edit module work
Comment | File | Size | Author |
---|---|---|---|
#14 | 2186653-14_edit_wrapper.patch | 616 bytes | alvar0hurtad0 |
edit_wrapper.patch | 566 bytes | andypost | |
Comments
Comment #1
sunInteresting.
If I get you right, then Edit breaks if the entity does not provide a theme template that actually consumes and outputs the HTML attributes?
Only by coincidence, most/all entities in core have a theme template that outputs the attributes.
→ Do we need a default template for entities?
Comment #2
andypostYes, you get me right. I've just tested media_entity module and got this problem because there's no element to print entity attributes by default
Comment #3
Wim LeersHah, wow, that's interesting indeed!
Should this be moved to the Entity API component?
Comment #4
andypostMaybe better to implement this wrapper some other way (in entity view builder? then all core entities should override this
Comment #5
andypostComment #6
sunFixing issue title.
In an ideal world, we'd do this:
entity.html.twig
entity--node.html.twig
entity--user.html.twig
entity--whatever.html.twig
The lack of the first, i.e. a generic default template, is the root cause for this issue.
Comment #7
andypostComment #8
andypostComment #9
fagoyes, I'd agree we should have a generic default template just as entity module started doing it in d7 already.
I've not followed progress on the theme/twig side of things much though. I guess doing so would require us to fix #939462: Specific preprocess functions for theme hook suggestions are not invoked for template_preprocess_node() still being invoked.
I agree the ideal world would be
entity.html.twig
entity--node.html.twig
entity--user.html.twig
entity--whatever.html.twig
- but that's probably too much changes right now. Else, I'd suggest doing the same as entity.module in d7:
entity.html.twig
node.html.twig
node--type.html.twig
...
as this keeps the known template names. Related issue we had with entity.module: #1462772: template suggestions are not working if no custom template is defined
Comment #10
xjmComment #11
sunquickedit.module is just the symptom, but not the root cause. Can't decide whether this falls under the umbrella of the entity system or the theme system. Basically both. Entity system is covered by an issue tag already, so let's go for theme system. :)
Closely related: #2270883: Automatically add theme hook suggestions for all entity types
Comment #12
andypostAnother one affected #2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found."
Comment #13
jhedstromAs per #11 I think this needs work (original patch just fixes the symptom). Also needs a reroll since the module is now called quickedit.module.
Comment #14
alvar0hurtad0Reroll of #11 as #13 says
Comment #15
alvar0hurtad0Ups, the status.
Comment #16
manningpete CreditAttribution: manningpete commentedPatch applies, no reroll needed.
Comment #18
joelpittetIs there an example in core that we can test?
Comment #19
andypostYes, all
entity_test
entities have no templates so nice examples for testingComment #20
joelpittetMaybe we could get that entity_test entity on a screen and check for the data attributes/classes that need to be available for quick edit?
Comment #21
Wim LeersPer #11.
Comment #22
joelpittetIt's a symptom but localized to quickedit. I figure without a concrete example this will never be fixed. Adding more wrappers is not a solution in most cases as we have a tone of 'divitis' issues opened by morten and others to slim down the cruft.
If #14 fixes it for quickedit I'd like the quickedit subsystem maintainers to yay/nay this proposed patch.
Comment #23
Wim LeersI'm confused. The patches posted so far fix the symptom, not the cause.
It only affects custom entity types with essentially broken templates. (Quick Edit is not the only thing that wants to add attributes to rendered entities.) Therefore committing this fix will result in things being magically fixed as long as Quick Edit is installed, and mysteriously broken when it's not.
Custom entities just need to include sane templates for now, until the root cause of the problem is addressed in the entity system?
If this work-around is ever accepted, it belongs in
system.module
, notquickedit.module
. I think it's not acceptable inquickedit.module
for the provided reasons.Did that make sense?
Comment #24
joelpittetPanels and IPE has similar issues, from what I recall they only add markup when they need to hang their gear and drag and drop features onto. It seems like a decent solution. Of course that can cause some layout differences with some accidental CSS. I wonder if they have a new plan for D8 for IPE?
What bits outside of quickedit need this as well? We had similar concerns around RDF because it needs to hang attributes around certain bits of content. RDF has worked around this inside of preprocesses when the module is turned on from what I recall but quickedit doesn't need to add these extra divs(I hope) when permissions aren't relevant.
Comment #25
Wim LeersRDF is a clear example, but there could easily be other modules that want to add attributes (
data-
attributes or otherwise). I don't know of any concrete examples off the top of my head though.Is there a reason we can't implement @fago's proposal in #9?
Comment #26
joelpittetWe seem to run into problems when there is expected markup. I know from D7 a few times I've removed the seemingly harmless
$title_suffix
because I didn't think it was needed in my layout, then wondered where the contextual links disappeared to.re #9 it's an interesting thought, not sure how it would solve the problem. Considering it's a bit tricky to enforce markup. IDK maybe it's reasonable to expect a base of "all entity templates must print attributes on a block element or quick edit won't work"?
Comment #27
Wim LeersGiven #26, I pinged @fago.
Comment #28
fagoyep, that sounds reasonable to me.
#9 sounds great, but I'm not sure it addresses this problem? Still, the default template could be changed to not render the necessary wrapper?
Comment #29
andypostI'd better go with #2023571: Support preprocessing in EntityViewBuilder instead of #2270883: Automatically add theme hook suggestions for all entity types
Comment #30
andypostMore radical but reasonable change is to implement
entity.html.twig
and inherit all entity templates from it.But why we add default view builder if entity does not supposed to be viewed?
Also injection of "theme specifics" into entity definition looks ugly
so I'd better set required presence of
view_builder
key in entity annotation to point that entity "renderable"... otherwise we end-up with similar issues like #2698909: EntityViewBuilder uses non-existing #theme hooksComment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhy is this a "Contributed project blocker"? This only breaks functionality for entity types that are doing something wrong, and those entity types can fix the problem themselves. If there aren't any core entity types that are failing to provide the needed #theme, how is this blocking contrib? And if this isn't a contrib blocker, then I'm not sure it qualifies as a Major priority.
Yes,
entity_test
is an example that is doing something wrong, by unsetting#theme
inEntityTestViewBuilder::getBuildDefaults()
and not providing a replacement. And we may want to fix that in this issue. But I don't think this bug in a test entity type qualifies as a contrib blocker.Yes, I think this is a reasonable requirement. We probably should document it somewhere though. Not sure where, but some options might be
EntityViewBuilder::getBuildDefaults()
,EntityViewBuilderInterface::view()
, orentity.api.php
in the "View/render" section. If we choose to provide anentity.html.twig
default per #9, that could also be a great place for this documentation. There's two parts to this requirement:#theme
(which by default is provided inEntityViewBuilder::getBuildDefaults()
) or#theme_wrappers
is needed.{{ attributes }}
.Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSetting to Needs Work, because some problems with this approach are:
#theme
that outputs attributes (e.g., all the non-test ones in core).Comment #35
xjmComment #36
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNo responses to #32, so removing the "Contributed project blocker" tag, and downgrading to Normal priority.
Comment #37
lauriii@Cottser, @joelpittet, @xjm, @alexpott, and I discussed this a while back and we still think this is a major bug. We need some way for contributed modules to know that they are doing something "wrong". At the moment the system is based on assumptions that are not well documented, and working around them requires prior knowledge. We will need to decide whether to fix this in the entity system, theme system, quickedit, or somewhere else.
Comment #38
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI believe the fix belongs in the entity system. Either a docs-only patch per #32, and/or the inclusion of an entity.html.twig base theme hook / template per #9. While docs can be backported to 8.2, I think adding an 'entity' base theme hook and making all entity types inherit from it should be an 8.3 only change, and may need to wait on #2559825: Preprocess functions from base hooks not triggered for theme hooks not using the __ pattern.
Comment #46
andypost