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.

Files: 
CommentFileSizeAuthor
#23 form-base-config-2087279-23.patch17.96 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,827 pass(es). View
#23 interdiff.txt814 bytestim.plunkett
#22 drupal8.forms-system.2087279-22.patch17.95 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#19 drupal8.forms-system.2087279-19.patch17.15 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es). View
#19 interdiff.txt701 bytesdisasm
#17 config-2087279-17.patch16.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,106 pass(es), 24 fail(s), and 7 exception(s). View
#17 interdiff.txt1.42 KBdawehner
#15 config-2087279-15.patch15.31 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,180 pass(es), 24 fail(s), and 18 exception(s). View
#10 form-base.2.patch17.07 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,924 pass(es). View
#9 interdiff.txt817 bytestim.plunkett
#1 form-base-config.patch18.41 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-base-config.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
18.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form-base-config.patch. Unable to apply patch. See the log in the details link for more information. View

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
PASSED: [[SimpleTest]]: [MySQL] 58,924 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 59,180 pass(es), 24 fail(s), and 18 exception(s). View

.

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
FAILED: [[SimpleTest]]: [MySQL] 59,106 pass(es), 24 fail(s), and 7 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

tim.plunkett’s picture

FileSize
814 bytes
17.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,827 pass(es). View

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.