While working on #1471432: Rework language_list(), let people use more special languages I stumbled into our broken support for disabled languages. You can assign disabled languages to nodes but only if you have translation module enabled. Still then you cannot assign disabled languages to path aliases (which nodes might create) or stage interface translations a similar way. This seems to be the only feature of disabled languages, and it otherwise provides for lots of complication to try and be supported on the backend but not on the frontend. It certainly complicated #1471432: Rework language_list(), let people use more special languages to a point where it did not make sense to support anymore. Chatlog with @catch about the motivation:

[07:44am] GaborHojtsy: catch: hi
[07:45am] catch: Hi GaborHojtsy
[07:47am] GaborHojtsy: catch: I wanted to tap your mind for an interesting question "disabled languages" and what does that mean - in D7, if you disable a language, you can still assign it to content for example, but it would not show in the language switcher block; you cannot assign disabled languages to path aliases though, so when the content gets a path alias, you cannot edit it on the path alias admin, etc.... I stumbled into this problem when unifying the language lists available in http://drupal.org/node/1471432 (and all test fails are related to the previous assumptions)
[07:47am] Druplicon: http://drupal.org/node/1471432 => Rework language_list(), let people use more special languages => Drupal core, language system, normal, needs work, 49 comments, 10 IRC mentions
[07:47am] GaborHojtsy: catch: so the patch currently tries to walk that fine line "better" of allowing assignment of disabled languages to stuff but then not allowing the use of those things on the fronted... so you could assign a path alias to a disabled language but not use it, etc.
[07:47am] catch: GaborHojtsy: is this punishment for http://drupal.org/node/356036 ?
[07:47am] Druplicon: http://drupal.org/node/356036 => Allow creating nodes in disabled languages => Drupal core, translation.module, major, closed (fixed), 61 comments, 4 IRC mentions
[07:48am] GaborHojtsy: catch: yeah
[07:48am] GaborHojtsy: catch: so I'd be much happier if we would remove disabled languages to appear anywhere
[07:48am] GaborHojtsy: catch: people basically use this as content staging in new languages
[07:48am] catch: GaborHojtsy: it looks like that's what I originally wanted
[07:48am] GaborHojtsy: catch: before they introduce new languages
[07:49am] catch: GaborHojtsy: so I would not be at all opposed to making disabled really mean disabled.
[07:50am] catch: GaborHojtsy: and if people want to do content staging they can override either end of this to allow that workflow?
[07:51am] GaborHojtsy: catch: yeah, I think those people should use per node permissions or something to hide nodes in staged languages
[07:51am] GaborHojtsy: catch: this quickly spirals into other things, like menus in disabled languages, and you know....
[07:51am] catch: GaborHojtsy: yep.
[07:51am] GaborHojtsy: catch: so I'd better for "disabled" to just mean "you cannot see it on the site anymore but not yet deleted"
[07:53am] GaborHojtsy: catch: disabled languages are an interesting concept either way... what do we do with data (node, path alias, etc) that has a disabled language assigned (from before it was still enabled)
[07:53am] catch: GaborHojtsy: you've seen the disabled modules issue?
[07:53am] GaborHojtsy: catch: no
[07:53am] catch: GaborHojtsy: http://drupal.org/node/1199946
[07:53am] Druplicon: http://drupal.org/node/1199946 => Ensure that disabled modules maintain data integrity => Drupal core, base system, major, needs work, 87 comments, 23 IRC mentions
[07:54am] catch: GaborHojtsy: that contains my general feeling on 'disabled', applies to languages as much as modules.
[07:55am] GaborHojtsy: catch: so I'm constantly thinking what should disabled language mean; if I see a node in the node admin overview in that language, should I see the language name? should it say (disabled)? Should users be able to access content (eg. nodes) in a disabled language or does that hide all content in that language? or would it just remove all navigation to that language from the public? what if its a custom menu item? eh.
[07:55am] catch: GaborHojtsy: I think it's the sort of feature we should seriously just look at dropping because it's a big headache for very little gain.
[07:56am] catch: It's content depending on configuration, then removing the configuration from under the content's feet.
[07:56am] GaborHojtsy: catch: yeah
[08:00am] catch: GaborHojtsy: does that help at all?
[08:00am] GaborHojtsy: catch: yeah, I read up on the modules issue
[08:01am] GaborHojtsy: catch: similar case, I was really walking on eggs in http://drupal.org/node/1471432 to try and figure out when to allow disabled languages and when not
[08:01am] Druplicon: http://drupal.org/node/1471432 => Rework language_list(), let people use more special languages => Drupal core, language system, normal, needs work, 49 comments, 11 IRC mentions
[08:01am] catch: GaborHojtsy: I think I've lost that one, but I'm just going to never ever fix any issues to do with disabled modules ever again
[08:02am] GaborHojtsy: catch: I think I should mark the language issue postponed and open one for removing support for assigning disabled languages at least

