This is a spin-off from #1962606: Quick Edit + "Field" Views (table, grid …).

tl;dr: make it possible to use in-place editing on Views in Drupal 8 contrib

Edit currently only supports in-place editing of entity fields rendered by the Entity/Field API render pipeline (i.e. using field_view_field() and Entity Display view modes). This covers the majority of the use cases.

However, if we want in-place editing to also be an option in scenarios where entities (and their fields) are rendered by a different render pipeline, then we need to make it possible to let Edit call those instead of the Entity/Field API render pipeline.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.93 KB

This is an initial patch that shows the overall concept (lent from #1962606-15: Quick Edit + "Field" Views (table, grid …)).

Instead of view mode IDs like teaser and full, you can have view modes like edit_views-frontpage-page_1-34 (edit_views-$view_id-$view_display-$row_index), the edit_views- prefix indicates that this form of in-place editing is supported/implemented by the edit_views module, which then must implement a hook_edit_render_field($entity, $field_name, $langcode, <custom args>) method: edit_views_edit_render_field($entity, $field_name, $langcode, $view_id, $view_display, $row_index. The custom arguments should contain that metadata that is necessary for the alternative render pipeline to do its work.

Needs review of the overall approach. Once that is signed off, I'll work on test coverage.

Wim Leers’s picture

Priority: Normal » Major

Probably more appropriate.

Gábor Hojtsy’s picture

I only found one specific confusion with the code itself:

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -213,7 +213,25 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+      // Re-render the field. If the view mode ID is not an Entity Display view

I think "re-render" is a bit misleading. The field was not yet rendered in this request. We render it again due to the change :)

*However* I think the patch overall is confusing. If a field knows how to render itself in a view mode, then why not solve this there? If edit requires special rendering, then why not have a special render pipeline entry point for all cases (that may then rely on a core base implementation itself)?

Wim Leers’s picture

Heh… upon rereading the code, I don't find the code confusing. I *am* very confused by your comment though :D Seems like we're not yet on the same page.


"re-render" is indeed misleading. That should just be "render".


If a field knows how to render itself in a view mode, then why not solve this there?

A field does not know how to render itself. Field API knows how to render a field. That's why we call field_view_field(). The problem is that when the field is rendered as part of a table View for example, field_view_field() is never invoked at all: table Views don't use Field API's render pipeline, it uses its own render pipeline (including its own set of customization features, like the many "rewrite results" options). So we have to allow e.g. Views to invoke its own render pipeline.

If edit requires special rendering

Edit does not require special rendering at all, it only to be able to render individual fields after they've been modified during in-place editing. When fields are rendered by something else than Field API, say Views, then we need to use that other render pipeline.
This patch makes that possible, by defaulting to the Field API render pipeline and allowing a prefix containing a module name to be specified on "an Edit field ID", which would trigger the hook_edit_render_field() implementation to be called for the given module name.

Please take a look at the patch in #1962606-15: Quick Edit + "Field" Views (table, grid …) to see how Views would implement this.

That being said, I could change it so that Field module also has to implement this hook. Would that help clarify things?

Gábor Hojtsy’s picture

Right, thanks. I *think* it would be better if field module would do the same implementation and then we'd just invoke a hook with sufficient data and the hooks would figure out which one takes on rendering. I don't really have a strong opinion, this may be as fine.

Additionally to the tests, the new hook certainly needs docs.

Gábor Hojtsy’s picture

Here is an updated version with docs and unified argument structure between the "hook" and field_view_field(). Even if that function goes away or something else comes in later, we should have some kind of consistency, so I opted to use that signature for now (although field_view_field() is a bit more versatile, eg. allows an array in place of the view mode id, that edit will not do).

Wim Leers’s picture

Status: Needs review » Needs work

Nice :)

I think test coverage could be done as follows (I think you asked for feedback in this area earlier, so ignore this if you've already started):

  1. Create edit_render_pipeline_test.module.
  2. In there, implement hook_edit_render_field(), but also implement a new route (e.g. "alternative_node_render"), where you render nodes in your own particular way, and make sure that data-edit-id="node/1/<field name>/und/edit_render_pipeline_test-<customstuffhere>" is set somewhere in the DOM.
  3. Create EditLoadingTest::testAlternativeRenderPipeline, which is a WebTestBase, which should look fairly similar to the test you wrote for #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits. You'll want to make sure that editing still works, so that's why it's fine to do everything what you do there, with the exception of not expecting a validation error at the end, but instead the thing rendered through your render pipeline.

Hope this helps.

P.S.: One nitpick for the current patch:

+++ b/core/modules/edit/edit.api.php
@@ -29,5 +29,41 @@ function hook_edit_editor_alter(&$editors) {
+function hook_edit_render_field($entity, $field_name, $view_mode_id, $langcode) {

The typehint is missing here, though it is in the docs :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.94 KB
7.33 KB

Looking into this I figured out that the route / page controller is not needed, since whatever that controller returns we would hardcode anyway, so it would not test edit in any way. We do need to test edit with a custom view mode ID though, which is the part that is actually introduced here :) So IMHO it is perfectly fine to test edit with a view mode id that is otherwise not actually implemented in a full route controller render. The later would not test anything in edit.

That said, pretty much did what you suggested. Implemented the hook with identifiable extra markup, then in the test invoking edit with that render pipeline, ensuring the form worked and then that the form submission rendered back the field with the custom pipeline.

I also fixed the type hinting that you called out as well as a 'public' missing in the test :)

Wim Leers’s picture

Assigned: Wim Leers » Gábor Hojtsy
Status: Needs review » Needs work

Only a single nitpick, after which I believe this is RTBC :)

