This is fine, but then Edit should 1) document this, 2) not add a data-edit-id attribute if no valid View mode is defined.

This was implicitly reported at #2083387-23: Make it possible to use in-place editing on entities rendered by a custom render pipeline (e.g. Views).

CommentFileSizeAuthor
#4 interdiff.txt725 byteswim leers
#4 edit_view_modes_display_options-2120335-4.patch1.96 KBwim leers
#1 2120335-1-test_only_fail.patch1.08 KBwim leers
#1 edit_view_modes_display_options-2120335-1.patch1.91 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Updated issue summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Spark, +sprint
Related issues: +#2083387: Make it possible to use in-place editing on entities rendered by a custom render pipeline (e.g. Views)
StatusFileSize
new edit_view_modes_display_options-2120335-1.patch1.91 KB
new 2120335-1-test_only_fail.patch1.08 KB

Et voila.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented 
+++ b/core/modules/edit/edit.module
@@ -178,6 +178,13 @@ function edit_preprocess_field(&$variables) {
+  // Edit module only supports view modes, not dynamically defined "display
+  // options" (which always result in the "_custom" view mode).
+  // @see https://drupal.org/node/2120335
+  if ($element['#view_mode'] === '_custom') {
+    return;
+  }

Hah, I looked for explanation on that link and it got back to this issue. Where is this behaviour explained? Why is this a surefire way to do the checking?

Status: Needs review » Needs work

The last submitted patch, 1: 2120335-1-test_only_fail.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new edit_view_modes_display_options-2120335-4.patch1.96 KB
new interdiff.txt725 bytes

Because that is the view mode field_view_field() sets when not passing a view mode, but instead generating one "on the fly" using "display options": https://api.drupal.org/api/drupal/core%21modules%21field%21field.module/....

You're right that this should be explicitly documented though, so fixed that in this reroll.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: edit_view_modes_display_options-2120335-4.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

4: edit_view_modes_display_options-2120335-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: edit_view_modes_display_options-2120335-4.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

4: edit_view_modes_display_options-2120335-4.patch queued for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

4: edit_view_modes_display_options-2120335-4.patch queued for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #5, now that testbot is finally cooperating.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This patch is just updating the code to reality, so seems fine.

Committed and pushed to 8.x. Thanks!

However. I have absolutely no frigging idea what you are talking about :P and since the code in question links back to this issue, could you please update the issue summary with some English? :) I think what you mean is straight-up fields on e.g. a node or block will work, but if the field is rendered in a custom way, like for example Views saying to replace some token text in the field, then it won't.

...but I have no frigging idea, so a concrete example would be super helpful. :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Helps to read the automated test, I guess.

So basically, if under admin/structure/types/manage/article/display you have changed any of the selections there from their defaults, in-place editing can't work..? Or only if this is changed at display-time vs. configure-time?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

In fact, the code *does* contain the full English explanation that you need, it's just bizarre/confusing terminology in Field API:

+  // Edit module only supports view modes, not dynamically defined "display
+  // options" (which field_view_field() always names the "_custom" view mode).
+  // @see field_view_field()

If you look at field_view_field()'s $display_options parameter you'll see it accepts two arguments: 1) the name of a view mode, 2) a dynamically defined array of display options. What the docs say is that only 1) is supported, not 2).
In other words: if you create (or modify) a view mode ("Entity Display" in D8 parlance), in-place editing will work. Otherwise, if you dynamically define an array of "display options", it won't. Why? Because Edit doesn't have a view mode to refer to, to know how to rerender the field when in-place editing.

I hope that sufficiently clarifies it for you!

Status: Fixed » Closed (fixed)

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