Problem/Motivation

Part of #1976158: Rename entity storage/list/form/render "controllers" to handlers.

Proposed resolution

- Rename EntityFormController including all related subclasses and interfaces to EntityForm.
- Rename EntityManager::getFormController() to EntityManager::getFormObject(), to explain the difference to getForm() of FormBuilder and EntityFormBuilder.

Remaining tasks

User interface changes

API changes

Classes and methods renamed as listed above.

Comments

berdir’s picture

Status: Active » Needs review
StatusFileSize
new126.19 KB

This does the following:

- Rename all *FormController classes and interfaces to *Form
- Renames getFormController() to getForm() on EntityManager
- Renames text references like "form controller" to just form.

Exceptions:
- There are a bunch of FormController classes that are actually controllers that then display a form.
- There are a few controller classes referenced to as "Temporary form controller", which means that are temporary wrappers that call to the FormBuilder for an old-style function based form.

Let's see what I missed. This was much less worse than I feared because, while we have a lot of form controller classes, we have almost no references to getForm() and those classes outside of the entity annotations.

amateescu’s picture

- Renames getFormController() to getForm() on EntityManager

Umm.. this is not confusing at all :)

berdir’s picture

Yes, @timplunkett also said that in the parent issue. EntityFormInterface implements FormInterface...

We have \Drupal::formBuilder()->getForm() to which you can pass an object that implements FormInterface (which would imply that an implementation of that interface is a "form"?), you get the form from a form? So we really already have that double meaning of "Form"?

And we have EntityFormBuilder::getForm(), to which you pass an entity and an operation and it gives you the form, using EntityManager::getFormController() internally, and yes, changing that to getForm() as well is weird.

Looking at the documentation of FormBuilder.:getForm():

   * Gets a renderable form array.
   *
   * @param \Drupal\Core\Form\FormInterface|string $form_arg
   *   The value must be one of the following:
   *   - The name of a class that implements \Drupal\Core\Form\FormInterface.
   *   - An instance of a class that implements \Drupal\Core\Form\FormInterface.
   *   - The name of a function that builds the form.

I would seriously argue that it's rather that method that is misnamed? :) Because that returns the result of buildForm(), and how can a method getForm() that returns an object implemting FormInterface be misnamed? :)

Anyway, I guess we can't change that, so I'm open to ideas? Note that there's almost zero usage of that method...

yched’s picture

We have \Drupal::formBuilder()->getForm() to which you can pass an object
that implements FormInterface (which would imply that an implementation of
that interface is a "form"?), you get the form from a form?

THIS :-(

effulgentsia’s picture

+1 to this rename, despite the following:

Note that there's almost zero usage of that method...

Yeah, given how little code interacts directly with FormBuilder, I think we can even rename those methods after beta if we really needed to. The bigger concern is that there are lots of functions/methods that receive $form and $form_state as parameters, and in all cases, $form is the render array, not the instance of a *Form class. Also, the *Form classes implement FormInterface, which has a buildForm() method, so we ask each "form" object to "build a form", by which we mean, the "form" render array. Kind of sucks, but it reflects the reality that we just didn't have time in D8 to upgrade FAPI to OO, so our forms have a dual existence of object and array.

andypost’s picture

Suppose the getForm() method should be changed to accept form display somehow
because passing an operation is not enough now!
Each form could be rendered in different configurable or default display object

berdir’s picture

StatusFileSize
new126.33 KB

Re-rolled.

@andypost: That has nothing to do with this rename.

Discussed this a bit with @timplunkett as well, looks like everyone is onboard, can we move forward?

tim.plunkett’s picture

Honestly the EntityFormBuilderInterface::getForm and EntityManagerInterface::getForm naming class is still confusing to me.
If everyone else is okay with it, that's fine. But we could also use getFormObject or something...

effulgentsia’s picture

+1 to changing EntityManagerInterface::getForm() to getFormObject().

berdir’s picture

getFormObject() it is. Verified that unit test coverage annotations are still correct (there are bugs but not for entity manager.

berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 10: entity-form-controller-rename-2238077-10.patch, failed testing.

berdir’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
StatusFileSize
new127.04 KB
new4.09 KB

Updated https://drupal.org/node/2200867, re-roll, issue summary written.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new127.08 KB

A use statement addition in #2244775: Declare a maximum length for contact category machine name conflicted, no other changes.

I reviewed this with git diff --color-words, everything looks good. Thanks @Berdir!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: entity-form-2238077-14.patch, failed testing.

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 16: 2238077.15.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new128.15 KB
new1.88 KB
berdir’s picture

Forgot the comment. Did a re-roll too, and when comparing the files, noticed a few mistakes both in my re-roll and the one from @alexpott, fixed both, see manual interdiff.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

Came back green, back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c79ac2f and pushed to 8.x. Thanks!

  • Commit c79ac2f on 8.x by alexpott:
    Issue #2238077 by Berdir, alexpott, tim.plunkett: Rename...

Status: Fixed » Closed (fixed)

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