#965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE adds these new special languages as constants. Currently LANGUAGE_NONE (to be renamed in that issue to LANGUAGE_NOT_SPECIFIED) is allowed to be assigned to some things, like nodes and path aliases, however, these languages are added on as extra items to the language list in form functions.
We should let users assign LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE as well to things like files, nodes, etc. Since the list of special languages grows in Drupal 8 from 1 (in Drupal 7) to 4 with these additions (and the already present LANGUAGE_SYSTEM), we either want to have a common language list extension wrapper or add these languages as regular pieces in the language table and then figure out #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc). Both have disadvantages. If we extend the list occasionally in code, we need to remember to do it at all times when applied to the same thing (such as for nodes in the filter UI and the node forms). If we decide to add them as languages in the language table, then we'll need to filter that down when needed, such as when UI language is considered, where these special languages have no meaning (except LANGUAGE_SYSTEM).
BTW solving #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) would have other benefits too, eg. you could configure you don't want certain special languages to show up for nodes or files and always enforce an exact language selection, etc.
Currently both LANGUAGE_NONE and LANGUAGE_SYSTEM are added occasionally when needed to the language list.
Marking postponed on #965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE landing.
Comment | File | Size | Author |
---|---|---|---|
#116 | 1471432-116.patch | 34.82 KB | vasi1186 |
#114 | 1471432-114.patch | 34.83 KB | vasi1186 |
#114 | interdiff-112-114.txt | 7.53 KB | vasi1186 |
#112 | 1471432-112.patch | 34.74 KB | vasi1186 |
#112 | interdiff_110_112.txt | 18.32 KB | vasi1186 |
Comments
Comment #1
Gábor Hojtsy#965300: Change LANGUAGE_NONE to LANGUAGE_NOT_SPECIFIED; add LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE landed, so here we go.
Comment #2
Gábor HojtsyOk, here is a starter patch to get some thinking going :) It build off of the concept of keeping these special languages, and just including them for display as an extension of the normal language list. So it introduces a language_list_prepare() that can take a list of flags and would add/remove languages based on those flags. It has the languages associated with flags built-in. This is not nearly as flexible as #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc), though I've added a drupal_alter() for kicks (although the $options are very limited, so an alter would not be able to tell much which one to include/remove for a specific content type for example).
I've applied this to the language list. Since this list is used as a summary dashboard for languages, it made sense to include all languages and use the opportunity to educate people on what are they:
This does not let people order the special languages (there is no point?) but they can move the normal languages to inbetween special languages. When saved/reloaded, the normal languages are lined up again all above the special languages of course.
I've also redone the interface translation filtering page dropdown with this new code, but no UI change there.
Finally, added the new languages to the node assignment dropdown and to the language name function so they are properly displayed in summaries:
Note that making this work is not as simple as adding the special languages as options for the node dropdown though. There are various conditions in fields, entities, etc. where the translatability, etc. is tied to the 'und' code. We need to update those places to not let translatability for the other special codes either (IMHO).
Comment #3
Gábor HojtsyNote we could alternatively also go the other direction and put in all these special languages in the language table and filter out ones not applicable as the situation dictates. We can use the 'locked' pattern that the node_type table uses for non-deletable/non-editable (code defined) content types. That would let people to disable "Not specified" let's say so it would not show up for node creation (i18n module includes such a feature for sites that never want to have nodes that have no specified language). That would also make it possible to reoder the special languages which would have an effect in eg. the list for node assignment. Not sure it is any useful to have the special langauges intermixed that way with the normal languages. Anyway, so we have some options here :)
Comment #4
Kristen PolMy 2 cents: If these will be added to the language list, I like the idea of being able to reorder them in the list in case someone wants them first or last or something else entirely.
Kristen
Comment #5
dawehnerRegarding #3
Everytime you do things on runtime, things are harder to solve when querying things and you need this on the sql level.
For example if you put them in the table language fallbacks might be easier to be managed.
This is somehow not a that big issues at this point, but it should be better check_plained.
Comment #6
pixeliteI think it would be important to allow admins to disable these special languages, either site-wide or on a per content type basis.
I also think that exposing these special languages on the node edit page as part of the language selection widget is potentially confusing. When you create a node for the first time, you're making two choices: will the node will be translatable and if so, what the language of the content currently being added is. Combining these in a single select box will be confusing for some site admins.
Comment #7
pixeliteThe patch in #2 no longer applies. I'm attaching an updated patch.
Comment #8
Kristen PolHere are my thoughts on the UI/functionality:
Comment #9
tobiasbI hope this patch is what Gábor wanted, but in my understanding this should be the right patch ;-)
Comment #11
tobiasbComment #13
tobiasbComment #15
Kristen PolI am getting a failed "hunk":
Comment #16
tobiasbOk, found the stupid mistake and fixed some tests.
Comment #18
tobiasbwhy on hell copies my eclipse ide text with "mac line-endings" and not with \n :(
Comment #20
dawehnerThe central problem is that if you save a language the static cache in locale_language_url_rewrite_url is not resetted.
So if you reset the static caches, it works at least for the test in common.test.
Comment #22
tobiasbI found the stupid bug, yeaah ;-)
Comment #24
tobiasbComment #26
Gábor Hojtsy@tobiasb: good progress! Thanks for working on this! Some notes:
1. The upgrade function should not call the API function that is also used on fresh installs unfortunately. It needs to hard-wire the same list to be safe to use the same data regardless of any future updates to the language list.
2. Looks to me instead of doing custom filtering at all times based on language_list() return values, we should make language_list() capable of filtering those languages out that are locked (which applies to many cases). Can you look into implementing it that way.
I'll also ask some other people to take a look, but this looks like it is a better direction at the end that my original patch :)
Comment #27
Gábor HojtsyMake the title friendlier :)
Comment #28
klonos...make the title easier to search for ;)
Comment #29
Gábor HojtsyHere is an update that makes language_list() filterable by two built-in filters. One is the only_enabled filter that was there before. Now it has a much more telling constant bit to tell the function to filter for that. Also added a locked filter. Used that in places instead to "declaratively" tell the system which languages we need at places instead of the procedural code to filter at all times.
Did not fix the update function. Will likely still have at least as many fails as before :)
Comment #31
Gábor HojtsyI should use proper bitwise operators :) LANGUAGE_FILTER_DISABLED | LANGUAGE_FILTER_LOCKED is the right bit combination instead of LANGUAGE_FILTER_DISABLED & LANGUAGE_FILTER_LOCKED which nulls them out and will not filter by either. Ha.
Comment #33
Gábor HojtsyFix upgrade path by not using API functions in the update and putting in the special languages soon enough in upgrade bootstrap prepare. Passes upgrade tests for me locally.
Comment #35
Gábor Hojtsylanguage_default() should also specify that it is unlocked. This should complete the patch hopefully :)
Comment #36
Gábor HojtsySome more cleanups.
- Don't add LANGUAGE_SYSTEM and LANGUAGE_ALL as special languages, since these only ever apply to the interface translation realm and not elsewhere. These are already handled in their own way there from before the patch.
- Reinstantiated descriptions for the three more universal special languages, so the language list looks like this (when English UI translation is also enabled):
Let's get this reviewed!
Comment #37
Gábor HojtsyDid not roll that properly. Rerolled here.
Comment #38
yoroy CreditAttribution: yoroy commentedGabor showed off this screen in IRC, of course I had some niggles about the ui text. Only minor ones this time :)
- I'm not a fan of the 'may be used' pattern, just use 'can' or 'use this' wherever possible
- We discussed my suggestion for 'No language specified' while I was typing this and decided that it wasn't an improvement, so lets leave that as it is :)
Comment #39
dawehnerThe docs of the function needs the change as well.
I'm just wondering why here the filter for locked is not applied.
Some people could nitpick that this function doesn't have a @return.
I'm wondering whether this could/should be simplified a bit by using language_special_languages() from above.
Comment #40
Gábor Hojtsy@dawhener: I agree with everything but your last comment. Update functions should not reuse any functions that are not purely used in updates, since they might change and might have different results depending on when they ran, which makes the upgrade path unpredictable. Therefore update functions need things hardwired and/or need to use update specific versions of functions which never call outside hooks if at all possible.
Comment #41
Gábor HojtsyFixed both things mentioned by @dawehner and @yoroy. Also decided this would be a right time to make the jump to switch language_list() and make it filter by default for the "front-end languages". @catch suggested before that filtering for enabled languages on demand is not a good idea since we likely want to have that more often. Well, I did a deep check of language_list() calls for this patch, and the results are varied. Disabled and special languages are allowed for assignment on the backend but then disabled languages are not displayed to the user (and path aliases in disabled languages, etc. are not used). The question on WTH are disabled languages is "those can be used for content/translation/path alias, etc. assignment but are not displayed on the front end until the switch is flipped". It is like an unpublished node you are working on that is not showing up on the site. An unpublished language will not show in language switchers and user language selection, but will be assignable on the backend (such as for nodes).
So this flipped approach makes it much easier to figure out where are the disabled and locked languages used. Since this flips all language_list() use, it might fail again at some places :)
Comment #43
Gábor HojtsyWorking on rerolling this for all the different stuff moving around.
Comment #44
Gábor HojtsyRetitled the issue for the current approach. I think its better if the special language constants are not there to scare eveybody away. Despite asking so many people to look into this, it was pretty well neglected, so let's try and make it more inviting :) I think its not a very complicated patch.
Comment #46
Gábor HojtsyFixed a small code glitch not referencing language properly and a disabled language test not loading all disabled languages too. Should pass more tests.
Comment #48
Gábor HojtsyTwo more tiny places where language objects were not referenced properly and resulted in failures.
Comment #50
Gábor HojtsyAfter discussion with @catch, marking postponed on #1539072: Support for disabled languages broken, drop it. See IRC log there.
Comment #51
Gábor Hojtsy#1539072: Support for disabled languages broken, drop it landed so this needs a big reroll. Should simplify this a great deal. @kalmanhosszu was interested in working on this, so also unassigning.
Comment #52
nod_Comment #53
nod_There is a life outside JS :)
Comment #55
nod_Ok not quite sure what I'm doing but we'll see.
Comment #57
Gábor HojtsyThanks! @nod_: disabled languages were removed in #1539072: Support for disabled languages broken, drop it, so LANGUAGE_ADD_DISABLED and all its use should not be there anymore. :) The previous test fails were around disabled languages, so this should also help with making the patch work :)
Comment #58
Gábor Hojtsy@nod_: are you actually working on this? You are keeping a hold by being assigned.
Comment #59
nod_ohh sorry about that, I thought Artusamak was on it. I'm not sure i'll have time to work on that soon enough.
Comment #60
kalman.hosszu CreditAttribution: kalman.hosszu commentedI can dedicate time for this issue.
Comment #61
kalman.hosszu CreditAttribution: kalman.hosszu commentedLet's try an updatet patch.
Comment #63
kalman.hosszu CreditAttribution: kalman.hosszu commentedI have to go off on the weekend, so I'll able to restart the work on this on monday.
Comment #64
Gábor HojtsyAre you still working on this?
Comment #65
kalman.hosszu CreditAttribution: kalman.hosszu commentedI can work on it.
Comment #66
Gábor HojtsyA quick review:
This should not be removed. Language types can still be disabled.
Recent changes in the updates pointed to moving the code into the update bootstrap (here) instead of in misleading update function that are invoked directly. @catch requested we move stuff here instead of confusing update functions, so let's not remove this code.
As said above, do not move this out of the upgrade bootstrap.
Integrate this directly in the upgrade bootstrap.
Based on *un*locked count, right? :)
Outdated code? We don't have disabled languages anymore. We should not have these groups here.
Comment #67
kalman.hosszu CreditAttribution: kalman.hosszu commentedA quick update.
Comment #68
kalman.hosszu CreditAttribution: kalman.hosszu commentedAn interdiff attached for the path above.
Comment #69
Gábor HojtsyNicer, smaller patch. Found this issue at a glance:
Enabled is not a valid column anymore.
Comment #70
kalman.hosszu CreditAttribution: kalman.hosszu commentedThe "enabled" col is removed.
Comment #72
kalman.hosszu CreditAttribution: kalman.hosszu commentedA little update in search tests, and an entity test fix.
Comment #74
Gábor Hojtsy#72: 1471432-72.patch queued for re-testing.
Comment #76
kalman.hosszu CreditAttribution: kalman.hosszu commentedI have computer issues since last week so I can't work on it :(
Comment #77
Gábor Hojtsy@kalman.hosszu: that is unfortunate to hear.
As for the patch, I don't agree with the search test changes, I think we discussed this over chat that if people don't have content in these possibly obscure languages, those languages should not show up in search. Hopefully there is an easy way to look up if we have search data in given languages, and display only those where we have.
Rerolled the patch since it had lots of other places where it did not apply 100% anymore. Removed the search test changes.
Comment #79
Gábor HojtsyThe tests failed with PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest995296language' doesn't exist and Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 53 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Statement.php on line 58 (which I don't think is related to this patch much).
Comment #80
Gábor Hojtsy#77: 1471432-77.patch queued for re-testing.
Comment #82
aries CreditAttribution: aries commentedTaken.
Comment #83
vasi1186 CreditAttribution: vasi1186 commented#77: 1471432-77.patch queued for re-testing.
Comment #85
vasi1186 CreditAttribution: vasi1186 commentedThe previous patch failed testing because: "Unable to apply patch."
Comment #87
vasi1186 CreditAttribution: vasi1186 commentedprevious patch wrong.
Comment #89
vasi1186 CreditAttribution: vasi1186 commented#87: 1471432-87.patch queued for re-testing.
Comment #91
vasi1186 CreditAttribution: vasi1186 commentedFixed some issues in the update.inc.
Comment #92
vasi1186 CreditAttribution: vasi1186 commenteddiff between wrong commits in the previous patch...
Comment #94
Gábor HojtsySo now we only have the memory issue left, it sounds like it goes in an infinite loop somewhere and eats up memory.
Comment #95
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that should at least pass the tests :).
Comment #97
vasi1186 CreditAttribution: vasi1186 commentedA new patch that should pass those 3 failed testcases.
Comment #98
vasi1186 CreditAttribution: vasi1186 commentedComment #99
vasi1186 CreditAttribution: vasi1186 commented#97: 1471432-97.patch queued for re-testing.
Comment #101
Schnitzel CreditAttribution: Schnitzel commentedRerolling because of PSR-0 patches.
Comment #102
Schnitzel CreditAttribution: Schnitzel commentedoh, too much in this patch, my local git was a bit brocken.
fixxed now.
Comment #103
Gábor HojtsyA few comments:
Strange to have this as 2, when this is the only language_add constant.
Whitespace.
'define' => 'defined'
'the LANGUAGE_' => 'LANGUAGE_'
whitespace issues with the operator (should have space before, after).
'for the one of the' => 'for one of the'
This is the only place where this is being cloned. Why?!
Since these don't apply anymore, remove them? We don't leave commented out code in Drupal like that.
Comment #104
Schnitzel CreditAttribution: Schnitzel commentednew version of the patch, @vasi1186 did also some work here.
This is not needed anymore, because language_default() returns a new instance of the Object.
Comment #105
vasi1186 CreditAttribution: vasi1186 commented#104: 1471432-104.patch queued for re-testing.
Comment #107
Schnitzel CreditAttribution: Schnitzel commentedreroll because of PSR-0 testlove.
Comment #108
Gábor HojtsyLooks good!
Comment #109
webchickNice work! Overall this looks fine, but coming into it fresh I had a couple of observations/comments:
Since we already have a constant that means "All of the languages," could we scrap this second one that's specific only to the language_list() function?
(totally minor: there should be spaces around ? and : here, per coding standards)
There's a few places where we're manually specifying an array of these "special" constants. Since we went from 1 -> 3 in D8, and could conceivably (maybe) go from 3 -> 4 later in D8 or D9, should we add a simple helper function instead so if we later expand/contract this list we can do it in one place?
This in particular was confusing, and I asked Gábor about it, because I *thought* that the point of the LANGUAGE_ADD_LOCKED constant was to get me the list of languages, including the locked ones. He pointed out that language_list() does different logic depending on if the language system is loaded or not, and the function to figure out the special languages is part of Language module which may not be enabled on a given site.
So I suggested...
Ah-ha. Here's the helper function I was looking for above. :)
So that this is available everywhere, it *probably* makes sense to move it to bootstrap.inc alongside language_list() (I can hear chx beating me in the head with a club suggesting adding more code to bootstrap.inc, but it honestly doesn't make sense to keep these decoupled.)
Comment #110
vasi1186 CreditAttribution: vasi1186 commentedAttached a new patch that should solve the issues from #109. The main changes:
- get rid of the LANGUAGE_ADD_LOCKED and use LANGUAGE_ALL instead.
- move the language_special_languages() function from language.install in bootstrap.inc and replace the duplicated code from update.inc and language.install to use this function.
Comment #111
Gábor HojtsyThis is not used in this version of the patch in place of real language codes anymore (due to the new UI patch landed), so this todo is outdated, it does not matter.
Now that it is just one flag, and we are not foreseeing more (since disabled languages are gone), I think we want to name this $all with a default of FALSE or something like that, and have LANGUAGE_ALL possibly defined as TRUE.
No disabled languages anymore.
Well, to make sure this will always work, let's have a $weight argument on this function and count from there. Pass in the existing max weight from the outside, since it can be different based on the circumstances (so it works best all the time). Sites can have 20 languages (localize.drupal.org has over a 100 :). I'd hate to need to hack core to make it work :)
Naming for this function seems misleading, since we have exactly one specific language that is named not applicable. So maybe name this language_is_locked or something :)
"default to enabled" is not a relevant comment here, that already happens elsewhere.
I don't think we need to include any of these comments here anymore, the code under it is very descriptive with language_list(LANGUAGE_ALL) in itself.
Strange terminology :) ..._special_languages() as $...not_applicable. They are really the locked languages. There is only *one* not applicable language so to speak.
debug
There are no disabled languages anymore.
Comment #112
vasi1186 CreditAttribution: vasi1186 commentedAttached the new patch and an interrdiff between 110 and 112.
Comment #113
Gábor HojtsyWe said this would be TRUE :) It is not used anywhere else in core after your previous patch, right? :) So it would not make sense if we apply this patch, right? See below too.
Go until 80 chars, don't need to wrap so early.
No, I think we agreed that we keep LANGUAGE_ALL as TRUE, so the invocation pattern is clear, and people are not wondering WTH is TRUE there. Same everywhere.
Comment #114
vasi1186 CreditAttribution: vasi1186 commentedOK, let's see this one then.
Comment #115
Gábor HojtsyJust one note now: one empty line too much at the end of code docs.
Comment #116
vasi1186 CreditAttribution: vasi1186 commentedRemoved the line.
Comment #117
Gábor HojtsyShould be RTBC based on all reviews and the previous one being green then.
Comment #118
webchickAwesome! Even better than what I asked for. :)
Committed and pushed to 8.x. Thanks!
Comment #119
vasi1186 CreditAttribution: vasi1186 commented#116: 1471432-116.patch queued for re-testing.
Comment #121
vasi1186 CreditAttribution: vasi1186 commentedComment #122
Gábor HojtsyQuick followup: #1637710: Pass on language weight to language_locked_languages() when available.
Comment #123
Gábor HojtsyAnother followup issue: #1640050: Locale summary in language table should be "not applicable" for locked languages
Comment #124
Gábor HojtsyAdded a changelog entry at http://drupal.org/node/1647438.
Comment #125
Gábor HojtsyAn important followup: #1649400: Make locked/special languages fully extensible.
Moving this off the sprint board as it is done as-is.
Comment #126.0
(not verified) CreditAttribution: commentedAdded more notes on why http://drupal.org/node/1314250 would be useful.