Updated: Comment #60

Sandbox: http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/fiel...

Problem/Motivation

The field_attach_form_*() functions:
- are old D7-style functional code
- live in field.module but are called from Core/Entity, which is backwards
- are still formulated in the old entity translation model (receive an $entity and a $langcode)
- are still largely based on code that predates EntityNG / Entity translation (convoluted field_invoke() / field_invoke_method() iterators)

Proposed resolution

Replace them by simple iterators in EntityFormDisplay objects, that assume that multilingual logic has been resolved upstream.
See #30 / #31 / #40 for discussion leading to EntityFormDisplay.

(obsoletes a previous approach that placed the methods in a new ContentEntityFormHelper service - see #20-#23 for discussion about this former approach)

User interface changes

None

API changes

TODO

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

Tagging, we really want to track this :)

yched’s picture

Issue tags: -Field API

The sandbox simply contains current HEAD for now.

Wim Leers’s picture

Issue tags: -Entity Field API

I think this is also the branch where a new equivalent for field_attach_form() and friends will be introduced?

(Rationale for why that is necessary, as per the discussion yched, amateescu and I had at #1901100-97: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits, in the bottom.)

EDIT: link to a specific comment in that issue.

Wim Leers’s picture

Issue summary: View changes

link to sandbox

Berdir’s picture

Issue tags: +Field API

Note that entity forms don't yet support multiple entities (core as a test case for this, so we'll notice, real examples are e.g. inline entity form)

See these issues, which are all related:
#1728816: Ensure multiple entity forms can be embedded into one form
#766146: Support multiple forms with same $form_id on the same page
#2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects

andypost’s picture

yched’s picture

Very preliminary patch, but that's where I'm at before going afk :-/

This removes the calls to field_attach_form_*() in EntityFormController, and replaces them with simple iterations on fields.
This is mostly a demonstration of how simple this can be done once #2019055: Switch from field-level language fallback to entity-level language fallback gets in (multilingual tests will fail here).
Also, the code currently still only works on configurable fields, because of explicit restrictions placed in EntityFormDisplay::getRenderer(). In the end, that same code should work on all fields (base, config) that are known by the EntityFormDisplay.

Still @todo:
- Adapt other code that calls field_attach_form_*() (edit.module, tests...), and remove the functions completely.
This might require stepping back a little and see what helpers might be needed for the "single field" cases needed by edit.module
- Do the same for f_a_view() / f_a_prepare_view()...

The code has been pushed to the sandox (field_attach-remove-2095195 branch)

effulgentsia’s picture

Issue summary: View changes
Issue tags: +beta target

Per http://buytaert.net/the-next-step-for-drupal-8-is-a-beta, this is the kind of thing that will be much more disruptive to remove after beta than before, so tagging as a beta target.

yched’s picture

Issue tags: +Entity Field API

wrong tag :-/

yched’s picture

Priority: Normal » Major

"beta target" -> bumping priority accordingly

yched’s picture

Reviving this. Reroll + testbot for now.

plach’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: field_attach-remove-2095195-10.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
5.06 KB

Silly typos

yched’s picture

Status: Needs review » Needs work

ok, back to NW then.

yched’s picture

yched’s picture

Status: Needs work » Needs review
FileSize
33.75 KB

Still work to be done, but this is starting to come into shape.

Will post more comments later on, but for now this should be green (fingers crossed, bot acted a little shaky in the helper issue)

Points of interest:
- bottom of ContentEntityFormController, which includes three "widget attacher" methods (generate, extract values, validate). Those are the (way simpler) replacements for field_attach_form*().
- the rest of the class uses them,
- edit.module's EditFieldForm uses them too.

For now they are static methods in ContentEntityFormController, which means EditFieldForm neets to get to the right form controller, and uses it as a mere holder of helper methods - which does feel a bit washy IMO.

From my @todo notes in the patch:

  // @todo Decide where those "generic widget attacher" methods should go.
  // - Stay as static methods here ?
  // - Switch to regular, non static methods ?
  //   then $entity & $form_display params should be removed
  //   in favor of getFormDisplay() / getEntity(),
  //   It means other forms like EditFieldForm would need to call 
  //   setEntity() / setFormDisplay() before calling those.
  // - Move to a different, yet to be introduced, ContentEntityFormHandler helper class
  //   that is *not* a form itself and is thus cleaner to reuse ?
  //   But for now it would only make sense for ContentEntities ...
amateescu’s picture

