Closed (fixed)
Project:
Drupal core
Version:
8.1.x-dev
Component:
forms system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Feb 2016 at 06:05 UTC
Updated:
1 Jul 2016 at 10:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tstoecklerSo this was intentionally done so that sub-classes could more easily use
ContainerInjectionInterface.I don't personally agree with that, but that was the reason at the time.
Comment #3
dawehnerYeah, it was always a reason purely driven by TX, IMHO.
In general we cannot inject anything for those base classes, because this would break a hell lot of implementations out there
Comment #4
tim.plunkettWe can inject it, but only if we still fall back.
IMO this will confuse people about whether they need to call
parent::__construct()(they don't have to)I believe we should trait-ify the remaining service wrappers in FormBase and be done with it.
Honestly the only reason I extend FormBase is to skip an empty validateForm(), which is a terrible reason, but so it goes.
Comment #5
mile23Yup.
The traits are neeto, but for instance
StringTranslationTraitallows me to take a separate step tosetStringTranslation()in my factory method, but thengetNumberOfPlurals()ignores injected services anyway. That's out of scope here, but yah.Also the other traits on
FormBasehave the same problem: If there's no injected service they use\Drupalwhen they should really complain.So then the basic idea of
FormBaseis a little broken. All it gives you is the knowledge that you should implementFormInterfaceand then holds your hand if you did a bad job of dependency injection.As it stands, it's a very poor example of how to implement
ContainerInjectionInterface, because the router is going to hand you a container, which you ignore and then infer from\Drupalbecause it works.In that case, let's document that in the docblock.
Comment #7
daffie commentedI f we want to do container injection the correct way we are making an API change and breaking backwards compatibility. So we cannot do that. Want we can do is adding a waning how container injection should be done. And the current patch has that waning. For me this is RTBC.
Comment #8
tim.plunkettThis issue needs a new title before commit.
Comment #9
daffie commented@tim.plukett: You are right, thanks.
Comment #10
mile23It's a doc change so 8.1.x.
I saw my own typo, so I fixed it, and then changed the docblock a bit. Here's the new one, easier to read here than in the patch:
Comment #11
mile23Comment #12
daffie commentedSo is the warning text even better.
Comment #14
mile23Setting back to RTBC after spurious testbot fail.
Comment #15
alexpottThat's a lot of commas. How about:
Alternately, do not use FormBase. A class can implement FormInterface, use the traits it needs, and inject services from the container as required.Comment #16
mile23Changes per #15.
Comment #17
daffie commentedAlexpott got what he wished for, so back to RTBC.
Comment #18
alexpottCommitted 2ca66f6 and pushed to 8.1.x and 8.2.x. Thanks!