Attached a non-tested quick patch based on my work from #1471432: Rework language_list(), let people use more special languages.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drop-disabled-languages.patch, failed testing.

andypost’s picture

Let's take another issue into account #400450: Be more graceful when editing nodes with disabled node types

Suppose we should work with all disabled controls by the same way.

For example when some filter format is used but not available we have fall-back format, probably we could have a fall-back to system language or node-defaults

Gábor Hojtsy’s picture

No, please no, no, no :( Supporting disabled languages is a nightmare as you think about it trickling down different areas. See the chatlog above. If you support it on nodes only on translation, that is an impossible feature. You need to support it on nodes without translation then (eg. in overviews, when you create new content only in that language "for staging" as the comment in the removed code stated the intention for this mis-feature). We need to support disabled languages on aliases, so the node alias can be saved. Then we need to be able to navigate to this page, which is impossible if the language is disabled and you use field translation, so we'd need to support getting to pages in disabled languages (configuring and supporting path prefixes for languages in disabled languages too). Same for blocks, and so on and on. So what is the difference of disabled or enabled languages then, since everything is so interconnected that it trickles down to supporting it at all places.

andypost’s picture

@Gábor Hojtsy I scare that dropping this support we loose ability to enable language version of site with one click. I see only one purpose of disabled language is to provide ability to make some language version of site without publishing.

How do we change workflow for creating multilingual sites then?

Gábor Hojtsy’s picture

@andypost: the point is you already cannot do that. It does not work for field translation because you cannot configure path prefixes and domains for a disabled language, and it will not show disabled languages in switchers, so you cannot switch to it otherwise. It does not let you assign path aliases in that language, it does not unpublish the menu items you create for the nodes, etc. It does not even work for nodes and it is not going to be supported for other entity types... And the list goes on and on. If you need content staging, use solutions for that. Node/entity access, staging servers, etc. It is just as likely that you need to stage a new language or a new menu section or a new subsite. These involve menu settings and pieces of content. Disabled languages nevet spanned that, it only allowed for node assignment just as an accident that grew into a kind-of-feature. It provides so broken a benefit and is so complex to fix (basically solve the content staging problem just for languages), that it is pointless.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
35.53 KB

Changed the tests too that worked with disabling / enabling languages. There are lots of opportunities to clean up tests, I was surprised that the path.test had tests for testing path prefixes in foreign languages (has nothing to do with functionality in path module at all). Trying to focus this issue on just removing the deadly broken support for disabled languages though.

Status: Needs review » Needs work

The last submitted patch, drop-disabled-languages-6.patch, failed testing.

plach’s picture

FWIW, altough being one of the persons that actually made disable languages assignable to nodes, I agree that using to proper tools for staging site modifications should be the right way. It looked like a valid feature way back then but the experience teach us that it wasn't that great idea after all.

Aside from staging content the idea of disabling languages was useful only to remove native english from the accessible languages. In D8 this is not need anymore and if we don't support the ability to stage new languages, since there are better ways to do that, I'd say it makes totally sense to drop support for disabled languages altogether. This will make the UX simpler besides the already stated DX advantages.

andypost’s picture

What's about removing "enabled" and introducing "published" ?
probably we should close #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc)

Gábor Hojtsy’s picture

No, #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) still makes sense, eg. we need to let users remove all kinds of special languages from node assignment if they want to as a usability feature. It might or might not be contrib though.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
37.21 KB

Fixed the remaining tests that assumptions about disabled languages. Added language deletion and addition in their place. As expected, this was very limited to certain obscure areas in the tests even. It feels so great to do this cleanup :)