+++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
@@ -80,13 +80,16 @@ public function getFormId() {
+    $class = \Drupal::entityManager()->getControllerClass($entity->entityType(), 'form', 'default');

@@ -136,7 +146,9 @@ protected function init(array &$form_state, EntityInterface $entity, $field_name
+    $class = \Drupal::entityManager()->getControllerClass($entity->entityType(), 'form', 'default');

@@ -171,7 +183,8 @@ protected function buildEntity(array $form, array &$form_state) {
+    $class = \Drupal::entityManager()->getControllerClass($entity->entityType(), 'form', 'default');

This a problem.. an entity type might not have a 'default' form operation/controller defined, but a 'edit' one instead. We should first check for 'edit' and then default to 'default' :)

  // - Move to a different, yet to be introduced, ContentEntityFormHandler helper class
  //   that is *not* a form itself and is thus cleaner to reuse ?
  //   But for now it would only make sense for ContentEntities ...

As I proposed it in IRC, this would be my preference, introduce a 'content_entity_form_builder' class/service.

I see that you're also doing #2151775: EntityFormController does things that only make sense for ContentEntities as part of this issue.. can we close that one as a dupe?

Wim Leers’s picture

Some relevant feedback regarding the affected Edit module hunks — not a review:

  1. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -80,13 +80,16 @@ public function getFormId() {
    +  // @todo can $entity / $field_name be NULL ??
    

    No, never :)

  2. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -80,13 +80,16 @@ public function getFormId() {
    +    $class = \Drupal::entityManager()->getControllerClass($entity->entityType(), 'form', 'default');
    ...
    +    // @todo should the class be a property in $this ?
    

    Probably :)

  3. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -128,7 +131,14 @@ protected function init(array &$form_state, EntityInterface $entity, $field_name
    +    // @todo Revisit what really needs to be in $form_state...
    ...
    +    $form_state['form_display'] = $display;
    

    The idea here (IIRC, I think amateescu introduced this code) is that $form_state['form_display contains the form display mode that Edit will use. By default, that's the 'default' form display mode. A contrib module could provide a UI and always override $form_state['form_display'].

yched’s picture

Thanks for the feedback, folks.

So, moving attachWidgets() / validateWidgets() / extractWidgetsValues() (actual names TBD) out of the "Entity FormBase forms" to a more generic helper class where they can more cleanly be used by other forms makes sense to me too.

As discussed in #2090509-20: Optimize entity_get_render_display() for the case of "multiple entity view", this class could also be the place where entity_get_render_form_display() ends up, while entity_get_render_display() goes to EntityViewBuilder where it's a perfect match.

But then this means:
- adding a new 'entity_controller' (/handler) key - which brings #1976158: Rename entity storage/list/form/render "controllers" to handlers into the mix ;-)
- adding corresponding getXxxController() methods to EntityManager()
- agreeing on names for the above ('form_builder' / ContentEntityFormBuilder ? symmetrical with EntityViewBuilder, but are we ok having a "form builder" and "form controllers" next to each other ?)
- putting the entry into the annotations of all existing (content) entity types (a bit verbose since they will all use the same class ?)
- what about config entity types for which none of this make sense ?

Before embarking in this, I'd like to get agreement from @fago / @Brdir :-)

yched’s picture

Also, TBH, I still haven't completely wrapped my mind around our current 'form' controllers, what it means that there are several keyed by "operation", what's common between an edit form and a delete form, what's the exact relation with form_modes... :-)

amateescu’s picture

FWIW, I'm not suggesting a new entity "controller", just a regular class/service, similar to 'form_builder' but with methods needed by (content) entity forms.

And yes, I know I need to write that change notice about form modes and how they relate to form controllers :) Initially, I was waiting for #2006348: Remove default/fallback entity form operation which would change this area a bit, but that issue has been stalled for months so I'm not sure if it's worth waiting any longer..

yched’s picture

re "Just a regular class / service":

Well, that would be simpler for sure.
That would also break the existing pattern we have for entity helper classes - the entry point is the entity manager, and from there you get the controller class for the task you're performing (access check, content translation...).

But why not - I guess there are already existing "per entity type" controllers that use services for part of their logic.

So we would have :

On the form side, methods to:
- collect the EntityFormDisplay to use for a given entity in a given form mode (our current entity_get_render_form_display())
- generate & process widgets for the fields of an entity according to an EntityFormDisplay (our current field_attach_form*())

--> Living in a service (name TBD) in the container, that EntityFormControllers and other forms can use.

On the View side (deal with multiple $entities), methods to:
- collect the EntityFormDisplay*s* to use for a given set of entities in a given view mode (our current entity_get_render_display()
- generate formatter output for the fields of a set of entities according to an EntityDisplay
(our current field_attach_view*() + field_view_field() / field_view_value() single-field shortcuts)

--> So far the entity type's EntityViewBuilder controller was the envisioned place for those.

I'd like to preserve symmetry and consitency between form & view / widgets & formatters, it's always been a great sanity help so far. So:
- maybe the methods on the view side should go in a service as well, that the EntityViewBuilder would use ?
- maybe it's one single service for both ? [hmm - maybe not. we have FormatterPluginmanager / WidgetPluginManager, EntityViewDisplay / EntityFormDisplay, so probably two separate, symmetrical services here as well...]

This might make it easier for contrib to switch in logic for "EntityViewDisplay entity per entity" - something I've been willing to allow since we introduced the Displays, but was unsure how.

I'd still welcome opinions from @fago & @berdir, but I'll probably shoot for that in the next patches. Once we have those services, we can see what names make sense for them (promises to be fun :-p)

yched’s picture

Patch does as per #23 - form side only.

yched’s picture

Non-important stuff:

- Unified method names in ContentEntityFormHelper a bit:
attachWidgets()
extractWidgetsValues()
validateWidgetsValues()
- removed all existing calls to field_attach_form*()
(doing so, moved field_test_entity_nested_form() to a FormController)
- moved & adapted the imprtant pieces of phpdoc from the old functions.

The significant patch hunks are still:
a) The new service:
- core/core.services.yml
- core/lib/Drupal/Core/Entity/ContentEntityFormHelper.php
b) Example of forms that use it:
- core/lib/Drupal/Core/Entity/ContentEntityFormController.php
- core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php

effulgentsia’s picture

Title: Remove field_attach_[form|view]() » Remove core usage of deprecated field_attach_[form|view]()
Issue tags: -beta target +beta blocker
Parent issue: » #2061107: Remove deprecated procedural functions in Field API

Per #2061107-9: Remove deprecated procedural functions in Field API, we can't remove the functions entirely in this issue, and the latest patch here doesn't, so retitling. Raising to beta blocker since that other issue is and this is a requirement for it. Not raising to critical, because the parent issue isn't. I'm not yet clear on what core maintainers plan to do if and when there are no critical, but only major, beta blockers outstanding: they'll probably need to make a judgment call at that time or before then, as to whether to raise to critical or remove the beta blocker tag.

yched’s picture

yched’s picture

Issue summary: View changes

Updated the issue summary with the approach in the current patch, and pointers to #20 - #23 for the discussion.

swentel’s picture

First quick review - looks sweet overall - some small things

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormHelper.php
@@ -0,0 +1,211 @@
+ */ ¶

Obsolete space

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
@@ -131,34 +147,42 @@ public function isDefaultFormLangcode(array $form_state) {
+        call_user_func_array($function, array($entity->entityType(), $entity, &$form, &$form_state));

Is passing the entity type still relevant, it's on the $entity no ? (maybe out of scope)

fago’s picture

Looking at the patch and reading comments, I still think that a separate "ContentEntityFormHelper" doesn't make much sense. If it is about re-usable logic related to entity forms it should be provided in a base class. However, it actually seems to be more related about the interaction of widgets and entity forms - what makes me think that it would make sense to part of the widget manager.

We could do likewise for formatters and widgets, which both exist for being rendered as part of an entity view/form, so providing according utility methods + helpers as well makes sense to me.

- "regular" EntityFormControllers
- other forms like edit.module's EditFieldForm, or our test "nested entities" form.

I could still see those other uses become/use entity form base class as well, a form for editing a single entity field still is an entity form for me. That's probably out of scope for this issue though.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormControllerInterface.php
@@ -0,0 +1,41 @@
+interface ContentEntityFormControllerInterface extends EntityFormControllerInterface {

Also, I'm still hesitant about introducing this separate interface.

Even if config entities do not use the display, I think should have a single API where people can expect them to be there. If you'd render a config entity you'd end up having display objects as well - even if empty. Moreover I can see them becoming useful for config entities also, in particular once we have different kind of display components.

yched’s picture

I could still see those other uses [edit.module's EditFieldForm, or our test "nested entities" form] become/use entity form base class as well, a form for editing a single entity field still is an entity form for me. That's probably out of scope for this issue though.

Well, that's the whole question I've circling around for a couple weeks now :-).

It seems @wim had troubles making that happen - see #2090509: Optimize entity_get_render_display() for the case of "multiple entity view", comment #23 and following. Our current EntityFormControllers still seem very much tied to the case of being "the main edit form for the entity". Some entity types hardcode form elements in the form() method in there, some 3rd party code hardcode alters on those. I don't really see them become clean reusable base classes for "any form that will do stuff with an entity" soon :-/.

If it is about re-usable logic related to entity forms it should be provided in a base class. However, it actually seems to be more related about the interaction of widgets and entity forms

See #23 - we also need a place to put the logic for "determine the EntityDisplay object(s) to use for a given view/form_mode" - the current entity_get_render_(form)_display(). I was planning to put them on the same "generic helper classes" that receive the attach[Widgets|Formatters]() methods - but that can't we the {Widget|Formatter]PluginManager.
Where would those go then ? As I wrote in #23 too, putting that logic in a swappable service would make it easier to provide alternate implementations allowing "per entity overrides" in contrib.

Also, we currently have a logic of :
- "Some form code" uses an EntityFormDisplay for building its form
- That Display knows the widget settings to use, so it's the one who talks to the WidgetPluginManager and fetches & persists the instanciated $widget plugins for the consuming code (Display->getRenderer($component_name))

If the attachWidgets*() methods live on the WidgetPluginManager, the resulting code flow becomes a weird dance between the Manager and the Display that would IMO be really convoluted to follow.

Maybe put the attach[Widgets|Formatters]*() methods on the Display itself, then ? Drawback: means more dependencies for those (a FormBuilder, probably a ModuleHandler...).
And still: how do you get to that display object ? i.e where does entity_get_render_(form)_display() live ?

Even if config entities do not use the display, I think should have a single API where people can expect them to be there. If you'd render a config entity you'd end up having display objects as well - even if empty. Moreover I can see them becoming useful for config entities also, in particular once we have different kind of display components.

Hmm - that sounds like science fiction to me at this point, honestly.
Rendering a ConfigEntity makes no real sense to me, and we're not considering making ConfigEntity forms use widgets or be configurable in a UI ?

The fact that ConfigEntity forms currently try to load an EntityFormDisplay object and bake some logic aroud it feels aberration IMO. I guess we could keep the methods in the interface I guess, but then the base implementations should be empty on the config side, and the base code should not have anything to do with displays. Having the methods there would still feel weird IMO, but I guess I could live with that ?

xjm’s picture

Issue tags: +API change

Also the API changes are presumably approved but let's get confirmation once we know what they are (maybe we already do). :)

xjm’s picture

Title: Remove core usage of deprecated field_attach_[form|view]() » Remove deprecated field_attach_[form|view]()

Discussed with @berdir in IRC. When @webchick, @effulgentsia, and I discussed these issues last week, @webchick expressed that she'd prefer to:

  1. Remove the old procedural API in as few commits as possible while respecting issue scope.
  2. Not leave HEAD in a "half-done state" where possible.

Based on these two criteria, and since the API change here is pretty much approved, let's remove the functions in this issue as well. @berdir and @yched both mentioned that this issue might need to be split in two (one for *_form() and one for *_view()); if so, then let's make both issues beta blockers and remove the appropriate functions in each issue. That way we're looking at at most three additional commits to remove the remainder of the deprecated D7 API. Thanks!

xjm’s picture

Priority: Major » Critical

Also, I'm comfortable calling this critical. The related issues already are, and the field attach API in D7 was frankly the most confounding part of the whole system. :)

xjm’s picture

And this too.

yched’s picture

Reroll, + rmoves the old functions.

The last submitted patch, 36: field_attach-remove-2095195-36.patch, failed testing.

yched’s picture

Title: Remove deprecated field_attach_[form|view]() » Remove deprecated field_attach_form_*()
FileSize
84.5 KB
2.14 KB

Fixing fails for the current patch - updates the newly introduced CustomBlockFormController.

+ Since getting an agreed approach might take some more time, forked off #2167267: Remove deprecated field_attach_*_view() as per #33

Wim Leers’s picture

I wanted to try #38 to see how it works for Edit module. Unfortunately, I can't get D8 HEAD working anymore, neither with PHP 5.3, nor with 5.4, nor on simplytest.me. I'll provide manual testing feedback later.

effulgentsia’s picture

Maybe put the attach[Widgets|Formatters]*() methods on the Display itself, then ?

I like this option the best. To be a proper domain object, a Display should be the entry point for everything having to do with displaying the stuff it's configured to display. It could farm stuff out to helpers as needed, e.g. to (Formatter|Widget)Manager, but I don't see the benefit of farming out the attach*() functions.

Drawback: means more dependencies for those (a FormBuilder, probably a ModuleHandler...).

I don't see this as a drawback. A FormDisplay depending on FormBuilder makes sense to me, and we have other config entities that depend on ModuleHandler already anyway.

And still: how do you get to that display object ? i.e where does entity_get_render_(form)_display() live ?

How about a static method on EntityDisplay/EntityFormDisplay? The caller (e.g., ContentEntityFormController) would need to invoke the method on the correct class, which it could get from EntityType->getClass().

This might make it easier for contrib to switch in logic for "EntityDisplay entity per entity"

Such a contrib project would currently need to subclass EntityDisplay anyway, at a minimum to override its id() method, at which point, it could override the static method just as easily.

amateescu’s picture

#40 sounds good to me, I won't shed any tear for the separate helper service I proposed above :)

yched’s picture

Thanks for the feedback.
I'll give a shot at moving the attachWidgets*() methods to EntityFormDisplay.
(note: for symmetry, this means that #2167267: Remove deprecated field_attach_*_view() would add its methods to EntityDisplay as well)

Not sure I fully see the "static EntityDisplay::getRenderDisplays()" solution to allow "Display per entity" in contrib, but supporting this case will require further changes at several places in the current entity_view() stack that currently assume "an array of displays keyed by bundle", so that will need an issue of its own, if at all.

yched’s picture

More generally, and probably independently of where we put the "manage field widgets" methods :

I think ultimately what we need is some generic & reusable logic to handle "an entity of type Foo being placed in a form, validated, submitted and saved", be it for :
- the canonical "single entity forms" defined by the entity type provider,
- other forms like edit's "single field form"
- nested forms with several entities at different places in the $form structure - aka Inline Entity Form, or our simple core surrogate field_test_entity_nested_form().

That means, using an EntityType's FormController *for some of its code*, and not because it's a standalone form itself - since the actual form is something else, for example a larger one.

Meaning, something very much like the nesting / #parent-aware ability we added to field_attach_form() towards the end of the D7 cycle, but on the level of the whole entity, including validating and saving.

Interestingly enough, most of the job back then for "nestable field_attach_form()" was about clustering $form_state: while the $form you build might be a subpart of a larger form structure, the $form_state you receive and manipulate is the one of the real, complete form. So you need to isolate a part of $form_state that's your own, and only work in there.

It's reason 1. why EntityFormController is currently not nestable-friendly : all its code acts like it owns the $form_state, so there's no logic in there that can be reused for a nested form. Should be fixable.

Reason 2. is that the code in EntityFormController and its current concrete subclasses (NodeFormController, ...) are a mix of "generic processing for the entity type in a form" and "stuff that should happen when submitting node/42/edit" (e.g drupal_set_messages, redirects...). That might be harder to unravel :-/.

yched’s picture

shameless plug: found #2169875: Remnant D7 code in NodeFormController needlessly skips submitForm() while digging in the save() / submit() / submitForm() subtleties in EntityFormController.

sun’s picture

Issue tags: +@deprecated
Berdir’s picture

I had a problem in #2114707: Allow per-bundle overrides of field definitions when trying to remove the hack in EntityDisplayBase::getFieldDefinitions().

Right now, it does check if the target entity is a content entity and if not, doesn't return any field definitions.

The call is coming from EntityFormController::init().

@amateescu told me that this will fix it, so posting here that I will keep that hack for now.

We discussed that we should probably throw an exception when someone tries to create a form/view display for a config entity, as that is not supported, then we can remove the check and instead fail early with a useful error (if I remove it now, I get fatal errors).

yched’s picture

@Berdir: yes, this patch cleans up *EntityFormController so that only ContentEntityFormController tries to load an EntityDisplay.

We discussed that we should probably throw an exception when someone tries to create a form/view display for a config entity, as that is not supported

Agreed. Opened #2181511: Throw an exception when creating an EntityDisplay for a non-ContentEntity

amateescu’s picture

@yched, then we should close #2151775: EntityFormController does things that only make sense for ContentEntities as a duplicate? Is there anything else in there that is not fixed by this patch?

yched’s picture

Xano’s picture

Can we please fix #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects before we commit this patch? Otherwise we'll break Inline Entity Form, Payment and probably a couple dozen smaller contributed modules that need to embed entity forms into other forms.

Berdir’s picture

@Xano: Core has a test with 2 entities in the same form too that needs to be taken care off, so I expect this will support that use case. And there's edit.module, which needs per-field edit forms. That's why it's not a trivial patch that simply moves stuff into the entity form base class ;)

yched’s picture

@Xano: The current approach, although not implemented in the latest patch nor reflected in the issue summary, is to put the replacement functions for field_attach_form_*() in the EntityFormDisplay object.
So they will be accessible to any code just like field_attach_form_*() are available in D7, and this won't make things any better or worse for IEF when it comes to "embedding field widgets in a form".

#43 has considerations that are actually more related to #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects, and that I still need to move over there.

But no, let's not postpone.

swentel’s picture

+ inline entity form doesn't even have a 8.x branch yet.

swentel’s picture

swentel’s picture

we could probably use getEntityTypeId() too, but let's see if this gets back green first.

swentel’s picture

Also pushed into branch

Status: Needs review » Needs work

The last submitted patch, 54: field_attach-remove-2095195-54.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
85.95 KB
3.13 KB

Fix order of injection and use getEntitytypeID() method - should fix most of the tests.

Status: Needs review » Needs work

The last submitted patch, 58: field_attach-remove-2095195-58.patch, failed testing.

yched’s picture

Status: Needs work » Postponed

@swentel: thanks for the rerolls !

Probably no need to sweat too much on the fails for now though. The currently agreed approach is to move the field_attach_*() functions to the EntityDisplays, rather than in a new helper service as the current patch does. This will reshuffle code quite a bit, and notably remove all the injection / constructor changes.

This approach is being implemented and validated in #2167267: Remove deprecated field_attach_*_view(). When that one is committed (or at least RTBCed), I'll reroll this one accordingly.

So, it's probably best to postpone while #2167267: Remove deprecated field_attach_*_view() progresses. I should have done this earlier - sorry.

yched’s picture

Issue summary: View changes

Updated the issue summary to reflect the current status.

yched’s picture

swentel’s picture

Ah damn, I'll do a review on the other one for tomorrow to get that one forward so we gain some new momentum :)

yched’s picture

Status: Postponed » Active

#2167267: Remove deprecated field_attach_*_view() is close to RTBC, we can unpostpone this one.

yched’s picture

So, as per #2167267: Remove deprecated field_attach_*_view() and the EntityViewDisplay::build($entity) method,
the plan here is to move
field_attach_form()
field_attach_form_validate()
field_attach_extract_form_values()
to three methods in EntityFormDisplay(), that will act on "all components in the FormDisplay except extra_fields".

Like in the other issue, first challenge is going to be naming the methods...
$display->buildForm($entity, $form, $form_state)
$display->validateFormValues($entity, $form, $form_state)
$display->extractFormValues($entity, $form, $form_state)
?

sun’s picture

Would it make sense to simply implement FormInterface?

The only straw method would probably be getFormId(), but perhaps it would be acceptable to implement that as no-op?

If that idea doesn't fly, then I'd at least go with the identical method names (+ ideally also signature), for DX/consistency reasons.

yched’s picture

@sun :

Would it make sense to simply implement FormInterface?

"EntityFormDisplay implements FormInterface" would feel very weird IMO. Our current FormInterface forms are very much full-fledged form atm.

I see that you posted thoughts about reconsidering that in #2006248-27: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects. No time for a deep read right now, but see #43 above about the challenges specific to EntityFormControllers.

amateescu’s picture

New patch that takes the same direction as #2167267: Remove deprecated field_attach_*_view(), with the method names proposed by @yched in #65. Sadly, I can't provide a useful interdiff because the reroll has been quite messy after all the recent changes in core.

Status: Needs review » Needs work

The last submitted patch, 68: field_attach-remove-2095195-68.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
64.28 KB
1.01 KB

Just a copy/paste problem :)

jibran’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
@@ -60,33 +56,36 @@ public function form(array $form, array &$form_state) {
+  public function processForm($element, $form_state, $form) {

Doc block of the function is missing.

Xano’s picture

I'm taking care of that, and the re-roll.

Xano’s picture

FileSize
65.93 KB

Re-roll, addressed #71, and I added a few minor bits of in-line documentation for type hinting. Everything else looked OK.

yched’s picture

@Xano:

  1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
    @@ -315,12 +316,13 @@ public function extractFormValues(FieldItemListInterface $items, array $form, ar
    +  public function flagErrors(FieldItemListInterface $items, ConstraintViolationListInterface $violations, array $form, array &$form_state) {
    +    /** @var \Symfony\Component\Validator\ConstraintViolationListInterface|\Symfony\Component\Validator\ConstraintViolationInterface[] $violations */
    

    not sure why that @var typehint was added, the the method already has a typehint ?

  2. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
    @@ -356,6 +358,7 @@ public function flagErrors(FieldItemListInterface $items, array $form, array &$f
    +        /** @var \Symfony\Component\Validator\ConstraintViolationListInterface|\Symfony\Component\Validator\ConstraintViolationInterface[] $delta_violations */
    

    $violations_by_delta is an array, not a ViolationList, it's assembled just above.

(More generally, I'm still :-/ on those inline @var hints...)

yched’s picture

- Fixed the remarks about inline @var hints in #74
- Added a @todo about assigning weights for extra fields in ContentEntityFormController, like we have in EntityViewBuilder
- Cleaned up remnant stuff about 'constraint_violations', they're not passed in $form_state anymore
- Added the proper hint on ContentEntityInterface in EntityFormDisplay::collectRenderDisplay(), that we couldn't add in #2090509: Optimize entity_get_render_display() for the case of "multiple entity view"
- Removed field_invoke_method() & the rest of field.attach.inc completely, it's not used anymore. Yay !

yched’s picture

The only remaining question is what become of hook_field_attach_form() & hook_field_attach_extract_form_values().
TBH, I'm not sure about the use case for those.

- hook_field_attach_form() allows altering a $form array containing all the widgets for the entity
Same as in #2167267: Remove deprecated field_attach_*_view(), it doesn't duplicate hook_[entity]_form_alter() because that one only runs on "full entity forms" (EntityFormController), not for nested forms.

But we already have hook_field_widget_form_alter(), which runs for each widget individually (there's no equivalent for formatters for performance reasons).
So hook_field_attach_form() only adds the ability for "cross widget" manipulation ?

- hook_field_attach_extract_form_values() lets you alter the $entity after widgets values have been extracted into it.
No really sure what are the use cases either - asked for feedback on twitter : https://twitter.com/yched/status/436672312235282432

fago’s picture

Started reviewing - not progressed far yet. Here a start though:

Would it make sense to simply implement FormInterface?

I think making it implement something along the lines of (the to be added) EmbeddableFormInterface would make sense here, also see #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects. Could it be the solution for embedding entity forms? You don't necessarily want to embed a 100% accurate cppy anyway, so embedding a possibly different display variant makes sense. Can be a follow-up, of course.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
    @@ -63,30 +59,39 @@ public function form(array $form, array &$form_state) {
    +    // Assign the weights configured in the form display.
    +    // @todo: Once https://drupal.org/node/1875974 provides the missing API,
    

    Seems to be the business of the form display, thus should live over there?

  2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
    @@ -7,9 +7,153 @@
    +   * accessed by field_form_get_state() and field_form_set_state().
    

    Should be moved to methods.

yched’s picture

Implementing EmbeddableFormInterface when it lands definitely sounds like something we should look into. Haven't looked at the latest state of the patch over there though.

The main question IMO remains : in an "embedded entity form", what do we want to embed exactly ?
- the output generated by the EntityFormDisplay here (mostly : field widgets) ?
- the "non-widget" extra-fields, the entity building / saving behavior, the buttons, the hook_entity_form_alter()s... provided by the EntityFormController ?

But well, this shouldn't really change what we're currently doing here, so +1 for followup (or #1728816: Ensure multiple entity forms can be embedded into one form, that's already there for that)

#77.1 : not sure what you mean ?

#77.2 : yep, that's #2200355: Move functions in field.form.inc into Core/Field. Might be affected by what happens on the EmbeddableFormInterface front, though, because what those functions provide (clustering of $form_state to support several subforms) will probably be a generic need for "embeddable forms".

fago’s picture

ad #77.1 Handling display component weights and extra fields is the business of the entity display form, so it should be handled in there.

Generally, the line between "entity form" and "entity display form" is still a bit blurry to me. I'd see the entity display form as a generic form matching the display config, while the entity form might include entity type specific additions without widgets or further customizations and alterations. Right?

Continued reviewing. Generally this looks great, I'm not fully done yet though.
Some more comments:

  1. +++ b/core/lib/Drupal/Core/Field/WidgetBaseInterface.php
    @@ -59,12 +61,14 @@ public function extractFormValues(FieldItemListInterface $items, array $form, ar
    +   * @param \Symfony\Component\Validator\ConstraintViolationListInterface|\Symfony\Component\Validator\ConstraintViolationInterface[] $violations
    +   *   The constraint violations that were detected.
    

    According to the type-hint it must be a ConstraintViolationList - what seems reasonable. So the other one should go.

  2. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
    @@ -148,6 +149,66 @@ public function getRenderer($field_name) {
    +    // @todo hook_field_attach_form() ?
    +    // We still need a hook to allow alteration of "forms with widgets", because
    +    // the regular "entity form" alter hook won't run non "non enity forms"...
    +    // Although, there's already hook_field_widget_form_alter() to alter each widget ?
    

    While this seems reasonable, what's the use-case one would go with this hook compared to the entity form hook?
    If it's just about having a generic hook, I guess it tells us we miss a generic entity form hook instead.

  3. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
    @@ -148,6 +149,66 @@ public function getRenderer($field_name) {
    +    // @todo hook_field_attach_extract_form_values() ? - See above.
    

    I don't see this hook as being required. If you do cross-widget alterations that have different value extraction needs than the widget I guess you'll break the widget value extraction anyway. So you'd have to ensure your form values are in place as usual for the widgets by making use of FAPI callbacks to alter the form values before widget value extraction runs.
    Altogether, this seems like an edge case that doesn't fit on the "form for the configured components", but would make sense of the potentially customized entity form.

  4. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
    @@ -148,6 +149,66 @@ public function getRenderer($field_name) {
    +        $violations = $items->validate();
    

    This would only create violations of constraints trigged by and assigned to the given field, but miss violations for this field but issues by cross-field validation of constraints put somewhere else. I think it would be better run validation on entity level and filter violations by field.
    This seems to be more an edge case though, so I'd be fine with improving it in a follow-up.

    Also, who is in charge of adding in violation errors on the root - the caller? If so, that needs to be documented.
    Maybe validateFormValues() could return the list of violations that are not covered by the displayed fields, so the caller can move on with them?

yched’s picture

Handling display component weights and extra fields is the business of the entity display form, so it should be handled in there

That's the case already.
- EntityFormDisplay::buildForm() assigns the weights of the elements it generates - i.e just fields atm.
- The form elements for extra fields are not present yet, they are added later (in [MyEntityFormController]::forrm() or hook_ENTITY_form_alter()), so we can't assign their weights yet.
The weights of extra fields is assigned in EntityFormController::processForm(). There is a @todo to make that step 'extra fields' only.

This is exaclty similar to what happens on the "view" side with EntityViewDisplay and EntityViewBuilder.

Generally, the line between "entity form" and "entity display form" is still a bit blurry to me. I'd see the entity display form as a generic form matching the display config, while the entity form might include entity type specific additions without widgets or further customizations and alterations. Right?

Right. The EntityFormDisplay handles the generic components that it holds the configuration of (widgets, contrib field groups...).
It does not produce a full form in itself (no form id, no submits...), but is used by other forms ("the entity form", edit.module's in place form) and cannot handle the elements that are generated directly on a specific form Class or form_id (i.e the "extra fields", for which it can only store weights and produce them when asked).

1. Yup, got fixed in #75
2. 3. Yes, last thursday on IRC it was decided to remove those hooks, but that was not done yet. Done now.
4.

This would only create violations of constraints trigged by and assigned to the given field

Yes, followup I think. That part is a 1:1 copy of what EntityFormDisplay::validateFormValues() does in HEAD. We only run field-by-field validate() in entity form submits currently.

Also, who is in charge of adding in violation errors on the root - the caller? If so, that needs to be documented.
Maybe validateFormValues() could return the list of violations that are not covered by the displayed fields, so the caller can move on with them?

Not sure of what you have in mind. EntityFormDisplay::validateFormValues() is only in charge of running validation and assigning errors for the form elements it generated. So, root errors or errors not covered by the displayed fields are out of its scope.

Updated patch: reroll + removal of the hooks.

yched’s picture

Side note : deleted the very stale
field_attach-remove-2095195
and field_attach-remove-2095195-service
branches from the sandbox,
and pushed the latest code to a (recreated) field_attach-remove-2095195 branch.

yched’s picture

Issue summary: View changes
yched’s picture

yched’s picture

Still pending the end of @fago's review - I'll be afk for the next two weeks, so I'll leave you guys drive this home :-)

cosmicdreams’s picture

Status: Needs review » Needs work

The last submitted patch, 80: field_attach-remove-2095195-80.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
75.5 KB

ok, finished reviewing. Sry about the delay.

The weights of extra fields is assigned in EntityFormController::processForm(). There is a @todo to make that step 'extra fields' only.

Yes, my point is that this should live in the display's processForm() then. Usually, the entity form controller forwards methods to the display form, so the display form can take care of its stuff. But here, the form controller does take care of stuff that does belong to the display form?
Instead it should go into EntityFormDisplay::processForm()

This is exaclty similar to what happens on the "view" side with EntityViewDisplay and EntityViewBuilder.

Yeah, seems like this is topic of #1875974: Abstract 'component type' specific code out of EntityDisplay. Finding a way to move it to the ViewDisplay as well would be good imo.

  public function buildForm(ContentEntityInterface $entity, array &$form, array &$form_state);

This should return the built form - just as FormInterface::buildForm().

Same way validateFormValues() should be named along FormInterface, i.e. validateForm(). As discussed in #66, #77, #78 we could implement something like an EmbeddableFormInterface later on. To ease that later on we should make sure the methods stay as much consistent as possible right now - so let's make them the same except for the additional $entity parameter now. Later on we can look into setting the $entity as property as we do for entity forms also.

    // Assign the weights configured in the form display.
    // @todo: Once https://drupal.org/node/1875974 provides the missing API,
    //   only do it for 'extra fields', since other components have been taken
    //   care of in EntityViewDisplay::buildMultiple().
    foreach ($form_display->getComponents() as $name => $options) {

Comment refers to EntityViewDisplay.

use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\entity\Entity\EntityFormDisplay;

/**
 * Base class for entity form controllers.
 */
class EntityFormController extends FormBase implements EntityFormControllerInterface {

Deprecated use statement.

class ContentEntityFormController extends EntityFormController implements ContentEntityFormControllerInterface {

As discussed in #30, #31 I'm not sure about introducing ContentEntityFormControllerInterface.

While this moves the field-attach logic into the display form, it also moves the display-form attach logic from EntityFormController to ContentEntityFormController. The latter doesn't seem really related to this issue and I'm not convinced it's the right thing to do.
Wouldn't it be simpler to have something like a NULL entity display form and keeping one flow of entity-form logic? Howsoever, I think this should be discussed in a separate issue.

This patch takes care of #2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements also, so marked it as duplicate.
Also, I was not able to find an issue for discussion of handling violations so I opened #2212013: Improve handling of entity validation violation errors to improve that.

I've attached a re-roll from #80 with fixed merge conflicts, no other changes.

fago’s picture

Thinking about it a bit more I don't think we should hard-bake entity displays to content entities either. Generating forms based on display components seems to be something useful for config entity forms as well, e.g. afaik we are already generating such forms for config entity translation. While those cannot use entity field widgets their own sort of widgets could be added in as display component.

martin107’s picture

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

HEAD is moving quickly. Patch no longer applies.

andyceo’s picture

Status: Needs work » Needs review

87: d8_form_remove.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 87: d8_form_remove.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
78.88 KB

Re-rolled, relatively small conflicts, moved a change from ConfigFieldItemList to FieldItemList as the code has been moved there and some code changed in field.attach.inc, re-deleted that file.

Status: Needs review » Needs work

The last submitted patch, 92: d8_form_remove-2095195-92.patch, failed testing.

yched’s picture

Thanks a lot @Berdir !
Patch reintroduces ConfigFieldItemList.php though :-)

Berdir’s picture

Status: Needs work » Needs review
FileSize
75.04 KB
4.85 KB

Yep I did. I'm also not sure how the following change got lost or what affected that but it seems to fix things, obviously :)

martin107’s picture

Issue tags: -Needs reroll
yched’s picture

Issue summary: View changes
pfrenssen’s picture

Patch still applies with a little fuzz in the book module.

pfrenssen’s picture

Rerolled against latest 8.x.

fago’s picture

FileSize
7.21 KB
77.02 KB

ok, I took a look at to address my remarks:
- Moved the processing logic of the entity display form to the entity display form
- Moved the buildEntity customizations into copyFormValuesToEntity(), as this is really what we want to override. There is no reason to differentiate the content entity form controller from the entity form controller here.
- Looked into unifying the buildForm,validateForm methods etc, but as there is no consistency between entity form controller and form interface methods we won't reach consistency here anyways - thus left that out for now. I guess we should fix the embeddable form interface first and then make sure it complies to that one (what ever it will look like).

Updated patch attached.

swentel’s picture

Status: Needs review » Reviewed & tested by the community
fago’s picture

FileSize
76.94 KB

Updated patch and fixed conflicts.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Couldn't find a change notice for this so I went ahead and made one: https://drupal.org/node/2225801

It looked like we'd forgotten to update the hook documentation for other hook_ENTITY_TYPE_prepare_form() implementations, but we don't have those separately documented, so can't update what isn't there.

Patch itself looks great to me. Committed/pushed to 8.x, thanks!

  • Commit 83b797d on 8.x by catch:
    Issue #2095195 by yched, fago, Berdir, amateescu, swentel, pfrenssen,...
yched’s picture

Yay @ the commit !

Mostly agreed with the latest changes from @fago, just :

1)
- EFController::form() adds EFController::processForm() as a #process callback
- EFDisplay::buildForm() also adds its own EFDisplay::processForm() as an additional #process callback, which assigns weights and sets #access on extra fields.

One consequence is that the EFD gets serialized as part of $form, while I think it was "only" serialized as part of $form_state so far.

Maybe ContentEFController::processForm() could take care of calling EFDisplay::processForm() directly without having an additional #process ? ContentEFController is already built on a pattern of calling EFDisplay methods at various points in its lifecycle...

2) On the "view" side, assigning weights is still split between EVDisplay::buildMultiple() and EVBuilder::viewMultiple() for extra fields (that are not present yet when the previous step runs)

For consistency, could we similarly move all of this to a single #pre_render callback ?

fago’s picture

One consequence is that the EFD gets serialized as part of $form, while I think it was "only" serialized as part of $form_state so far.

Maybe ContentEFController::processForm() could take care of calling EFDisplay::processForm() directly without having an additional #process ? ContentEFController is already built on a pattern of calling EFDisplay methods at various points in its lifecycle...

I was thinking of doing that initially, but processForm() is not part of a form interface and thus it probably won't be part of an embeddable form interface either - so I thought it's better if the EFDisplay takes care of registering itself.

Next, the remaining EntityFormController only does the re-freshing of $this->entity what seems to be hack that (as I'd assume) is necessary only because of the EFC being serialized to both $form and $form_state too :/. I was thinking serializing $form and $form_state together could easily solve that problem generally for us - but I'm not sure whether we already have an issue for cleaning that up? I've not been able to find one.

#105.2 seems totally reasonable.

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Ouch, I missed that this removed hook_field_attach_form(). That will hurt, as it was a nice pairing for hook_field_extra_fields() in D7. I honestly have no idea how to modernize that code now, and the change notice (https://drupal.org/node/2225801) is absolutely zero help. It just says "Use the Entity API instead", as though that will impart some information.