Make node and path depend on language module only for language support, get rid of locale_language_name [#1414314]

This is the final nail in the coffin for locale module to be needed for any base language assignment support for two data types it supports language assignment with, nodes and paths. Now that we got over locale_language_list() in #1387608: Unify language_list() and locale_language_list(), next up is getting rid of locale_language_name() and the very convoluted invocation patterns used for that. Since the names of languages is relatively well standardized, we can centralize that logic instead of repeating at places, so looks like we still need a language_name() kind of function which can return the name of the language for printing. However, unlike the previous locale_language_name() this should be able to return the name for disabled languages (why not?), and be able to return something useful if the language was unknown, instead of lumping that up with LANGUAGE_NONE, which is "a distinct type of unknown".

The suggested patch moves language_load() up to the same level as language_list() and adds a language_name() on the same level, so path and node do not need to use convoluted module_invoke()s to do their stuff. If language module is disabled but the language codes are still there, it will display them as unknown languages with their language codes intact. Better than it was before for path module, same behavior as for node module before.

The attached patch makes both node module and path module provide their language assignment features with language.module enabled only, which is the promise of language module itself. Its goal is to serve as a base for language definitions and assignment, and with this patch it would finally fulfill that for the existing two data types it serves.

For comment language support to depend on language module only, that is actually a bit of a different story. IMHO we should do that in a different issue, but we can lump it up here if desired. And apart from that, locale module only works with config data (date formats) and its own data (UI translations) for language. The config data is hopefully soon to be refactored as part of CMI, and we are going to work on refactoring the UI translation part as well.

Comment	File	Size	Author
#16	language-name-simplified-16.patch	9.52 KB	Gábor Hojtsy
#16
#7	language-name-simplified-4.patch	6.81 KB	tstoeckler
#7
#4	language-name-simplified-4.patch	6.81 KB	Gábor Hojtsy
#4
	language-name-simplified.patch	6.79 KB	Gábor Hojtsy

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 23 January 2012 at 16:45

I think I might have over-promised a bit on this one. Even though path module got largely independent of locale module (minus the patch above) in #1236680: Move path language settings from Locale to Path module, node module is still in large ways dependent on locale module due to the patch in #1236680: Move path language settings from Locale to Path module still being in the works. Both of these need to land to make node independent of locale module.

Comment #2

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 23 January 2012 at 16:46

The second link was supposed to be #540294: Move node language settings from Locale to Node module.

Comment #3

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 23 January 2012 at 19:26

Status:

Needs review

» Needs work

+++ b/core/modules/path/path.admin.inc
@@ -14,9 +14,9 @@
+  // Enable language column if lanugauge is enabled or if we have any alias with language

Typo: lanugauge -> language.
Also, since we're already changing this, I think we usually do "language.module" for that to be linkable on a.d.o (although I'm actually not sure that that works with inline comments ATM).
And since we're at it, perhaps: "any alias with A language." (Also note the period at the end.)

