Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#38 | 1924992-system-config-form-37.patch | 42.78 KB | kim.pepper |
#38 | interdiff-from-30.txt | 678 bytes | kim.pepper |
#36 | 1924992-system-config-form-36.patch | 42.8 KB | kim.pepper |
#36 | interdiff.txt | 2.17 KB | kim.pepper |
#33 | 1924992-system-config-form-33.patch | 40.63 KB | kim.pepper |
Comments
Comment #1
tim.plunkettThis contains #1921996: Convert system_config_form() to implement FormInterface as a base class. for now. Will reroll once that's in.
Comment #2
tim.plunkettTagging.
Comment #4
tim.plunkettI'll have to ask chx if he thinks drupal_form_submit() should work with FormInterface. In the meantime, here's a fix.
Comment #5
tim.plunkettNope, that workaround should be removed, this is a bug. Postponing on #1925140: drupal_form_submit() does not accept FormInterface objects
Comment #6
tim.plunkettI fixed #1925140: drupal_form_submit() does not accept FormInterface objects but its not in yet, here's the interdiff from #1, I'll post a patch when there are less dependencies.
Comment #7
tim.plunkettThose other issues were committed.
Comment #8
tim.plunkettThis needs a reroll, use #1925738: Convert language's system_config_form() to SystemConfigFormBase as a guide
Comment #9
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #11
robmc CreditAttribution: robmc commentedComment #12
damiankloip CreditAttribution: damiankloip commentedIt looks good, I think we just need to change the test as well to reflect those changes?
Comment #13
Crell CreditAttribution: Crell commentedIf we're entering a config context here, when are we leaving it?
Otherwise this looks fine to me. Well, aside from the form having way too many values in it to be considered sane, but this is not the issue to deal with that. :-)
Comment #14
tim.plunkettThe newest patch is missing all of the changes to user_menu(), as well as deleting any of the old user_admin_settings() code.
I answer @Crell's question with another question: Where does anything ever leave that context? I see config_import(), config_install_default_config(), user_mail() (what?!), and no where else but tests.
Comment #15
Crell CreditAttribution: Crell commentedUm, duh. Crell--. I can't even claim it's late for missing that. :-)
Comment #16
damiankloip CreditAttribution: damiankloip commentedNot sure what happened there, this should be better.
Comment #18
damiankloip CreditAttribution: damiankloip commentedWithout the fatal would be nice.
Comment #19
Crell CreditAttribution: Crell commentedI know this works, because of how drupal_get_form() works now, but we should probably make it $this->form, not $this->form_id, if what we're saving is a form object and not an ID. (And then adjust the rest of the test accordingly.)
The NOT_USED stuff is gone. Instead, specify 'route_name' => 'user_account_settings'.
I'm actually kinda surprised it worked. :-)
Comment #20
kim.pepperMade changes as per #19 as well as a couple of other cleanups.
Kim
Comment #22
kim.pepperFor some reason
Drupal\user\AccountSettingsForm::formSubmit()
is not being called to save the form values ondrupal_submit_form()
Comment #23
kim.pepperCan I suggest we rename this class from
SystemConfigFormBase
toSystemConfigFormTestBase
to make it clearer it's a base class for tests?Comment #24
tim.plunkettThere's an issue for that. I'll find it and post it here when I'm at my computer
Comment #25
tim.plunkett#1945326: Rename \Drupal\system\Tests\System\SystemConfigFormBase to \Drupal\system\Tests\System\SystemConfigFormTestBase
And I think the method is named submitForm()
Comment #26
damiankloip CreditAttribution: damiankloip commentedJust a couple of small things needed fixing.
Comment #27
kim.pepperChasing head.
Comment #28
tim.plunkettI think this should call parent::__construct($config_factory, $context) instead.
I'm guessing that's the only real change (other than create() which is fine). Interdiffs++ ;)
Comment #29
kim.pepperI had some rebasing weirdness for #27 which I assume was due to the rename issue going in #1945326: Rename \Drupal\system\Tests\System\SystemConfigFormBase to \Drupal\system\Tests\System\SystemConfigFormTestBase.
Seems some pretty simple changes to me, but I'm not super sure why those changes came up in the interdiff.
Interdiff for #27 attached.
Comment #30
kim.pepperThis fixes the issues raised in #28
Comment #31
Crell CreditAttribution: Crell commentedIt doesn't matter in this case, but it's common practice to call parent::__construct() first and then do the new stuff unless the new stuff is specifically modifying the parameters to pass up to the parent.
Not something that should block a commit; just a note.
I couldn't find anything else to complain about, so unless Tim wants to disagree...
Comment #32
tim.plunkettI don't see how this is related to the issue.
While removing that, can you swap the __construct bit?
Comment #33
kim.pepperNot sure how that happened. Here's a patch that removes those changes, as well as swaps the construct bit.
Comment #35
tim.plunkettOh! That explains why those hunks are in there. The user test extends SystemConfigFormTestBase, which was not using the new approach yet. My sincerest apologies.
@kim.pepper, can you reroll one last time, re-adding those test changes but keeping your constructor switch?
Comment #36
kim.pepper@tim.plunkett Makes sense now. My head was full of other stuff yesterday afternoon, and I couldn't quite remember what the reasons for adding these changes were!
Here's a re-roll, re-adding those changes, and including the constructor switch.
Comment #38
kim.pepperLet's try that again.
This is just patch #30 with constructor switch.
Comment #39
tim.plunkettThanks, that looks good now.
Comment #40
alexpottCommitted 87f01f0 and pushed to 8.x. Thanks!