Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Updated: Comment #33
Problem/Motivation
Forms often need boilerplate code, such as the translation manager or an empty validateForm() method.
Proposed resolution
Provide a base class with a set of standard services and helper methods:
The 'string_translation' service is added with a protected function t()
protected function getRequest()
protected function getCurrentUser()
Remaining tasks
Review
User interface changes
N/A
API changes
API additions
\Drupal\Core\Form\FormBase
is added, containingprotected function t()
as a helper method\Drupal\Core\Entity\EntityFormControllerInterface
has apublic function setTranslationManager()
, which is called automatically to prevent constructor changes
API changes
- While in route based forms, the request can still be retrieved in buildForm(), it is unreliable.
$this->getRequest()
should be used - Entity forms (
EntityFormControllerInterface
) can no longer useEntityControllerInterface
, they should be like all other forms and useControllerInterface
\Drupal\image\Form\ImageStyleFormBase
had its$this->translator
property changed to$this->translationManager
to match the rest of core
Related Issues
#2049159: Create a ControllerBase class to stop the boilerplate code madness
Comment | File | Size | Author |
---|---|---|---|
#36 | formbase-2059245-36.patch | 142.63 KB | tim.plunkett |
#35 | interdiff.txt | 2.96 KB | tim.plunkett |
#35 | formbase-2059245-35.patch | 145.65 KB | tim.plunkett |
#33 | formbase-2059245-33.patch | 146.67 KB | tim.plunkett |
#33 | interdiff-mychanges.txt | 34.22 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere's a start.
Comment #2
tim.plunkettHere's the rest.
This does NOT change t() everywhere in every class it can.
It does however remove any unnecessary usage of $this->translator->translate(), and uses the new $this->t() once for each of the major base classes.
Ideally we would make this change as soon as possible, and then have a single follow-up issue to change t() to $this->t() in all Forms.
Comment #3
tim.plunkettI have a couple stale
use
statements in there, but I'd much rather have some actual feedback (and wait for the testbot) before rerolling.Comment #5
tim.plunkettMissed ControllerInterface!
Comment #7
tim.plunkettMissed a spot (or three)
Comment #8
Crell CreditAttribution: Crell commentedYou'll hate me for this, but... should this be called FormControllerBase, since it's implementing ControllerInterface as well?
(Which will be renamed soon, too.)
Are there any other services that seem to be super-common in forms? ConfigFactory, maybe?
Comment #9
tim.plunkettI don't think FormControllerBase makes any more sense. Every form will need translation, there's no reason not to extend FormBase or one of its children.
SystemConfigFormBase already exists for things that need ConfigFactory.
Comment #10
fubhy CreditAttribution: fubhy commentedIt's a pity that we can't use Traits as that would instantly resolve all the problems.
Anyways, I think we should do this like we did with ControllerBase. It's much simpler to extend it if you don't have to care about the base dependencies because they are just pulled from the container on demand. Also, that allows us to add a lot more base things (like EntityManager, etc.) without any overhead.
In fact, why don't we just extend from ControllerBase for this? After all, forms are basically controllers. Let's just add a FormBase that extends from ControllerBase and implements FormInterface. That should do it imho.
Comment #12
tim.plunkettWe don't want to extend ControllerBase, because that is for "thin" controllers. They have a couple common services, and then hand off to some other object.
Whereas a form IS the object, it's not really going anywhere. So it will need additional constructor injection, and isn't ContainerAware.
Comment #13
tim.plunkettNote, this will make it way easier to do #2004282: Injected dependencies and serialization hell
Comment #14
tim.plunkett#7: formbase-2059245-7.patch queued for re-testing.
Comment #15
andypostThis change is really needed but will require re-roll to inject
current_user
from #2062151: Create a current user service to ensure that current account is always available tooAlso I think this change needs profiling because most of forms with the change will make more calls to
parent::__construct()
Comment #16
tim.plunkettWe're not profiling because of 1 call each to parent::__construct(). That's insane. It's part of the language, and it is required.
It doesn't matter which one goes first, that doesn't yet remove _account.
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedProfiling for those parent calls is totally unnecessary.
That said, needing those parent calls is pretty goofy imo. If you're going to have a base class that provides methods like t(), it should just get it from \Drupal unless $this->translator has already been set either in the child classes' constructor or a setter method. Trying to do DI with inheritance like this is just kind of a mess.
Comment #18
msonnabaum CreditAttribution: msonnabaum commentedHere's a version that addresses my concerns in #17. It just passes through to the \Drupal class for dependencies rather than requiring them to be injected. However, that call can still be bypassed in tests by calling the setter, or simply setting the property it in the constructor.
Since this is violating the typical advice we give to never use the \Drupal class in a class, I talked to Larry about this here at MWDS to come up with some guidelines around when this is ok.
The exception is when you are a) a base class, b) exist at the application layer (controllers, forms, possibly plugins), and c) provide service dependencies that are so common among that type of object that the majority would require them to be injected (translation manager, url generator, request, etc).
Comment #19
msonnabaum CreditAttribution: msonnabaum commentedAnd interdiff.
Comment #20
Crell CreditAttribution: Crell commentedAddendum to #18 that Mark and I discussed: The base class's access methods must have corresponding setter methods for setter injection during testing.
In a just world we wouldn't have a base class but use traits here (with the same rules), but sadly we're not living in a just world.
Comment #22
tim.plunkettThis would make #2069395: EntityFormController implements createInstance() for no reason obsolete. This came after discussion with @larowlan.
module_handler should never have been handled by constructor injection, especially since we have EntityManager::getFormController.
Comment #23
benjy CreditAttribution: benjy commentedYeah +1 for #22.
Comment #25
tim.plunkettThese failures were introduced in @msonnabaum's patch, going to debug that.
Comment #26
tim.plunkettHere's some fixes, but also includes setRequest/getRequest/getCurrentUser as discussed.
This rounds out the methods I'd like to see.
Comment #28
tim.plunkett#26: formbase-2059245-26.patch queued for re-testing.
Comment #29
tim.plunkett@msonnabaum points out some of these should have been protected.
Comment #30
dawehnerLet's open a follow up to convert this to Url::parse()
In general I prefer to explicit inject dependencies. Yes we do already have the full container available, though this adds some additional documentation info and makes it potentially easier to unit test.
Wait, why do we not use the interface here?
Could we just switch to the moduleHandlerInterface while we are changing this line anyway?
Comment #31
Crell CreditAttribution: Crell commentedI didn't review the whole patch, but FormBase looks reasonable and is what msonnabaum and I had discussed. One nit:
I think convention has static();
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedmaybe we could save a getRequest() call here?
dont we already have this method in FormBase? why duplicate ? as far as i can see they are the same
while this is great DX wise, we still have setters for non-optional dependencies..which might be a pain for new people not familiar with it..but i cant find an alternate way:P
i would say getTranslationManager() or one could confuse it with the translators that are being managed by the manager..it also reflects the property name;)
this typehint is wrong. it should either typehinted with TranslationManager or nothing. Translators dont have translate() method
thanks for fixing this
Comment #33
tim.plunkettThis is now blocked on #2049709: TranslationManager::translate() should be on an interface, and this patch includes that patch.
I'm attaching two interdiffs, one of changes I made myself, and the other addressing the reviews.
Comment #33.0
tim.plunkettUpdated
Comment #35
tim.plunkettStupid mistakes.
Comment #36
tim.plunkettBlocker went in.
57 files changed, 495 insertions(+), 678 deletions(-)
Comment #37
larowlanThis will change if #2062151: Create a current user service to ensure that current account is always available goes in first - but does it warrant an @todo in case this goes in first?
Notes for further reviewers: These were added because the original form was wrong
Notes for further reviewers: This went because injected in the parent already
Do we need to add the last two of these to the issue summary?
Should the issue summary mention the other setter methods in addition to setTranslationManager?
I think the issue summary should reference #2018411-32: Figure out a nice DX when working with injected translation insofar as $this->t() is the accepted approach for l.d.o.
Also manually tested as follows
I think this is good to go
Comment #38
tim.plunkettI don't think getCurrentUser needs a @todo. Either that issue will replace _account or it won't, this is encapsulated nicely.
Comment #39
larowlanYep, now for API change approval.
Comment #40
ParisLiakos CreditAttribution: ParisLiakos commentedThis looks sexy to me:)
Just one really minor thing
I wont make you reroll 142kb for this:P
if it gets rerolled maybe quickly add public in front of it
RTBC to me
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedoh, larowlan was faster:P
+1
Comment #42
webchickAlex walked me through this, based on what I've seen there's a number of important DX improvements here (e.g. $this->t() instead of $this->blahaldsadjlsajdlsad(), and removing a bunch of boilerplate code in places like a/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php. I feel comfortable tagging this as an "Approved API change," given we just did something similar for ControllerBase, so tagging. But I'm not sure I'm comfortable committing it since I'd need a bit more time to go through it, so leaving for catch/alex. I also have a few concerns with the overall "DI for DI's sake" movement here, and I'll try and post an issue about it later.
Comment #43
effulgentsia CreditAttribution: effulgentsia commented#1999346: Use Symfony Request for fieldui module and possibly other WSCII-conversion issues are soft blocked on this (they can proceed without this in theory, but reviews like #1999346-25: Use Symfony Request for fieldui module slow them down). Since every WSCII-conversion issue is in fact a critical (as children of #1971384: [META] Convert page callbacks to controllers), I think that makes this issue at least a major.
Comment #44
alexpottLet's get this done.
Committed 410d56a and pushed to 8.x. Thanks!
I think we might need to update a few change notices.
Picked up some very minor stuff during review. Fixed during commit - thanks Paris!
Comment #45
smiletrl CreditAttribution: smiletrl commentedAdd a change record https://drupal.org/node/2076011, which is based on the issue summary.
Comment #46
jibranAdded #2072251: [meta] Modernize forms to use FormBase to change notice also added
getCurrentUser()
method. I think change notice looks great.Comment #47
tim.plunkettComment #48.0
(not verified) CreditAttribution: commentedupdated