Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Locale module's language object listing and single loading API is quite dated, and it does not conform to recent ways used anymore to load single and multiple objects. Functions that are non-standard and would be involved in this cleanup are:
http://api.drupal.org/api/drupal/modules--locale--locale.module/function...
http://api.drupal.org/api/drupal/modules--locale--locale.module/function...
http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/langua...
Maybe even: http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/langua...
The main challange here is that right now we have this API kind of duplicated with language_* and locale_*, the later only available if you have locale module enabled. Ideally this new language load API would be unified and optionally work with locale module if its enabled, not duplicate similar code at both places.
For inspiration on how to do this, see http://api.drupal.org/api/search/7/load_multiple for other standard ways this is applied.
Parent issue
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 1260510-no_multiple-23.patch | 5.11 KB | Gábor Hojtsy |
| #17 | 1260510-no_multiple.patch | 5.12 KB | Berdir |
| #15 | 1260510-ll_5.patch | 5.57 KB | Berdir |
| #9 | 1260510-ll.patch | 7.89 KB | andypost |
| #7 | 1260510-ll.patch | 7.87 KB | andypost |
Comments
Comment #1
Gábor HojtsyTagging.
Comment #2
andypostInitial simple patch
Should we re-factor language_list() in flavor of object oriented storage?
1) Old array storage still easy to serialize into variable 'language_default'
2) _load_multiple() should introduce $conditions and caching
Comment #3
Gábor HojtsyLooks like although you support $langcodes on the multi-loader, you only return the last one found?
Comment #4
andypostChanged to use language_list() from bootstrap.inc
_load is a wrapper because language list() has a lot of usage all over a core with different arguments
This patch also removes a call in validate function, not sure we need to query each time to validate values for language edit form
Comment #5
andypostAnother 2 queries been removed
Comment #6
andypostAnother 2 queries been removed
Comment #7
andypostlast ones places are ported to language_load()
We need drupal_static_reset('language_list') because data has been changed, it seems that a bug in current implementation
Added separate issue #1261212: Static cache for language_list() is not cleared in _locale_rebuild_js()
Comment #8
Gábor HojtsySven figured out that the default property is not added to the object when it is loaded. The default property is actually derived data we have on the language object, and do not actually save it in the language table. Maybe we should save it in the language table eventually, but for now I think we should add the property to the language object after it is loaded (to all objects either FALSE or TRUE). We found this at http://drupal.org/node/1260520#comment-4911622
Comment #9
andypostpatch been in wrong format
Temporary set status for review by bot
Comment #10
andypostback to cnw because we need to decide on default language - use variable or change a schema to add this
Comment #11
Gábor HojtsyLet's do what we did so far, use variable. This is a big enough change in itself, let's not overload with all kinds of other nice to do stuff. I agree we need to revisit default language storage, we can do that separate.
What other piece do you think we are missing here?
Comment #12
lambic CreditAttribution: lambic commentedThis patch looks like a great start to me. Some other things to consider:
locale_language_name() should either be removed (I don't see it used anywhere) or rewritten to use language_load().
language_list() and locale_language_list() are kind of ugly but I'm not sure what the best approach is to improve them. I considered merging them, but they're a bit too different.
It might be worth abstracting the db_insert(), db_update() and db_delete() calls out to locale api functions to make schema changes easier in the future.
The queries on locale_sources and locale_targets should also be abstracted, but there's not much commonality between the queries I've seen, and those tables probably need to be reworked anyway.
Comment #13
Gábor Hojtsy@lambic: for the locale data storage API itself, #361597: CRUD API for locale source and locale target strings is our issue for that. The goal of this issue is to avoid having the custom language_list() and locale_language_list() stuff and have a load API that is consistent with the rest of Drupal (hopefully). So this issue is entirely for languages, not UI translations.
Comment #14
Gábor HojtsyI've just marked #1311582: Refactor language_list() a duplicate. Can we somehow limit the scope of this patch to be able to continue? It seems to be stalled for trying to solve too much at once, which was probably my mistake, sorry... Let's just do a language_load() for now since there are plenty of places to use that and the patch started off in that direction pretty well.
Comment #15
BerdirAttaching a re-roll of the last patch. The validation parts in locale.admin.inc have been reimplemented without relying on sql queries and the other part was related to code moved to gettext.inc.
I have not made any other chances as I wasn't able to find any direct queries to the languages table anymore.
Note sure what to do about locale_language_name(), I found two usages in i18n_menu/taxonomy for 7.x but nothing in 8.x and the whole function is quite weird as it returns t('All') if the language is not defined... I suggest a follow-up issue.
Comment #16
Gábor HojtsyThanks! Let's drop the language_load_multiple() for now in this patch and we can work towards trying to get rid of / unify language_list() and locale_language_list() in another issue I guess. This is getting to be good as-is without that :)
As for the locale_languge_name(), it is being used via module_invoke() in node and path modules for display of language names in the overview table based on whether locale module is enabled. I think that probably needs to be moved to "system" level like language_default(), but we can figure this out step by step IMHO, not sure anymore that we want to cover it here in the interest of making progress instead of trying to solve all problems at once...
So all-in-all, let's drop the load_multiple for now...
Comment #17
BerdirFine with me.
Comment #18
Gábor HojtsyI think this looks beautiful as-is. We should apply this to other areas as we go, but its already extensive and due to to the chance of conflicts with other ongoing patches, IMHO we should get this in straight and improve on it as we go. It is great to see Drupal standard APIs being applied to languages. :)
Comment #19
Gábor HojtsyFor the "OMG why is this not called locale_language_load() like the rest of the locale_language_*() CRUD", we are getting very close to have language module on its own, so by not using a temporary name here, we just make our life easier in the patch at #1301040: Move language listing functionality from locale.module to a new language.module. So IMHO we should get this in as-is and not obsess with the temporary inconsistent naming for now.
Comment #20
Gábor HojtsyAlso discussed with @chx that this patch has no tests in itself. We discussed that now that the menu system uses %language => language_load(), and we do test editing and deleting languages, and those functions now get the language object as returned by language_load(), those tests would fail if this function did not work, so we have plenty of places where we test this works indirectly.
Comment #21
catch#17: 1260510-no_multiple.patch queued for re-testing.
Comment #23
Gábor HojtsyRerolled to apply again. No changes.
Comment #24
Gábor HojtsyComment #25
catchThanks! Committed and pushed to 8.x.
Comment #26
Gábor HojtsyThanks, I've added the change notice http://drupal.org/node/1323148 for this.
Comment #28
Gábor HojtsyTagging for base language system.
Comment #28.0
Gábor HojtsyAdded parent.