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

Files: 
CommentFileSizeAuthor
#36 formbase-2059245-36.patch142.63 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]
#35 interdiff.txt2.96 KBtim.plunkett
#35 formbase-2059245-35.patch145.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,158 pass(es).
[ View ]
#33 formbase-2059245-33.patch146.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,949 pass(es), 45 fail(s), and 52 exception(s).
[ View ]
#33 interdiff-mychanges.txt34.22 KBtim.plunkett
#33 interdiff-review.txt15.86 KBtim.plunkett
#29 formbase-2059245-29.patch130.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,926 pass(es).
[ View ]
#29 interdiff.txt863 bytestim.plunkett
#26 interdiff.txt36.6 KBtim.plunkett
#26 formbase-2059245-26.patch130.16 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#22 formbase-2059245-22.patch114.54 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,215 pass(es), 914 fail(s), and 128 exception(s).
[ View ]
#22 interdiff.txt35.38 KBtim.plunkett
#19 interdiff.txt5.92 KBmsonnabaum
#18 formbase-2059245-17.patch81.72 KBmsonnabaum
FAILED: [[SimpleTest]]: [MySQL] 57,078 pass(es), 689 fail(s), and 74 exception(s).
[ View ]
#7 formbase-2059245-7.patch81.47 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,111 pass(es).
[ View ]
#7 interdiff.txt6.62 KBtim.plunkett
#5 formbase-2059245-5.patch74.85 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,625 pass(es), 27 fail(s), and 11 exception(s).
[ View ]
#5 interdiff.txt3.01 KBtim.plunkett
#2 formbase-2059245-2.patch73.79 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,623 pass(es), 27 fail(s), and 49 exception(s).
[ View ]
#2 interdiff.txt47.96 KBtim.plunkett
#1 formbase-2059245-1.patch25.83 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,882 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new25.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,882 pass(es).
[ View ]

Here's a start.

Issue tags:+API change
StatusFileSize
new47.96 KB
new73.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,623 pass(es), 27 fail(s), and 49 exception(s).
[ View ]

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.

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.

Issue tags:+DX (Developer Experience)
StatusFileSize
new3.01 KB
new74.85 KB
FAILED: [[SimpleTest]]: [MySQL] 57,625 pass(es), 27 fail(s), and 11 exception(s).
[ View ]

Missed ControllerInterface!

Issue tags:-DX (Developer Experience)
StatusFileSize
new6.62 KB
new81.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,111 pass(es).
[ View ]

Missed a spot (or three)

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

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.

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.

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.

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

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

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

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.

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.

StatusFileSize
new81.72 KB
FAILED: [[SimpleTest]]: [MySQL] 57,078 pass(es), 689 fail(s), and 74 exception(s).
[ View ]

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

StatusFileSize
new5.92 KB

And interdiff.

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.

StatusFileSize
new35.38 KB
new114.54 KB
FAILED: [[SimpleTest]]: [MySQL] 56,215 pass(es), 914 fail(s), and 128 exception(s).
[ View ]

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.

Yeah +1 for #22.

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

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

Status:Needs work» Needs review
StatusFileSize
new130.16 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new36.6 KB

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

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

Issue tags:+DX (Developer Experience), +FormInterface
StatusFileSize
new863 bytes
new130.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,926 pass(es).
[ View ]

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

  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?

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

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

Status:Needs work» Needs review
StatusFileSize
new15.86 KB
new34.22 KB
new146.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,949 pass(es), 45 fail(s), and 52 exception(s).
[ View ]

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.

Issue summary:View changes

Updated

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new145.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,158 pass(es).
[ View ]
new2.96 KB

Stupid mistakes.

StatusFileSize
new142.63 KB
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]

Blocker went in.

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

  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

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

Status:Needs review» Reviewed & tested by the community

Yep, now for API change approval.

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

oh, larowlan was faster:P
+1

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.

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.

Title:Add a FormBase class containing useful methodsChange 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!

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.

Title:Change notice: Add a FormBase class containing useful methodsAdd 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.

Assigned:tim.plunkett» Unassigned

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

Issue summary:View changes

updated