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.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | entity-form-2238077-17-interdiff.txt | 1.88 KB | berdir |
| #18 | entity-form-2238077-17.patch | 128.15 KB | berdir |
| #16 | 2238077.15.patch | 127.4 KB | alexpott |
| #14 | entity-form-2238077-14.patch | 127.08 KB | tim.plunkett |
Comments
Comment #1
berdirThis 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.
Comment #2
amateescu commentedUmm.. this is not confusing at all :)
Comment #3
berdirYes, @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():
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...
Comment #4
yched commentedTHIS :-(
Comment #5
effulgentsia commented+1 to this rename, despite the following:
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.
Comment #6
andypostSuppose the
getForm()method should be changed to accept form display somehowbecause passing an operation is not enough now!
Each form could be rendered in different configurable or default display object
Comment #7
berdirRe-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?
Comment #8
tim.plunkettHonestly 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
getFormObjector something...Comment #9
effulgentsia commented+1 to changing EntityManagerInterface::getForm() to getFormObject().
Comment #10
berdirgetFormObject() it is. Verified that unit test coverage annotations are still correct (there are bugs but not for entity manager.
Comment #11
berdir10: entity-form-controller-rename-2238077-10.patch queued for re-testing.
Comment #13
berdirUpdated https://drupal.org/node/2200867, re-roll, issue summary written.
Comment #14
tim.plunkettA 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!
Comment #16
alexpottRerolled due to #216064: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link
Comment #18
berdirComment #19
berdirForgot 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.
Comment #20
berdirCame back green, back to RTBC.
Comment #21
alexpottCommitted c79ac2f and pushed to 8.x. Thanks!