Problem/Motivation
In Drupal 7, the language names in the language list show up as localized to their native names. In Drupal 8 native names don't exist anymore as a concept, however language names are translated to any other language just as well as any other configuration entity. We should restore the behavior of the language switcher block to use the localized language name for that language through a specific language context.
Proposed resolution
Make language list display in language switcher block use the localized language names by loading the language objects in the specific language context from config. This is the same as the Drupal 7 behaviour.
Remaining tasks
Review. Commit.
How to test
- Install drupal 8 with default (English) language + standard profile.
- Enable language & configuration translation module
- Add one or two language from admin/config/regional/language/add
- Go to admin/config/regional/language and select 'translate' from the dropdown of the language you want to translate.
- Assign language switched block to 'sidebar first' region
- Now you can see the localized name (i.e. translated string).
User interface changes
- Language names in language switchers are localized to their respective language (if translation available)
API changes
None.
Related Issues
- #1498880: Theme language switcher for seven theme
- Follow-up: #2135379: Make the language switcher configurable and extensible
- Follow-up: #2232375: Make language switcher block cacheable
Comment | File | Size | Author |
---|---|---|---|
#148 | 2107427-language-switcher-labels-148.patch | 10.78 KB | Gábor Hojtsy |
#145 | 2107427-language-switcher-labels-145.patch | 10.8 KB | Gábor Hojtsy |
#145 | interdiff.txt | 585 bytes | Gábor Hojtsy |
#143 | 2107427-language-switcher-labels-143.patch | 10.8 KB | Gábor Hojtsy |
#143 | interdiff.txt | 585 bytes | Gábor Hojtsy |
Comments
Comment #1
penyaskitoComment #2
penyaskitoUse config contexts for localizing the labels, like we used at the Date localization clean-up in Drupal/Core/Datetime/Date.php dateFormat method). If locale is not enabled we will show the English language names. Not sure if this is desirable.
For the tests, I needed an example of managing config files for testing that I found at the config_translation module (ConfigTranslationUiTest). The tests needed enabling locale.
Comment #4
penyaskitoComment #5
Gábor HojtsyFunctionally looks good, thanks! Now only some code organization :)
I think this should be a utility function (or an option to language_list(), which should maintain a cached list of languages with localized names as well). It looks odd that we'd need to avoid language_load() or any entity method for that matter to do this, but the sole reason is language_load() is dependent on language_list() which caches original entity values, right?
Add phpdoc on this :)
Comment #6
penyaskitoReduced scope for sticking to the regression, moved improvement proposals to #2135379: Make the language switcher configurable and extensible.
Comment #7
penyaskito#5.1 Yes, the issue is caching. I'm not sure how do we want to maintain the localized names in every language. drupal_static an array of arrays with the full language objects? Caching labels separately? Caching full objects multiple times where the only difference is the label?
BTW language_list() is caching in request negotiated language, so not sure what happens if we need different language labels in the same requests if we don't use overrides with language contexts there. I think this is something that won't happen because we will use language contexts e.g. sending mails in different languages, but maybe is worth to consider something different.
#5.2 Not addressed yet.
If the attached is the way forward, we need to update language_list docblock too.
Comment #8
penyaskito#5.1 I'm not very happy with the final implementation but this includes caching of both translated and untranslated languages list.
#5.2 and language_list docblock have been fixed.
Comment #10
penyaskitoTests failed/passed as expected.
Comment #11
Gábor HojtsyI agree this is not very elegant, but not sure how best to do it. We can introduce yet another function that does similar things... or refactor part of the logic to another function and then #1862202: Objectify the language system would refactor that again. I'd invite reviewers from #1862202: Objectify the language system :)
Comment #12
penyaskitoI'm tempted of asking for an RTBC here for the sake of the added test and care about separating to two different methods in LanguageManager in #1862202: Objectify the language system, where we could add a private abstract method for avoiding code duplication, which I don't know how to avoid here if we split in two different methods.
Comment #13
penyaskitoAfter chat with Gabor, we think that we should postpone this on #1862202: Objectify the language system
Comment #14
Gábor HojtsyTagging with a new tag for tracking config context related issues.
Comment #15
penyaskito#1862202: Objectify the language system landed!
Comment #16
penyaskitoThe interdiff is only of the tests, for the rest of the patch I started from scratch. I'm not sure if this is the way to go, but tests are green.
Comment #18
penyaskito16: 2107427-language-switcher-labels-16.only-test.patch queued for re-testing.
Comment #20
Gábor HojtsyIt sounds problematic to me to change the default behaviour of the language list without even an option to go differently or another way to get the language list. I'd add an option and use that for the switcher, not change the listing method wholesale to return the localized language all the time.
Comment #21
estoyausenteComment #22
penyaskitoComment #23
szato CreditAttribution: szato commentedSorry, we forgot to re-assign the issue. We have a solution, please review.
Comment #24
csg CreditAttribution: csg commentedI have tested the patch in #23, and it works. The code looks good too, apart from some seemingly unnecessary blank lines.
Comment #25
szato CreditAttribution: szato commentedBlank lines removed.
Comment #26
gloob CreditAttribution: gloob commentedComment #27
dermarioThe patch a applies an language switcher seems to work as proposed.
Comment #28
penyaskitoI think Gábor's comment #20 is still an issue, and adding an undocumented name_native property to languages that could or not be set is not a good thing for DX.
Comment #29
Gábor HojtsyI think the proposed patch is good for continuing with the discussion :) So we have at least three use cases for languages:
1. Installer language selector, that is already covered with the predefined languages.
2. Need to display a language list translated to the interface language. Happens if you are editing a block, creating a node, etc. The language selector should contain all language names in the interface language.
3. Need to display the native name of all languages. Occurs in the language switcher block (only).
This issue is to cover 3. (1) already works well. And then we have issues for 2 eg. #1879930: Language selectors are not showing localized to the page language. For both (2) and (3) the cache nature of the language list is an issue. Because the language list is cached in the form first found. So the proposed patch above resolved that problem for (3) by adding this "new" native name property (similar to how it is in Drupal 7).
There are countless other ways to solve these. Eg. alternately we can also introduce a method to get the language list localized and have it support (2) and (3) and cache for (3) and each variant of (2) separately, so we get the right localized versions. I guess we should pick a way and then get it in :) We can go around in circles with different approaches definitely :)
Comment #30
penyaskitoI don't have a really strong opinion on any of the options, but let me dump my feelings here.
Native name is better DX++ for people coming from D7, but then we should add this to the Language class itself so its less obscure.
But in the other hand I think this would be incoherent with how we handle other translated config, so DX--.
If we are sure that we only need to display the native name of all languages in the language switcher block, then I will discard the flaggy last parameter in #7 and just add a getNativeLanguages to the LanguageManager for simplicity.
We are going around in circles, and I finished thinking that Gábor's first suggestion at #5 is the way to go. Sorry for the mess.
Gábor, do you still think the same?
Comment #31
Gábor HojtsyYeah. getLocalizedLanguageNames() with a 'param' of native or 'localized' to langcode (specify langcode as well) would be fine. Or put these on the getLanguages() as arguments directly (and cache there specifically).
Something like:
Or the same as an extension of getLanguages().
Comment #32
penyaskitoComment #33
penyaskito@szato: I noticed that your patches lacked the test modification added in previous patches. I'm not sure how this happened to you.
Comment #34
penyaskitoThis grew more than I expected.
After discussion with Gábor on IRC, I don't find that switching depending on a parameter is a good idea, so I added a getNativeLanguages() method to the LanguageManagerInterface. The code there is mostly the same than at #16, without caching.
We use that method only on LanguageNegotiationSession::getLanguageSwitchLinks.
After changing that tests were failing, and the culprit was Language::sort. This calls SortArray::sortByWeightAndTitleKey($a, $b, $weight_key = 'weight', $title_key = 'title') with the default optional parameters. The title key is "title" when it should be "label" or "name". I found that we are not calling that passing the optional params anywhere, and by using uasort there is no way of passing that, so I just cloned it to sortByWeightAndLabelKey. Probably we need to remove those params if they are unused and refactor the duplicated code, but I would leave that for a follow-up.
Sorry for not providing an interdiff, but this is mostly a new approach than the patches above.
Comment #35
Gábor HojtsyYeah we can say the config factory does the caching of these translated entities, so we don't need to do the caching I guess here. The data derivative generation we do is not very expensive at all. I think we could use the getLanguages() return value though for the base objects and just adjust the label values to use native names to avoid some duplicate code (eg. default identification), but then we'd need to load the translated objects anyway. So yeah. I think here we need to pick one of many ugly options either way.
Just two code comment issues then:
"firstly" => "first" is correct English :)
Docs code style is not good.
Comment #36
Gábor HojtsyComment #37
penyaskito#35.1: This was copied from sortByWeightAndTitleKey. Searched for it and looks like "firstly" is an English word. See the Oxford dictionary.
#35.2: Fixed.
Comment #39
Gábor Hojtsy@penyaskito: still working on this? I think you said you know the fails :)
Comment #40
dclavain CreditAttribution: dclavain commentedThe translated language name is in the first position of the array instead of the zero position.
Comment #42
penyaskito@dclavain: The problem is somehow at the ordering. The failing tests are because we expect "English|Francais" and we get "Francais|English". Initially, this was because we were ordering with sortByWeightAndTitleKey, which title was not defined, and in #37 we added sortByWeightAndLabelKey, which used the label property which is the native language name.
We should debug that this is working properly and take into account that maybe the weights could be different (we expect them to be equal) and this is the root of failures.
Let me know if you want to finish it or if should I give a try.
Comment #43
Gábor Hojtsy@penyaskito: looks like it would be great if you could give this a try, the issue is still assigned conveniently to you :)
Comment #44
dclavain CreditAttribution: dclavain commented@penyaskito I think that the problem is in the test.
The ordering method is correct.
Comment #47
BerdirThe dance of language objects, language entity and config gets crazier every day :(
Can't we use the Entity API here? For example, execute an Entity Field Query to get the ID's, and then load entity objects?
We might not be able to inject that, but we also should not be messing with config entity data directly, so we lose anyway ;)
This could also be pretty slow, as changing the language multiple times is a bit ugly. So it's definitely a candidate for being uploaded upfront, there's a config issue related to that, and we should post there that the language config entities should definitely be added there, including translations.
Comment #48
dclavain CreditAttribution: dclavain commentedComment #49
Gábor HojtsyI don't get this change an all related test changes. How is this related at all? Has nothing to do with the language switcher block. It is not even the correct change AFAIS.
Comment #50
dclavain CreditAttribution: dclavain commented37: 2107427-language-switcher-labels-37.patch queued for re-testing.
Comment #52
dclavain CreditAttribution: dclavain commentedIn testLanguageBlock function I have changed the order of block creation and in getLanguageSwitchLinks function I translate name of language, this change is not necesary for pass test.
Comment #53
penyaskitoWhy is t() needed? The title should already be in the native language ($this->languageManager->getNativeLanguages())
Comment #54
dclavain CreditAttribution: dclavain commentedNo, if you add one language, in the switch language block the title isn't in native language. You need add translate file and clear cache for that the title is in native language.
Comment #55
Gábor HojtsyIn IRC @penyaskito said to unassign this. Also here are my review points. I agree t() should not appear in any added code here, the whole point of the issue is not to use t().
This does not really retrieve the storage itself, but the list of files with the given prefix instead.
Maybe:
// Retrieve list of languages directly from configuration storage.
Indeed the whole point of the native language names retrieval is we would not use t(). The translations of languages is in the config system not to be accessed with t().
In the first instance it is not using the native names method even.
I think I understand why is the indentation of this is as-is but it looks very odd.
Comment #56
YesCT CreditAttribution: YesCT commentedI'll look into this.
Comment #57
YesCT CreditAttribution: YesCT commentedaddressed #55
used #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines as a guide.
also formatted array_map like
core/modules/field/lib/Drupal/field/Tests/Views/FieldUITest.php
core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php
some other nits.
renamed translateLanguage since it was really saving a label in config, not for t().
Tried this manually. By installing in Spanish, but the Espanol does not show in the language switcher.
I cleared the caches.
Maybe the language does not "come" with the language entity label config file?
Also, I do not see an extra setting on the language switcher block.
I need to look more into this, and also get some help.
Comment #58
dclavain CreditAttribution: dclavain commented@YesCT for this reason I had added t in 'title' => t($language->name, array(), array('langcode' => $language->id)).
comment #54
Comment #59
Gábor Hojtsy@YesCT/@dclavain: translations of language names to other languages should be downloaded with the .po file in installation. If the language you installed with did not contain a translation for its own language name, then you can enter it using the Interface translation UI or the config translation module by translating the language itself to other languages (than English, which all languages originally are).
Comment #60
vijaycs8557: 2107427-language-switcher-labels-57.patch queued for re-testing.
Comment #61
vijaycs85Comment #62
vijaycs85Tested patch in #57 and looks like working as expected. Updated issue summary with 'how to test'.
one question is, current fix shows the language name in their own language no matter what site's current language. For example, French language in language block displays as 'français' for all language sites (i.e. /fr or /ta or /en). IMHO, 'français' should be displayed only on FR site (i.e. /fr). If this is correct, here is the fix for this. otherwise, use the one in #57.
Attaching screenshots of this fix.
Comment #64
Gábor HojtsyNo, if you are French and you are looking at a Spanish page, you would only notice French in French, not French in Spanish :) So the idea to have always the native names of languages in the block is exactly so people can recognize the right one based on the native name of their language they know. So all languages should be localized to their own language at all times.
Comment #65
vijaycs85Okay, in that case, +1 for making #57 RTBC
Comment #66
Gábor HojtsyAll right, this is definitely not the nicest code in Drupal 8 but looks like some of the hacks need to be done to make this effective and performant. So let's go with this then. We have been arguing for long enough and tried a whole host of other approaches before. Let's fix the bug for once :)
Comment #67
BerdirThis is a pretty slow operation that sites with a language switcher will obviously have to do on every page.
I don't think we can use block caching, as the blocks have different links on every site. Should have caching within the language manager for the labels? We have 2 config storage loads per language and they can not be preloaded as preloading will not be able to handle the language switch.
Comment #68
Gábor HojtsyI did not profile this. My gut feeling was the config factory will cache the lookups already, so it would be a pretty shallow cache. Did not profile it (once again).
Comment #69
mikispeed CreditAttribution: mikispeed commentedPatch from #57 marked in #65 as RTBC didn't worked against current HEAD. Here is the updated patch that makes it work with the changes from issue #2219499: Generalize language config override responsibility.
Comment #70
pp CreditAttribution: pp commentedI do the steps in issue description. Without the patch the language name is English. Whit patch the language name is translated.
Comment #71
rodrigoaguileraI applied the patch too and fix the bug but I don't know how to do profiling.
If someone can send some link to do this I'll be grateful
Comment #72
webchickIt sounds like this still needs profiling, so knocking back to "needs review" and tagging. If you think that is wrong, catch is right there in Szeged, so please ask him. :D
Comment #73
Aron Novaki am going to profile it
Comment #74
Aron NovakI did the profiling, i followed the "How to test" procedure in the initial post to configure the test site, then, i issued
drush cache-rebuild
executed apachebench three times with the following command,ab -n 20 http://d8.local/hu
. After that, i loaded http://d8.local/hu?XDEBUG_PROFILE twice in the browser as admin user. Then, i applied the patch, double checked that the issue is solved (yes), and repeated the procedure. I always used the 2nd (3rd) measurement. You can see the attached files.My relevant screenshots from KCachegrind:
Pre-patch
Post-patch
My feeling is that given the fact that we have 10 extra queries, it might need work.
Comment #75
Aron Novakone missing file from the previous comment
Comment #76
vijaycs85Just had a look at this additional calls through xdebugger. The additional calls are coming from Drupal\language\ConfigurableLanguageManager::getNativeLanguages() to Drupal\Core\Cache\DatabaseBackend::getMultiple for every language. This can't be optimised at this method level as the loading should happen in the language currently overridden. So setting it to 'needs review' to get feedback on how to move forward with this.
Comment #77
Gábor Hojtsy@vijaycs85: yeah well the question is would caching inside the method help, so further calls would not be a problem. I'm not sure that would help for 80% cases as there is likely one call to this method per request (only 1 block displayed). However having a cached list of native names somewhere (state?) that is cleared out if translations / languages change may belp. Not sure that much complication is worth it for the benefit, but we cannot see how much benefit in time that would have right now...
I think this is Aron's/webchick's/berdir's point.
Comment #78
Gábor HojtsyRetitled since there were some misunderstanding of what localized would mean.
Comment #79
Wim LeersThis is actually wrong, French is called "français", not "Français" in its native form. I.e. lowercased, not uppercased. Fixed.
Berdir asked me to chime in from a perf POV. We ended up having an IRC discussion:
So, you could make
LanguageBlock
implementgetRequiredCacheContexts()
and let it returnarray('cache_context.url', 'cache_context.language')
, and that would work just fine. The question is whether that's worth it: you'd end up with a lot of cache entries. (And that's in the case of the default language negotiation mechanism, if you use another one, you'll want to return something else there.)Comment #80
penyaskitoThanks all for the awesome profiling work and comments.
How do we move next? I'm with Gábor that this will be called only once per request in 80% cases, so the only improvement I see is moving the native list to state, but I don't think it is worth to do that.
Should we just move forward with patch at #79?
Comment #81
Wim LeersTo clarify: I don't think there's anything left to be done in this patch performance-wise, because doing so would significantly expand the scope of the issue.
Must-have performance improvement follow-up
However, I think a must-have follow-up would be to have a "language redirect" route, i.e.
/language-redirect/<language code>
. So e.g. "français" would link to/language-redirect/fr?destination=node/42
. Because then everything except for thedestination
parameter could be cached globally. Thedestination
parameter could be set in JS, just likecontextual.js
already does today:To remain JS-less for anonymous users, we could still generate the entire block dynamically for anonymous users, because Drupal assumes that anonymous users get served cached pages anyway. This would then be in line with what we do for active link handling: we set the
active
class in PHP (on the server side) for anonymous users, but in JS (on the client side) for authenticated users.That's the best of both worlds.
Comment #82
Gábor HojtsyWim: I'm not sure that followup would be so simple, especially with outbound link altering per language being a supported feature (eg. a translation of a node may be a view or a panel :D), but that is better debated in that followup :)
Comment #83
Wim Leers#82: I don't follow, but yes, let's discuss that in a follow-up: #2232375: Make language switcher block cacheable.
Comment #84
YesCT CreditAttribution: YesCT commentedI'm going to review this.
Comment #85
YesCT CreditAttribution: YesCT commentedAdding follow-up from #83 to issue summary
Comment #86
YesCT CreditAttribution: YesCT commentedneeded a reroll. it was an easy one. automatic 3 way merge with no conflicts. (still reviewing)
Comment #87
mashermike CreditAttribution: mashermike commentedYesCT dragged me into a room and asked me to make this patch. This is my first patch and drupal contribution so be nice... (please?)
The sorting function in the previous patch is not pertinent to this issue. It will be included in a separate one.
Comment #89
mashermike CreditAttribution: mashermike commentedOk, I did the diff in the wrong direction so the patch did not work. This version should work.
Comment #90
mashermike CreditAttribution: mashermike commentedChanging status to send the patch to the bot.
Comment #92
penyaskitoNice work, @mashermike!
This three failures looks familiar to me. It's because #2239399: Languages should be sorted by label instead of title is required for this one to pass. I'm postponing on that one.
Comment #93
YesCT CreditAttribution: YesCT commentedwe should have paid attention to berdir in #47
this is messed. but not the fault here. cause getLanguages() is messed.
this should just be protected, in case someone wants to subclass this. (not likely, but still)
head is silently doing things we dont expect and should be broken.
#2151223: Add a method to SortArray to sort by weight and title might get a revert, or an issue to fix it.
or an issue to fix getLanugage: #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages() [edit: added link to issue]
Comment #94
Aron NovakComment #95
YesCT CreditAttribution: YesCT commentedComment #96
Gábor Hojtsy#2239399: Languages should be sorted by label instead of title was just closed down as fixed/duplicate. Should unpostpone this one finally!
Comment #99
Gábor HojtsyThis will certainly not work anymore, you should now use something like:
Comment #100
penyaskitoSorry, no interdiff. This is mostly #89 with #93.2 corrected and following #99. We do have doTestLanguageBlockAnonymous() and doTestLanguageBlockAuthenticated() now, so I added assertions in both.
Green locally!
Comment #101
Gábor HojtsyLooks great. Only minor things:
Comment not true anymore. The storage is not directly involved anymore.
Looks like this can be done earlier, otherwise, we instantiated a language and then threw away immediately.
And by name :) Maybe don't add that comment just let the implementation speak for itself.
Wrong indent.
I don't think we double-indent like this elsewhere?!
Comment #102
penyaskitoHandled #101 points.
Comment #103
Gábor HojtsyLooks good now. Updated issue summary and looked at the followups as well. Not sure all are necessary, but we can discuss there. While loading all the localized language overrides is not nice, we could not think of nicer solutions unfortunately.
Comment #106
penyaskitoRTBC per #103, tests errors were because of a db connection issue.
Comment #109
penyaskitoComment #110
alexpottWe need to watch the performance of this. Can't think of a better way of doing it though. That said these links appear in a block that should be permanently cached after #2232375: Make language switcher block cacheable is done. So I think we should be loading the language entities here and not be side stepping that.
Comment #111
penyaskitoThis patch changes the strategy to load the language entities.
I had to change the negotiation classes as we still have some languages with name and some others with label. It will be cleaned up in #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.
Comment #119
penyaskitoRerolled (automatic 3-way merge)
Comment #120
Gábor HojtsyLooks like what @alexpott asked for :)
Comment #121
alexpottThis is unnecessarily complex - just loop the langcodes array and there is no need for $i - no?
Not used
Comment #122
penyaskitoRight. Much cleaner now.
Comment #123
Gábor HojtsyIf we want to API-ify all the things, then we can just as well do $languages = $this->getLanguages() here... Looks like @alexpott is not a fan of our microoptimizations here...
This comment is now outdated.
One dot should be enough here. I know this was copied over but...
Comment #124
penyaskitoLet's try this then.
Comment #125
Gábor HojtsyAll right, as simple as it can get now :) Thanks!
Comment #126
alexpottSo this patch is displaying some of the current problems we have because ConfigurableLanguage and Language don't share a common interface. They really should but that is not the fault of this issue. The suggestions below are a work around for this.
This is not correct at the moment since ConfigurableLanguage does not implement this however we should fix this by making the suggested changes below.
lets create language objects from the config entities here... $native->toLanguageObject()
Need to change this to use the language object sort
and now you can use getName()
Comment #127
penyaskitoI'm dying for #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.
This covers everything at #126.
Comment #129
penyaskitoAfter discussion with @alexpott, postponing on #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.
Comment #130
Gábor Hojtsy#2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface landed. @penyaskito: yours again :)
Comment #131
penyaskitoUsing Language::sort requires to reimplement it, as the ArraySort methods use properties instead of methods. This leaves SortArray::sortByWeightAndTitleKey() unused, so maybe we should remove it. At some point it was already considered, see #2151223: Add a method to SortArray to sort by weight and title.
Comment #132
Gábor HojtsyOh well... This at least means that the "use .... SortArray" in Language.php is now unused and should be removed. Otherwise I think this issue has been a land of compromise and not agreeing on said compromises, so we can just as well go with this approach (once the use is fixed) and see if we can agree :D
Comment #133
penyaskitoI always forget to review use statements...
Comment #134
Gábor HojtsyLets go with this one then :)
Comment #135
YesCT CreditAttribution: YesCT commentedsimilar to #2226533: Changes to the Language class due to the LanguageInterface (followup) this removes the only usage of sortByWeightAndTitle.
Ug. I read #131 but I dont why that sort change is needed for this issue though.
We can remove that method (and the test and dataprovider) in a follow-up. (or maybe, maybe, changes issue might get in first, but I doubt that.) #2332739: Remove SortArray::sortByWeightAndTitle
I tried this manually, there is one difference that was probably un-intended. The order a language is added in the language list used to be in the order by the english name of the language. Now, new languages are added always to the end of the language list. I'm not sure this was an intended change. Makes me wonder what weight languages get when they are added. Maybe before this patch they used to all get the same weight when added, so they got then sorted by title?
Did this change the weight when languages get added?
Comment #136
Gábor Hojtsy@YesCT: @penyakisto said we need to use our own sorting code because sortArray uses public properties to sort:
When casting to an array, only public properties will end up in the array.
Looks like both Language and ConfigurableLanguage have their label/weight and name/weight keys public though. The difference is Language has name while ConfigurableLanguage has label. So since @penyaskito attempts to use the sort method on Language to sort ConfigurableLanguage-es, the method cannot be used directly because the properties are named differently.
I think clearly introducing a sort method on ConfigurableLanguage would be better then. Marking needs work for this.
As for why the weighting of that list changes, that looks like a classic method of you saving the order of the languages at one point, from then on languages are not ordered alphabetically anymore. That could have happened before or after this patch either way, this patch does not change that behaviour, you just saved the order once.
Comment #137
alexpottHmmm... i think configurable and non configurable languages should be able to be sorted together. Especially now they have a common interface - perhaps we should postpone on #2332739: Remove SortArray::sortByWeightAndTitle
Comment #138
alexpottAlso actually I can not repeat YesCT's tests which show the languages always being added to the end. I suspect that during the test YesCT dragged a language on the language overview screen - this will change all the weights to be negative and therefore new languages will always be added to the end. This behaviour is completely unaffected by this patch.
Comment #139
YesCT CreditAttribution: YesCT commentedYes. I could not reproduce the order issue either. Sorry for the noise. (but glad to learn about the save effect)
Comment #140
Gábor Hojtsy@alexpott: While its great we may be fixing most language architecture issues before this patch, there are also others postponed on this one and this is not supposed to be that hard an issue to work on for a year and 140 comments. Probably a good example for https://twitter.com/webchick/status/507919029034835968 :/
Comment #141
YesCT CreditAttribution: YesCT commentedComment #142
Gábor Hojtsy#2332739: Remove SortArray::sortByWeightAndTitle landed. Unpostponing. I still think it would make most sense to sort a list of ConfigurableLanguage objects with the sort method on ConfigurableLanguage, that is most straightforward. In fact, they already have it properly inherited from ConfigEntityBase:
This is the behaviour we want. Also it did not need any of #2332739: Remove SortArray::sortByWeightAndTitle.
While we can keep using Language::sort() to sort ConfigurableLanguage objects, the two objects do not inherit from each other, so the best we could do is to ensure they have identical implementations with some tests ensuring that happens. #2334483: add test for sorting an array of languages that are Language and ConfigurableLanguage to show the sort() will work for configurable languages too. is about to add tests to ensure that the two in fact completely independent implementations on the two classes work identical. We have the option to postpone on that one (LOL) or just use the sort method on the class we are actually using :)
Other places eg. the admin config entity list will also order using the sort method on the config entity not some class that it only shares one of its implemented interfaces, so making use of the same sort method elsewhere makes most sense IMHO.
Comment #143
Gábor HojtsyRerolled. The Language.php change did not apply anymore due to #2332739: Remove SortArray::sortByWeightAndTitle doing the same change. The one line change for the sort included. Please review.
Comment #145
Gábor HojtsyLOL, this is too funny. The sort() method on Language takes an array Of LanguageInterface objects and returns the array sorted. The sort method on ConfigurableLanguage takes TWO objects and returns their relative order (to be used in a uasort()) like Language::sort(). Haha. Hahaha. Hahahahaha. Ha.
So we may either sort with the ConfigurableLanguage::sort as
(which eg. ConfigEntityListBuilder does as
uasort($entities, array($this->entityType->getClass(), 'sort'))
).Or do the less ugly looking
which was in the prior patch, although Language::sort() has even less to do with ConfigurableLanguage::sort() than we thought. Good thing about these small patches is we can talk about API design line by line.
Comment #147
penyaskito#145 is the same than #143. Applying the interdiff should fix it.
Comment #148
Gábor HojtsyRight, sorry, this should have been #145.
Comment #149
vijaycs85looks good. nothing to complain. let's get this in!
Comment #150
alexpottFinally :) Committed 67c8a34 and pushed to 8.0.x. Thanks!
Comment #152
Gábor HojtsyYay, thanks! I finally reopened #1879930: Language selectors are not showing localized to the page language which was postponed on this one 7 months ago.
Comment #153
Gábor HojtsyAmazing, thanks.