Sub-issue of #1924990: [meta] Convert all of system_config_form() to SystemConfigFormBase

CommentFileSizeAuthor
#14 language-1925738-14.patch1.33 KBtim.plunkett
#9 1925738-test-run.png42.05 KBKars-T
#8 language-1925738-8.patch22.66 KBtim.plunkett
#7 language-1925738-7.patch44.68 KBtim.plunkett
#7 interdiff.txt7.43 KBtim.plunkett
#4 language-1925738-4.patch24.65 KBtim.plunkett
#3 language-1925738-3.patch38.06 KBtim.plunkett
#3 interdiff.txt777 bytestim.plunkett
#1 language-1925738-1.patch38.05 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
language-1925738-1.patch38.05 KB

Includes #1921996: Convert system_config_form() to implement FormInterface as a base class. and #1925140: drupal_form_submit() does not accept FormInterface objects

Status: Needs review » Needs work

The last submitted patch, language-1925738-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt777 bytes
language-1925738-3.patch38.06 KB

Copy/paste fail on my part.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
language-1925738-4.patch24.65 KB

Those other issues were committed.

Status: Needs review » Needs work
Issue tags: -FormInterface

The last submitted patch, language-1925738-4.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: +FormInterface

#4: language-1925738-4.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt7.43 KB
language-1925738-7.patch44.68 KB

Now includes #1934832: Provide a dedicated approach for using forms in routes
I added a base class to cut down on copy/paste, but its much better with the _form in routing.yml and the ControllerInterface.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
language-1925738-8.patch22.66 KB

Okay! Everything is finally in.

Kars-T’s picture

Kars-T CreditAttribution: Kars-T commented
Status: Needs review » Reviewed & tested by the community
FileSize
1925738-test-run.png42.05 KB

The patch is rather basic as it encapsulates stuff from language.admin.inc in new SystemConfigFormBase objects and uses the symfony routing system. The source this way is much cleaner and easier to read. I checked if it uses the same config names and let the tests runs. They finished without any problems.

To me the changes make sense and as the tests don't show any errors I'd say it is rtbc.

1925738-test-run.png

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: -FormInterface

#8: language-1925738-8.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue tags: +FormInterface

#8: language-1925738-8.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/language/language.moduleundefined
@@ -114,18 +114,14 @@ function language_menu() {
+    'page callback' => 'NOT_USED',
     'access arguments' => array('administer languages'),
...
+    'page callback' => 'NOT_USED',
     'access arguments' => array('administer languages'),

@@ -145,10 +141,8 @@ function language_menu() {
+    'page callback' => 'NOT_USED',
     'access arguments' => array('administer languages'),

Is not it needs follow-up to clean-up? Why "route name" is not used?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Fixed » Needs review
FileSize
language-1925738-14.patch1.33 KB

Yep, that changed since March 8th when I wrote this :)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

Suppose bot would be green!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed c0eaf0f and pushed to 8.x. Thanks!

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