andypost’s picture

Status: Needs review » Needs work

Patch is good except comments and some assert messages.

Terminology should be reviewed - should we use Installed or Added languages?

+++ b/core/includes/bootstrap.inc
@@ -2551,45 +2551,34 @@ function language_multilingual() {
+ * Returns a list of enabled languages.

enabled? seem added is better

+++ b/core/modules/language/language.test
@@ -122,20 +113,13 @@ class LanguageListTest extends DrupalWebTestCase {
+    // Get the count of enabled languages.

s/enabled/added/

+++ b/core/modules/locale/locale.test
@@ -2330,8 +2291,6 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
     // Ensure enabled language appears.
     $this->assertText($name, t('Enabled language present.'));

same

+++ b/core/modules/system/system.module
@@ -3922,7 +3922,7 @@ function system_date_format_save($date_format, $dfid = 0) {
   // Retrieve an array of language objects for enabled languages.
-  $languages = language_list(TRUE);
+  $languages = language_list();

this comment should be fixed too

+++ b/core/modules/language/language.test
@@ -122,20 +113,13 @@ class LanguageListTest extends DrupalWebTestCase {
     // Delete the disabled language.
     $this->drupalPost('admin/config/regional/language/delete/fr', array(), t('Delete'));
+    // Get the count of enabled languages.
+    drupal_static_reset('language_list');
+    $enabled_languages = language_list();

Delete fr language

+++ b/core/modules/translation/translation.test
@@ -301,15 +284,9 @@ class TranslationTestCase extends DrupalWebTestCase {
-      // It's installed but not enabled. Enable it.
+      // It's installed. No need to do anything.

Maybe it's better idea to use Installed not Added language

Gábor Hojtsy’s picture

I think after this patch, enabled == installed == added == existing == present == setup == configured, etc. English is a flexible and versatile language, I don't think we need to unify this (right now at least).

So the language_list() comment that you noted still says enabled could be:

+ * Returns a list of enabled languages.
+ * Returns a list of installed languages.
+ * Returns a list of added languages.
+ * Returns a list of configured languages.
+ * Returns a list of languages set up.
+ * Returns a list of languages.
+ * Returns a list of languages on the system.

After this patch, all these mean the same thing :)

andypost’s picture

+1 to * Returns a list of configured languages.

Language could be configured(installed) via Add or Import

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
39.07 KB

Ok, picked "Returns a list of configured languages." for language_list(). Fixed the other places you noticed and a couple more where enabled languages were specifically called out.

kalman.hosszu’s picture

Status: Needs review » Reviewed & tested by the community

I agree with andypost's comments. I checked the code and everything is fixed, so I change the status to RTBC.

Kálmán

catch’s picture

Have a feeling the user module decoupling patch conflicted with this, marking for re-test.

catch’s picture

#15: drop-disabled-languages-15.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
39.12 KB

Only one hunk failed in locale.module related to the user module move-around indeed. Rerolled. Let's wait for it passing and then should be back to RTBC.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in then :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
41.43 KB

Made the update function inline in upgrade bootstrap as per @catch's recommendation in IRC (we are not supporting D8-D8 head updates still). Also moved the previous update function to there on his request. Its a minimal change. Should still pass.

Status: Needs review » Needs work

The last submitted patch, drop-disabled-languages-21.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
41.43 KB

Missed one column rename when copying the count update code.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Ok, so this one just moves the update code from update functions that would never have run to the upgrade bootstrap as per catch's suggestion. Should be still good to go.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed; it is actually a nice code simplification in addition fixing a usability wtf. Committed to 8.x.

Gábor Hojtsy’s picture

Issue tags: -sprint

Added a change notice at http://drupal.org/node/1548406

sun’s picture

Status: Fixed » Needs review
FileSize
510 bytes
sun’s picture

Status: Needs review » Fixed

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