+++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
@@ -305,6 +305,48 @@ function testPseudoFields() {
+    // Request form for editing through the custom edit pipeline.

There is no such thing as a "custom edit pipeline". You're merely requesting the form; the metadata for re-rendering when editing is complete just happens to be there as well.

So, replace with: "Retrieve field form.", just like elsewhere in EditLoadingTest.

Gábor Hojtsy’s picture

Yeah, well, by requesting *this* specific URI to edit the field, I'm instructing Edit to use that render pipeline for rendering it back. So while it does not make editing itself different, the result of the edit will be different. We need to tell edit to behave differently, but then edit delegates that to the render pipeline. IMHO it would be great to express this somehow and not just bluntly say we ask for the edit form, because the test specifically tests for not using the common edit flow of rendering back with the full/teaser display.

Wim Leers’s picture

True. But "custom edit pipeline" feels even more wrong to me :) The fact is that it's the *exact* same editing form, regardless of render pipeline.

Do you have an idea how to word it better?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.33 KB

Used "Request editing to render results with the custom render pipeline." as we discussed off the issue in chat :) No interdiff as that is the only change.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Hurray! :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/edit/edit.api.php
@@ -29,5 +29,41 @@ function hook_edit_editor_alter(&$editors) {
+function hook_edit_render_field(Drupal\Core\Entity\EntityInterface $entity, $field_name, $view_mode_id, $langcode) {
+  // @todo
+}

@todo :)

alexpott’s picture

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -216,7 +216,25 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
+        $output = \Drupal::moduleHandler()->invoke($module, 'edit_render_field', $args);

We could inject the module handler.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
9.56 KB

Injcted modulehandler, replaced @todo with basic code example in hook docs.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#14/#15: darn — I should have seen those things when reviewing! Sorry :(

AFAICT, Gábor completely solved your remarks, so back to RTBC.

catch’s picture

+++ b/core/modules/edit/edit.api.php
@@ -29,5 +29,45 @@ function hook_edit_editor_alter(&$editors) {
+ * Returns a renderable array for the value of a single field in an entity.

This is almost exactly the same function description as https://api.drupal.org/api/drupal/modules%21field%21field.module/functio... I think this needs a bit more docs as to how it's different - i.e. isn't the point that it renders that specific field identically to how it would have been rendered inline?

Also does Views consistently have an entity when it renders fields? It didn't used to but I think that changed so maybe it's OK. It's a bit hard to see how this will work with Views is there a follow-up issue for that already?

Wim Leers’s picture

#18: Yes, there's a follow-up. In fact, there's two. This issue exists to make sure edit.module provides the necessary APIs. And #2010946: "Lifting" of data- attributes from a field to its containing "view cell" ([quick]edit.module integration) exists to make sure views.module provides the necessary APIs. Then there's an overarching one to provide in-place editing for Views: #1962606: Quick Edit + "Field" Views (table, grid …).

Gábor Hojtsy’s picture

@catch: not sure what kind of documentation you are looking for. The patch includes three lines on this right after the line you quoted:

+ * Returns a renderable array for the value of a single field in an entity.
+ *
+ * To be implemented by modules which need custom render pipelines (such as
+ * Views), where field_view_field() would not result in proper rendering of
+ * the field.

Which explains this is to be used when the base functionality of field_view_field() would not suffice to render the field. We can add "when rendered by edit module" to that, but I think that is implicitly understood based on this being in the edit module's API for field rendering :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

I think it at least needs to be more explicit that the output of this needs to be identical to what would be output if Views was rendering normally as opposed to the hook being invoked.

Additionally it's not at all clear that field_view_field() might be called by the custom rendering pipeline. If I'm calling field_view_field() with an array for $display, then that also won't work with edit module, but field_view_field() absolutely will result in proper rendering of a field because that's what the custom pipeline is doing. Possibly 'field_view_field() with display mode" would be enough but it's not clear at all why field_view_field() isn't good enough from the current comment.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
10.06 KB

Here is a suggested update to the docs. As for the $view_mode vs. $display_options on field_view_field(), edit takes the view mode from the path and can therefore only be a string in edit. That is true. I'm honestly not sure what edit may miss by that and I am admittedly pretty clueless there. Moving to needs review back to Wim I guess.

Wim Leers’s picture

So AFAICT catch had two concerns:

  1. The docs weren't explicit enough that the output would need to be identical to the original output. Gábor has fixed that.
  2. Calling field_view_field() with an array of display options rather than a view mode is not supported by Edit. Nicely spotted. Incredible that none of us noticed that in the past year or so. I think it makes sense that Edit only supports view modes. But that also means it should explicitly not add a data-edit-id attribute to fields that *are* rendered with an array of display options, to prevent Edit from breaking.

I have one question for catch though:

[…] it's not clear at all why field_view_field() isn't good enough from the current comment.

Are you saying it's not clear why field_view_field() is not good enough in all scenarios, even when a field is rendered by Views inside a Views table for example?

catch’s picture

It's clear to me why it's not good enough, but it wasn't clear from the documentation. So what needs to be more explicit in the docs was that edit only works with field_view_field() and a string display mode. I didn't realise that edit was trying to support that at the moment had just assumed it wouldn't bother if field_view_field() was passed $display as an array, but that explains why the docs didn't make this explicit in the first place!

Wim Leers’s picture

I've created an issue to make the necessary changes to Edit to explicitly only support view modes: #2120335: Edit module only supports view modes, field_view_field() with a $display_options array is not supported.

Is there anything else you would like to see changed here? (I'm having trouble parsing #24.)

webchick’s picture

Assigned: Gábor Hojtsy » catch
Status: Needs review » Reviewed & tested by the community

Moving back to catch for feedback.

catch’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

Assigned: catch » Gábor Hojtsy
Issue tags: -sprint

Lovely, thank you! :)

Status: Fixed » Closed (fixed)

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