+++ b/core/modules/path/path.admin.inc
@@ -194,7 +194,7 @@ function path_admin_form_validate($form, &$form_state) {
+  // Language is only set if language module is enabled, otherwise save for all languages.

Like above, I think "language.module" is preferred.

Looks very good in general. I looked a bit at node_admin_nodes for the surrounding code, but there's nothing needed that's not in the patch. I have a deep urge to bikeshed this issue into fixing everything that is wrong with this function (< hint >a lot< /hint >), but I can resist.
-22 days to next Drupal core point release.

Comment #4

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 24 January 2012 at 06:17

Status:

Needs work

» Needs review

File	Size
language-name-simplified-4.patch	6.81 KB

All right, made the suggested changes (and made comments wrap properly as well).

Comment #5

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 25 January 2012 at 09:18

Looks like a pretty simple patch to me. Any other comments?

Comment #6

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 25 January 2012 at 09:30

Status:

Needs review

» Needs work

+++ b/core/modules/path/path.admin.inc
@@ -14,9 +14,10 @@
+  // Enable language column if language.module is enabled or if we have any
+  // alias with a language.

@@ -194,7 +194,8 @@ function path_admin_form_validate($form, &$form_state) {
+  // Language is only set if language.module is enabled, otherwise save for
+  // all languages.

It feels weird to mark this patch "needs work" based on this triviality, but I'm pretty sure these comments wrap too early.

-24 days to next Drupal core point release.

Comment #7

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 25 January 2012 at 09:33

Status:

Needs work

» Needs review

File	Size
language-name-simplified-4.patch	6.81 KB

Wait, the first one is actually correct. That must be a bug in Dreditor, showing the charcount incorrectly... Sorry! The second one was actually wrong, though, so here's a quick reroll.

Comment #8

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 25 January 2012 at 09:36

Status:

Needs review

» Reviewed & tested by the community

Wow, that is indeed super weird. Dreditor is, in fact, wrong. I manually counted the actual characters (twice!), and I'm confident that it is correct now.

Since I only moved a single word (docs!!!) from one line to another, I am marking this RTBC myself. Feel free to cut my head off.

Comment #9

25 January 2012 at 10:13

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, language-name-simplified-4.patch, failed testing.

Comment #10

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 25 January 2012 at 10:30

The fail seems to be unrelated, coming from #61456: Aggregator titles display quotes and other characters with HTML entity equivalents badly (write tests).

Comment #11

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 25 January 2012 at 14:57

Status:

Needs work

» Needs review

#7: language-name-simplified-4.patch queued for re-testing.

Comment #12

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 January 2012 at 12:30

Status:	Needs review	» Reviewed & tested by the community
Issue tags:		+sprint

Tagging as current focus. Back to RTBC. The earlier test fails were due to a test system glitch.

Comment #13

Dries CreditAttribution: Dries commented 26 January 2012 at 16:31

Status:

Reviewed & tested by the community

» Needs work

Nice clean-up.

+++ b/core/modules/path/path.admin.incundefined
@@ -44,7 +44,7 @@ function path_admin_overview($keys = NULL) {
-      $row['data']['langcode'] = module_invoke('locale', 'language_name', $data->langcode);

Do we want to rename the 'langcode' index to 'language'? We are storing the name, not the language code.

+++ b/core/modules/path/path.admin.incundefined
@@ -130,10 +130,10 @@ function path_admin_form($form, &$form_state, $path = array('source' => '', 'ali
-    $language_options = array(LANGUAGE_NONE => t('All languages'));

'Language neutral' feels a bit technical. Would 'Any language' be easier to understand? It would for me.

Comment #14

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 January 2012 at 16:58

@Dries: agreed on the langcode. For "Language neutral" I agree we need to clean that up. However, LANGUAGE_NONE does not mean 'Any language', it means 'No language'. The way we used it for nodes is that 'Language information not applicable' (eg. you have an image node with mountain pictures without titles or descriptions). The way we used it for paths is 'Applies to all languages, unless there is a more specific language alias'. Path module used to call it 'All' and node called it 'Language neutral' but it is in fact the same concept. It is really saying 'Language does not apply' not 'Any language'.

Comment #15

sun

German

Karlsruhe

CreditAttribution: sun commented 26 January 2012 at 17:04

Then I'd simply go with "- None -" (i.e., #empty_value => LANGUAGE_NONE)

Based on the explanation in #14, that's exactly what the option means (neither all, neither any, neither neutral, but yes, simply no language).

Comment #16

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 26 January 2012 at 22:49

Status:	Needs work	» Needs review
Issue tags:	-DX (Developer Experience)	+API clean-up

File	Size
language-name-simplified-16.patch	9.52 KB

@sun: Updated patch to use - None - for language names at all places where Language neutral would have been. Also using #empty_* as appropriate hopefully.

@Dries: modified 'langcode' at the place you rightly noticed to 'language_name'. It is not a 'language' per say, we are reserving using 'language' for language objects to make the API straightforward and simple.