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, containing protected function t() as a helper method
  • \Drupal\Core\Entity\EntityFormControllerInterface has a public 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 use EntityControllerInterface, they should be like all other forms and use ControllerInterface
  • \Drupal\image\Form\ImageStyleFormBase had its $this->translator property changed to $this->translationManager to match the rest of core

#2049159: Create a ControllerBase class to stop the boilerplate code madness

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
25.83 KB

Here's a start.

tim.plunkett’s picture

Issue tags: +API change
FileSize
47.96 KB
73.79 KB

Here'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.

tim.plunkett’s picture

I have a couple stale use statements in there, but I'd much rather have some actual feedback (and wait for the testbot) before rerolling.

tim.plunkett’s picture

Missed ControllerInterface!

tim.plunkett’s picture

Missed a spot (or three)

Crell’s picture

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -0,0 +1,75 @@
+abstract class FormBase implements FormInterface, ControllerInterface {

You'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?

tim.plunkett’s picture

I 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.

fubhy’s picture

It'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.

tim.plunkett’s picture

We 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.

tim.plunkett’s picture

Note, this will make it way easier to do #2004282: Injected dependencies and serialization hell

tim.plunkett’s picture

#7: formbase-2059245-7.patch queued for re-testing.

andypost’s picture

This 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 too

Also I think this change needs profiling because most of forms with the change will make more calls to parent::__construct()

tim.plunkett’s picture

We'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.

msonnabaum’s picture

Profiling 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.

msonnabaum’s picture

FileSize
81.72 KB

Here'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).

msonnabaum’s picture

FileSize
5.92 KB

And interdiff.

Crell’s picture

Addendum 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.

tim.plunkett’s picture

FileSize
35.38 KB
114.54 KB

This 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.

benjy’s picture

Yeah +1 for #22.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

These failures were introduced in @msonnabaum's patch, going to debug that.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
130.16 KB
36.6 KB

Here's some fixes, but also includes setRequest/getRequest/getCurrentUser as discussed.
This rounds out the methods I'd like to see.

tim.plunkett’s picture

#26: formbase-2059245-26.patch queued for re-testing.

tim.plunkett’s picture

@msonnabaum points out some of these should have been protected.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.php
    @@ -94,8 +86,8 @@ protected function actions(array $form, array &$form_state) {
    +      $options = drupal_parse_url($this->getRequest()->query->get('destination'));
    
    +++ b/core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php
    @@ -94,8 +85,8 @@ protected function actions(array $form, array &$form_state) {
    +      $options = drupal_parse_url($this->getRequest()->query->get('destination'));
    
    +++ b/core/lib/Drupal/Core/Form/ConfirmFormBase.php
    @@ -45,11 +43,11 @@ public function getFormName() {
    +      $options = drupal_parse_url($this->getRequest()->query->get('destination'));
    

    Let's open a follow up to convert this to Url::parse()

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -230,12 +230,17 @@ public function getFormController($entity_type, $operation) {
    +        ->setTranslationManager($this->container->get('string_translation'))
    +        ->setModuleHandler($this->container->get('module_handler'))
    

    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.

  3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
    @@ -45,13 +44,14 @@ class MenuLinkFormController extends EntityFormController implements EntityContr
    +   * @param \Drupal\Core\Routing\UrlGenerator $url_generator
    ...
    +  public function __construct(MenuLinkStorageControllerInterface $menu_link_storage_controller, AliasManagerInterface $path_alias_manager, UrlGenerator $url_generator) {
    

    Wait, why do we not use the interface here?

  4. +++ b/core/modules/statistics/lib/Drupal/statistics/StatisticsSettingsForm.php
    @@ -28,11 +29,14 @@ class StatisticsSettingsForm extends SystemConfigFormBase {
        * @param \Drupal\Core\Extension\ModuleHandler $module_handler
        *   The module handler.
    ...
    +  public function __construct(ConfigFactory $config_factory, ContextInterface $context, ModuleHandler $module_handler) {
    

    Could we just switch to the moduleHandlerInterface while we are changing this line anyway?

Crell’s picture

I didn't review the whole patch, but FormBase looks reasonable and is what msonnabaum and I had discussed. One nit:

@@ -0,0 +1,131 @@
+    return new static;

I think convention has static();

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.phpundefined
@@ -94,8 +86,8 @@ protected function actions(array $form, array &$form_state) {
+    if ($this->getRequest()->query->has('destination')) {
+      $options = drupal_parse_url($this->getRequest()->query->get('destination'));

+++ b/core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.phpundefined
@@ -94,8 +85,8 @@ protected function actions(array $form, array &$form_state) {
+    if ($this->getRequest()->query->has('destination')) {
+      $options = drupal_parse_url($this->getRequest()->query->get('destination'));

+++ b/core/lib/Drupal/Core/Form/ConfirmFormBase.phpundefined
@@ -45,11 +43,11 @@ public function getFormName() {
+    if ($this->getRequest()->query->has('destination')) {
+      $options = drupal_parse_url($this->getRequest()->query->get('destination'));

maybe we could save a getRequest() call here?

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
@@ -15,7 +17,7 @@
+class EntityFormController extends FormBase implements EntityFormControllerInterface {

@@ -566,4 +541,21 @@ public function setFormDisplay(EntityFormDisplayInterface $form_display, array &
+  public function setTranslationManager(TranslatorInterface $translation_manager) {
+    $this->translationManager = $translation_manager;
+    return $this;
+  }

dont we already have this method in FormBase? why duplicate ? as far as i can see they are the same

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -230,12 +230,17 @@ public function getFormController($entity_type, $operation) {
+        ->setTranslationManager($this->container->get('string_translation'))
+        ->setModuleHandler($this->container->get('module_handler'))
+        ->setOperation($operation);

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

+++ b/core/lib/Drupal/Core/Form/FormBase.phpundefined
@@ -0,0 +1,131 @@
+    return $this->translator()->translate($string, $args, $options);
...
+   * Gets the translator object.
...
+  protected function translator() {

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;)

+++ b/core/lib/Drupal/Core/Form/FormBase.phpundefined
@@ -0,0 +1,131 @@
+  public function setTranslationManager(TranslatorInterface $translation_manager) {

this typehint is wrong. it should either typehinted with TranslationManager or nothing. Translators dont have translate() method

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.phpundefined
@@ -47,8 +50,9 @@ class SettingsForm extends SystemConfigFormBase {
-  public function __construct(ConfigFactory $config_factory, AggregatorPluginManager $fetcher_manager, AggregatorPluginManager $parser_manager, AggregatorPluginManager $processor_manager) {
-    $this->configFactory = $config_factory;
+  public function __construct(ConfigFactory $config_factory, ContextInterface $context, AggregatorPluginManager $fetcher_manager, AggregatorPluginManager $parser_manager, AggregatorPluginManager $processor_manager) {
+    parent::__construct($config_factory, $context);

thanks for fixing this

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.86 KB
34.22 KB
146.67 KB

This 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.

tim.plunkett’s picture

Issue summary: View changes

Updated

Status: Needs review » Needs work

The last submitted patch, formbase-2059245-33.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
145.65 KB
2.96 KB

Stupid mistakes.

tim.plunkett’s picture

FileSize
142.63 KB

Blocker went in.

57 files changed, 495 insertions(+), 678 deletions(-)

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -0,0 +1,130 @@
    +    return $this->getRequest()->attributes->get('_account');
    

    This 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?

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Config\Context\ContextInterface;
    

    Notes for further reviewers: These were added because the original form was wrong

  3. +++ b/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php
    @@ -24,43 +23,6 @@ class EntityDisplayModeAddForm extends EntityDisplayModeFormBase {
    -      $container->get('entity.query'),
    

    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

  1. Manual install.
  2. Created and deleted a node.
  3. Created and deleted a term.
  4. Created and deleted a custom block.
  5. Created three terms, then re-ordered, then reset to alpha.
  6. Created and deleted a view

I think this is good to go

tim.plunkett’s picture

I don't think getCurrentUser needs a @todo. Either that issue will replace _account or it won't, this is encapsulated nicely.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Yep, now for API change approval.

ParisLiakos’s picture

This looks sexy to me:)

Just one really minor thing

+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.phpundefined
@@ -46,16 +44,12 @@
-  function __construct(ModuleHandlerInterface $module_handler, QueryFactory $query_factory, Date $date_service) {
...
+  function __construct(QueryFactory $query_factory, Date $date_service) {

I wont make you reroll 142kb for this:P
if it gets rerolled maybe quickly add public in front of it

RTBC to me

ParisLiakos’s picture

oh, larowlan was faster:P
+1

webchick’s picture

Issue tags: +Approved API change

Alex 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.

effulgentsia’s picture

Priority: Normal » Major

#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.

alexpott’s picture

Title: Add a FormBase class containing useful methods » Change notice: Add a FormBase class containing useful methods
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Let's get this done.

Committed 410d56a and pushed to 8.x. Thanks!

I think we might need to update a few change notices.

diff --git a/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php b/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php
index cf0854e..6eb2d2f 100644
--- a/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php
+++ b/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php
@@ -7,9 +7,6 @@

 namespace Drupal\entity\Form;

-use Drupal\Component\Plugin\PluginManagerInterface;
-use Drupal\Core\Entity\Query\QueryFactory;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

 /**
diff --git a/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
index 98fc24b..e04a610 100644
--- a/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
+++ b/core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php
@@ -49,7 +49,7 @@
    * @param \Drupal\Core\Datetime\Date $date_service
    *   The date service.
    */
-  function __construct(QueryFactory $query_factory, Date $date_service) {
+  public function __construct(QueryFactory $query_factory, Date $date_service) {
     $date = new DrupalDateTime();
     $this->patternType = $date->canUseIntl() ? DrupalDateTime::INTL : DrupalDateTime::PHP;

Picked up some very minor stuff during review. Fixed during commit - thanks Paris!

smiletrl’s picture

Status: Active » Needs review
Issue tags: -Needs change record

Add a change record https://drupal.org/node/2076011, which is based on the issue summary.

jibran’s picture

Title: Change notice: Add a FormBase class containing useful methods » Add a FormBase class containing useful methods
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: +FormBase

Added #2072251: [meta] Modernize forms to use FormBase to change notice also added getCurrentUser() method. I think change notice looks great.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

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

Anonymous’s picture

Issue summary: View changes

updated