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
Related Issues
Follow-up from #1974210: Convert admin/structure/forum to a Controller.
Comment | File | Size | Author |
---|---|---|---|
#23 | form-base-config-2087279-23.patch | 17.96 KB | tim.plunkett |
#23 | interdiff.txt | 814 bytes | tim.plunkett |
#22 | drupal8.forms-system.2087279-22.patch | 17.95 KB | disasm |
#19 | drupal8.forms-system.2087279-19.patch | 17.15 KB | disasm |
#19 | interdiff.txt | 701 bytes | disasm |
Comments
Comment #1
larowlansomething like this
Comment #2
jibranSystemConfigFormBase
is also using$configFactory
Comment #3
larowlanit can't be removed as it sets the context in the constructor, thus needs it there
Comment #4
larowlan(I couldn't figure out how to remove it)
Comment #6
jibran#1: form-base-config.patch queued for re-testing.
Comment #8
larowlanForum issue was reverted
Comment #9
tim.plunkettWe'd need this hunk for SystemConfigFormBase, and eventually phase out the constructor
Comment #10
larowlanRE-roll without forum overview.
Includes #9
Comment #11
larowlanComment #12
andypost+1 to RTBC, Each form needs config, and next one is
state()
?Comment #13
dawehnerI 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.
Comment #14
dawehnerJust hit into an issue with ControllerBase: #2089195: ControllerBase::config does not work as expected
Comment #15
dawehner.
Comment #17
dawehnerThere we go.
Comment #19
disasm CreditAttribution: disasm commentedthis fixes the test failures.
Comment #20
dawehnerOh damn I remembered when I worked on that last night but I seem to have missed to upload the patch.
Comment #21
tim.plunkettSomehow this lost the hunk from #9.
Comment #22
disasm CreditAttribution: disasm commentedadding #9. No interdiff attached since #9 was directly applied and committed.
Comment #23
tim.plunkettMore like this.
Comment #24
webchickCode like this makes my heart sing.
Comment #25
jibranPatch looks good it removes a lot off boilerplate code so RTBC.
Usually setter don't return anything. @see
FormBase::setRequest
orFormBase::setUrlGenerator
. But then we haveFormBase::setTranslationManager
returning$this
. We should make a rule and follow it. We can do it in followup issue.Comment #26
webchickAwesome, thank you! :D
Committed and pushed to 8.x. Thanks!
Jibran said he would take care of creating the follow-up in IRC.
Comment #27
webchick.
Comment #28.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #29
Gábor HojtsyTagging retroactively for configuration context because this is one of the few places where its used "properly" in the admin area.