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

Files: 
CommentFileSizeAuthor
#14 2186653-14_edit_wrapper.patch616 bytesalvar0hurtad0
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,470 pass(es). View
edit_wrapper.patch566 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 63,680 pass(es). View

Comments

sun’s picture

Interesting.

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?

andypost’s picture

Yes, 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

Wim Leers’s picture

Hah, wow, that's interesting indeed!

Should this be moved to the Entity API component?

andypost’s picture

Issue tags: +Entity Field API

Maybe better to implement this wrapper some other way (in entity view builder? then all core entities should override this

andypost’s picture

sun’s picture

Title: Edit module require wrapper in entity view builder » HTML attributes set for a wrapping container element are not output for all entities (breaks Edit module)

Fixing 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.

andypost’s picture

Issue tags: +Twig
andypost’s picture

fago’s picture

yes, 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

xjm’s picture

Title: HTML attributes set for a wrapping container element are not output for all entities (breaks Edit module) » HTML attributes set for a wrapping container element are not output for all entities (breaks Quick Edit module)
Component: edit.module » quickedit.module
sun’s picture

Title: HTML attributes set for a wrapping container element are not output for all entities (breaks Quick Edit module) » HTML attributes for wrapping container element are not output for all entities (breaks Quick Edit module)
Component: quickedit.module » theme system

quickedit.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

andypost’s picture

jhedstrom’s picture

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

As 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.

alvar0hurtad0’s picture

FileSize
616 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,470 pass(es). View

Reroll of #11 as #13 says

alvar0hurtad0’s picture

Status: Needs work » Needs review

Ups, the status.

manningpete’s picture

Issue tags: -Needs reroll

Patch applies, no reroll needed.

joelpittet’s picture

Status: Needs review » Postponed (maintainer needs more info)

Is there an example in core that we can test?

andypost’s picture

Status: Postponed (maintainer needs more info) » Needs review

Yes, all entity_test entities have no templates so nice examples for testing

joelpittet’s picture

Component: theme system » quickedit.module
Issue tags: +Needs tests

Maybe 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?

Wim Leers’s picture

Component: quickedit.module » theme system

Per #11.

joelpittet’s picture

Component: theme system » quickedit.module

It'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.

Wim Leers’s picture

I'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, not quickedit.module. I think it's not acceptable in quickedit.module for the provided reasons.


Did that make sense?

joelpittet’s picture

Component: quickedit.module » theme system

Panels 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.

Wim Leers’s picture

RDF 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?

joelpittet’s picture

We 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"?

Wim Leers’s picture

Given #26, I pinged @fago.

fago’s picture

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"?

yep, 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?

andypost’s picture

andypost’s picture

More 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 hooks

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

effulgentsia’s picture

Title: HTML attributes for wrapping container element are not output for all entities (breaks Quick Edit module) » $build['#attributes'] added in hook_entity_view_alter() are not output for entity types that provide neither a #theme nor #theme_wrappers (breaks Quick Edit module for those entity types)
Component: theme system » entity system

Why 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.

all entity_test entities have no templates so nice examples for testing

Yes, entity_test is an example that is doing something wrong, by unsetting #theme in EntityTestViewBuilder::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.

maybe it's reasonable to expect a base of "all entity templates must print attributes

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(), or entity.api.php in the "View/render" section. If we choose to provide an entity.html.twig default per #9, that could also be a great place for this documentation. There's two parts to this requirement:

  1. Either a #theme (which by default is provided in EntityViewBuilder::getBuildDefaults()) or #theme_wrappers is needed.
  2. And the implementation of one of them needs to output {{ attributes }}.
effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/modules/quickedit/quickedit.module
@@ -157,5 +157,8 @@ function quickedit_preprocess_field(&$variables) {
+  if (!isset($build['#theme_wrappers'])) {
+    $build['#theme_wrappers'] = array('container');
+  }

Setting to Needs Work, because some problems with this approach are:

  1. We don't want to add a container div to entity types that provide a #theme that outputs attributes (e.g., all the non-test ones in core).
  2. Per #23, this needs to be fixed outside of QuickEdit.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

effulgentsia’s picture

Priority: Major » Normal
Issue tags: -Contributed project blocker

No responses to #32, so removing the "Contributed project blocker" tag, and downgrading to Normal priority.

lauriii’s picture

Priority: Normal » Major
Issue tags: +Triaged D8 major

@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.

effulgentsia’s picture

We will need to decide whether to fix this in the entity system, theme system, quickedit, or somewhere else.

I 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.