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
StatusFileSize
new18.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

StatusFileSize
new817 bytes

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

larowlan’s picture

StatusFileSize
new17.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

StatusFileSize
new15.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
StatusFileSize
new1.42 KB
new16.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
StatusFileSize
new701 bytes
new17.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
StatusFileSize
new17.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

StatusFileSize
new814 bytes
new17.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.