Sort list of languages properly on install (and remove useless space) [#966818]

The current form that generates the list of languages is quite a mess.
You would think that it's a form of '#type' => 'radios', but in fact each language is its own form of '#type' => 'radio'. The form callback then uses '#return_value' to get everything working.
The attached patch
1. changes the form to build a normal $options array and simply use a '#type' => 'radios',
2. sticks a asort() in the middle to make the list properly sorted. This is especially nice, because asort() is case-sensitive, so when using l10n_install, all those languages who don't have a proper name (xxx-lolspeak, etc...) show up last.

This is technically an API change, but in practice there's only one project that this could theoretically affect (l10n_install), but that doesn't currently alter the form.
Also Gábor Hojtsy approved this, over in #944390: Improve usability of language selector (l10n_install) issue.

Comment	File	Size	Author
#16	sort_list_language-966818-16.patch	1.46 KB	valthebald
#16
#11	sort_list_language-966818-11.patch	1.46 KB	idflood
#11
#11	sort_list_language-966818-11-d8.patch	1.46 KB	idflood
#11
#7	966818-install-locale-form-7.patch	1.44 KB	idflood
#7
#1	install-form.png	55.49 KB	tstoeckler
	install-locale-form.patch	1.62 KB	tstoeckler

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 9 November 2010 at 16:52

File	Size
install-form.png	55.49 KB

And a screenshot showing that the usage of '#type' => 'radios' removes a lot of useless whitespace.

Comment #2

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 9 November 2010 at 17:31

Oh, looks like a much better default! Why is it needs work and not needs review?

Comment #3

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 9 November 2010 at 17:48

Status:

Needs work

» Needs review

That, I don't know :)

Comment #4

Bojhan CreditAttribution: Bojhan commented 11 November 2010 at 10:43

Sorry, but did you check the implications of this on the usability of all other forms? I mean, we can easily create this more usable, I agree - but not if it negatively impacts the readability of other forms.

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 November 2010 at 10:58

This is a PHP code change that only affect this single form. No other forms are affected in the installer or elsewhere. The patch merely changes from including the elements as 'radio' buttons one by one to include them as a group of 'radios' which required less PHP code and apparently displays tighter. See screenshot.

Comment #6

Bojhan CreditAttribution: Bojhan commented 11 November 2010 at 11:23

Strange, well RTBC from visual perspective then.

Comment #7

idflood CreditAttribution: idflood commented 24 November 2010 at 14:37

Status:

Needs review

» Needs work

File	Size
966818-install-locale-form-7.patch	1.44 KB

I applied the patch and at first it looked exactly to what I was expecting.

But the "value" attribute for the inputs are wrong with this patch ( output "1", "2", ... instead of "en", "fr",... ). The problem comes from the "sort($options)". It replace the array keys, leading to wrong values. Using asort($options) fixes the issue.

The new patch only replace the sort function with an asort.

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 11 April 2011 at 07:24

Status:

Needs work

» Needs review

Just marked #1121400: Sorting the Language list will dramatically improve usability a duplicate.

Comment #9

11 April 2011 at 07:42

Status:

Needs review

» Needs work

The last submitted patch, 966818-install-locale-form-7.patch, failed testing.

Comment #10

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 11 April 2011 at 21:01

Assigned:

Unassigned

» tstoeckler

Assigning this to me, so I don't forget. This simply needs a reroll, though, so if you're faster than me, feel free.

Comment #11

idflood CreditAttribution: idflood commented 12 April 2011 at 07:31

File	Size
sort_list_language-966818-11-d8.patch	1.46 KB

sort_list_language-966818-11.patch	1.46 KB

Here is a reroll of the patch in #7. I also created a d8 patch only to see it's exactly the same as d7.

Comment #12

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 12 April 2011 at 07:37

Assigned:

tstoeckler

» Unassigned

Looks good, will try it out later.

Comment #13

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 12 April 2011 at 07:55

Version:	7.x-dev	» 8.x-dev
Status:	Needs work	» Needs review

Ok, unfortunately needs to go to 8.x first. Since its the same, it should not be scary to anybody, right? :)

Comment #14

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 12 April 2011 at 12:35

+++ b/includes/install.core.inc
@@ -1236,19 +1236,23 @@ function install_select_locale(&$install_state) {
+      $name .= ' ' . st('(built-in)');

Not strictly related to this patch, but is this form of string concatenation compatible with RTL-languages?

Powered by Dreditor.

Comment #15

aschiwi CreditAttribution: aschiwi commented 9 July 2011 at 16:52

Definitely looks much nicer! The patch in #11 works for me (Drupal 8) and I have no problems installing in another language either.

Comment #16

valthebald

Sofia

CreditAttribution: valthebald commented 20 December 2012 at 18:38

File	Size
sort_list_language-966818-16.patch	1.46 KB

I'm not quite sure how to submit to retest patch that was ignored on the first run, so just re-upload patch sort_list_language-966818-11-d8.patch from #11

Comment #17

20 December 2012 at 18:39

Status:

Needs review

» Needs work

The last submitted patch, sort_list_language-966818-16.patch, failed testing.

Comment #18

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 21 December 2012 at 07:06

Issue tags:

+D8MI, +language-base

@valthebald: well, that will not apply to Drupal 8 anymore. Does this bug still relevant for Drupal 8? We have been reworking the language list in the installer various times in Drupal 8 :)

Comment #19

valthebald

Sofia

CreditAttribution: valthebald commented 21 December 2012 at 08:21

Status:

Needs work

» Closed (works as designed)

@Gábor Hojtsy: I guess no, just wanted to ensure

Comment #20

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 21 December 2012 at 09:19

Status:

Closed (works as designed)

» Needs work

@valthebald: all I said is the patch does not apply to Drupal 8 not that all problems are not applicable to Drupal 8. I'm not sure Drupal 8 would apply any ordering different to the language code base alphabetical list, looking at the code. Also even if none apply to Drupal 8, Drupal 7 could still use a fix. So I'd not go as fast as closing this down outright.