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.
Comment | File | Size | Author |
---|---|---|---|
#22 | edit_views_possible-2083387-22.patch | 10.06 KB | Gábor Hojtsy |
#22 | interdiff.txt | 1.28 KB | Gábor Hojtsy |
#16 | edit_views_possible-2083387-16.patch | 9.56 KB | Gábor Hojtsy |
#16 | interdiff.txt | 3.45 KB | Gábor Hojtsy |
#12 | edit_views_possible-2083387-12.patch | 7.33 KB | Gábor Hojtsy |
Comments
Comment #1
Wim LeersThis 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
andfull
, you can have view modes likeedit_views-frontpage-page_1-34
(edit_views-$view_id-$view_display-$row_index
), theedit_views-
prefix indicates that this form of in-place editing is supported/implemented by theedit_views
module, which then must implement ahook_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.
Comment #2
Wim LeersProbably more appropriate.
Comment #3
Gábor HojtsyI only found one specific confusion with the code itself:
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)?
Comment #4
Wim LeersHeh… 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".
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.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?
Comment #5
Gábor HojtsyRight, 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.
Comment #6
Gábor HojtsyHere 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).
Comment #7
Wim LeersNice :)
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):
edit_render_pipeline_test.module
.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 thatdata-edit-id="node/1/<field name>/und/edit_render_pipeline_test-<customstuffhere>"
is set somewhere in the DOM.EditLoadingTest::testAlternativeRenderPipeline
, which is aWebTestBase
, 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:
The typehint is missing here, though it is in the docs :)
Comment #8
Gábor HojtsyLooking 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 :)
Comment #9
Wim LeersOnly a single nitpick, after which I believe this is RTBC :)
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
.Comment #10
Gábor HojtsyYeah, 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.
Comment #11
Wim LeersTrue. 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?
Comment #12
Gábor HojtsyUsed "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.
Comment #13
Wim LeersHurray! :)
Comment #14
alexpott@todo :)
Comment #15
alexpottWe could inject the module handler.
Comment #16
Gábor HojtsyInjcted modulehandler, replaced @todo with basic code example in hook docs.
Comment #17
Wim Leers#14/#15: darn — I should have seen those things when reviewing! Sorry :(
AFAICT, Gábor completely solved your remarks, so back to RTBC.
Comment #18
catchThis 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?
Comment #19
Wim Leers#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 …).
Comment #20
Gábor Hojtsy@catch: not sure what kind of documentation you are looking for. The patch includes three lines on this right after the line you quoted:
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 :)
Comment #21
catchI 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.
Comment #22
Gábor HojtsyHere 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.
Comment #23
Wim LeersSo AFAICT catch had two concerns:
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 adata-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:
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?Comment #24
catchIt'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!
Comment #25
Wim LeersI'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.)
Comment #26
webchickMoving back to catch for feedback.
Comment #27
catchI think #2120335: Edit module only supports view modes, field_view_field() with a $display_options array is not supported covers any remaining follow-up. Committed/pushed to 8.x, thanks!
Comment #28
Wim LeersLovely, thank you! :)