Updated: Comment #N
Followup for #1974210: Convert admin/structure/forum to a Controller

Problem/Motivation

We're seeing the need for config objects more commonly in FormBase
Lets have a config() method.

Proposed resolution

Add a config() method

Remaining tasks

Patch
Review

User interface changes

None

API changes

New config() method on FormBase

Follow-up from #1974210: Convert admin/structure/forum to a Controller.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
18.41 KB

something like this

jibran’s picture

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

SystemConfigFormBase is also using $configFactory

larowlan’s picture

Status: Needs work » Needs review

it can't be removed as it sets the context in the constructor, thus needs it there

larowlan’s picture

(I couldn't figure out how to remove it)

Status: Needs review » Needs work
Issue tags: -WSCCI, -FormInterface, -FormBase

The last submitted patch, form-base-config.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

#1: form-base-config.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +FormInterface, +FormBase

The last submitted patch, form-base-config.patch, failed testing.

larowlan’s picture

Status: Needs work » Postponed

Forum issue was reverted

tim.plunkett’s picture

FileSize
817 bytes

We'd need this hunk for SystemConfigFormBase, and eventually phase out the constructor

larowlan’s picture

FileSize
17.07 KB

RE-roll without forum overview.
Includes #9

larowlan’s picture

Status: Postponed » Needs review
andypost’s picture

+1 to RTBC, Each form needs config, and next one is state()?

dawehner’s picture

I don't want to be the badguy but I prefer the way how ControllerBase does it, as you specify the name of the config file and return a Config object instead of getting the full config factory.

dawehner’s picture

Just hit into an issue with ControllerBase: #2089195: ControllerBase::config does not work as expected

dawehner’s picture

FileSize
15.31 KB

.

Status: Needs review » Needs work

The last submitted patch, config-2087279-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
16.73 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, config-2087279-17.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
701 bytes
17.15 KB

this fixes the test failures.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh damn I remembered when I worked on that last night but I seem to have missed to upload the patch.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Somehow this lost the hunk from #9.

disasm’s picture

Status: Needs work » Needs review
FileSize
17.95 KB

adding #9. No interdiff attached since #9 was directly applied and committed.

tim.plunkett’s picture

More like this.

webchick’s picture

+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
@@ -138,7 +126,7 @@ public function validateName(array &$form, array &$form_state) {
-    $flood_config = $this->configFactory->get('user.flood');
+    $flood_config = $this->config('user.flood');

Code like this makes my heart sing.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good it removes a lot off boilerplate code so RTBC.

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -104,6 +135,20 @@ public function setTranslationManager(TranslationInterface $translation_manager)
+   * @return self
+   *   The form.
...
+    return $this;

Usually setter don't return anything. @see FormBase::setRequest or FormBase::setUrlGenerator. But then we have FormBase::setTranslationManager returning $this. We should make a rule and follow it. We can do it in followup issue.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thank you! :D

Committed and pushed to 8.x. Thanks!

Jibran said he would take care of creating the follow-up in IRC.

webchick’s picture

.

Status: Fixed » Closed (fixed)
Issue tags: -DX (Developer Experience), -WSCCI, -FormInterface, -FormBase, -pants

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +D8MI, +language-config, +Configuration context

Tagging retroactively for configuration context because this is one of the few places where its used "properly" in the admin area.