Problem/Motivation

Entity forms where classes before the other forms were. This has been gradually merged together again, but entity forms are in some cases still special. One example is $form_state['controller'], which is now the same object as $form_state['build_info']['callback_object'].

It still hints at entity forms being controllers, but they were renamed to just entity forms and controllers re renamed in general to handlers: #1976158: Rename entity storage/list/form/render "controllers" to handlers.

The reason this object is important is that it's the API for entity form alter hooks to get the entity that is being edited: $form_state['controller']->getEntity().

Proposed resolution

We have the FormState object now, so it would be nice to have a single method for this object, something like:

FormStateInterface::getFormObject()? or just getObject() ?

Then you would do: $form_state->getFormObject()->getEntity()*

* Note that getEntity() would be specific to entity forms, and type hinting would not work for it, but it's better than what is available now.

Remaining tasks

Decide on method name, implement it.

User interface changes

API changes

$form_state['controller'] will go away, $form_state['build_info']['callback_object'] will probably still work. We could even consider to keep support for $form_state['controller'] if not having an API change is more important than getting rid of "entity controller". But I think not :)

CommentFileSizeAuthor
#17 form-state-controller-2313931-17-interdiff.txt763 bytesBerdir
#17 form-state-controller-2313931-17.patch25.6 KBBerdir
#17 form-state-controller-2313931-17.patch25.6 KBBerdir
#14 form-state-controller-2313931-14-interdiff.txt10.07 KBBerdir
#14 form-state-controller-2313931-14.patch25.6 KBBerdir
#12 form-state-controller-2313931-12-interdiff.txt1.17 KBBerdir
#12 form-state-controller-2313931-12.patch16.43 KBBerdir
#10 form-state-controller-2313931-10.patch14.55 KBBerdir
#6 form-state-controller-2313931-5.patch15.02 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Unify $form_state['controller'] with $form_state['build_info']['callback_object'] and add a method for it. » Merge $form_state['controller'] with $form_state['build_info']['callback_object'] and add a method for it.
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Both $form_state['controller'] and $form_state['build_info']['callback_object'] are new to D8, so I don't think we have to worry about BC there.

I vote for getFormObject().

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

getFormObject() sounds like what we should name the getter on FormState that gets $form when we convert it to an object in D9. ;) So I think that would confuse.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Version: 8.x-dev » 8.0.x-dev
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I don't think we should worry about 9.x here, that is very very far away and there are only few reasons to use it, so if that would really happen (and I'm not sure $form/render arrays will become render objects in 9.x) we can just rename this.

I'm +1 on getFormObject() too.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
form-state-controller-2313931-5.patch15.02 KB

Here is a first patch that removes $form_state['controller']. This will likely fail without #2309323: Allow #submit and #validate to be specified as methods of the form object, as that removes the need for the hacky re-assignment of $form_state['controller'] that I removed here.

Status: Needs review » Needs work

The last submitted patch, 6: form-state-controller-2313931-5.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

I still think getFormObject() sounds like it would get a... form... object. The bit about D9 was a joke; I just think that the name doesn't explain what it does. It's more like a getObjectThisFormIsEditing() but that's not exactly a good name either so I guess disregard my opinion if everyone else thinks that's a good name. :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ah, maybe there is a small misunderstanding here :) This is not "getObjectThisFormIsEditing()", it returns the object that is defining the form. getFormObject()->getEntity() would be getObjectThisFormIsEditing(). (Except that this concept doesn't exist outside of entity forms, as a single form can be about no or many objects).

We have no other name for whatever implements FormInterface. The class docblock for FormInterface is just "Provides an interface for a Form.". and getForm() would be more misleading than getFormObject() IMHO.

We already use getFormObject() in some cases, see FormController::getFormObject() as an example. So I think this is as consistent with HEAD as it can get.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
form-state-controller-2313931-10.patch14.55 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 10: form-state-controller-2313931-10.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
form-state-controller-2313931-12.patch16.43 KB
form-state-controller-2313931-12-interdiff.txt1.17 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
@@ -7,6 +7,7 @@
+use Drupal\Core\Annotation\Action;

    ?! 

  2. +++ b/core/modules/content_translation/content_translation.module
@@ -347,7 +347,7 @@ function content_translation_controller($entity_type_id) {
 function content_translation_form_controller(FormStateInterface $form_state) {
-  return isset($form_state['controller']) && $form_state['controller'] instanceof EntityFormInterface ? $form_state['controller'] : FALSE;
+  return $form_state->getFormObject() instanceof EntityFormInterface ? $form_state->getFormObject() : FALSE;

    This seems like a useless function.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
form-state-controller-2313931-14.patch25.6 KB
form-state-controller-2313931-14-interdiff.txt10.07 KB

Yep, removed that function, only problem is that we lose the right type hint for getEntity()/getFormLangcode(), which is the only thing it is used for.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Core/Entity/EntityForm.php
@@ -115,9 +115,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-    // Add the form object to the form state so it can be easily accessed by
-    // module-provided form handlers there.
-    $form_state['controller'] = $this;
+    // Flag that this form has been initialized.
+    $form_state['entity_form_initialized'] = TRUE;

Just wanted to say once again: Yay!

Thanks for addressing my feedback.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: form-state-controller-2313931-14.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
form-state-controller-2313931-17.patch25.6 KB
form-state-controller-2313931-17.patch25.6 KB
form-state-controller-2313931-17-interdiff.txt763 bytes

Mixed up $form_state/$form_object.

Status: Needs review » Needs work

The last submitted patch, 17: form-state-controller-2313931-17.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community
webchick’s picture

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

Nice clean-up.

Committed and pushed to 8.x. Thanks!

  • webchick committed c7013bd on 8.0.x 
    Issue #2313931 by Berdir: Merge ['controller'] with ['build_info']['...

Status: Fixed » Closed (fixed)

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