Introduce locale_language_save()

Sure, its an API design question :) Some modules do alter, others do presave. Agreed presave might make more sense here.

All right, here is a little update:

- some test files in locale.test and field.test still used locale_add_language(), fixed those, this should hopefully make it pass now :)

- realized using $language for these can be dangerous, we have the global $language, and should probably avoid using $language generally for other purposes... that would be a huge cleanup in locale module though... for now used $language_obj in the patch at places invoking the API, better suggestions welcome

- added a presave hook and made is_new a property of the object, so the presave hook knows if the language was new or not

- added logic to handle default language change in the API, so it updates the default language too if a new one is to be set the default

Ups, one inadvertent edit caused test fails. Removing that part.

BTW opened #1216094: We call too many things 'language', clean that up for the $language an an object vs. $langcode vs. $language as a string vs. $language as a non-global vs. $language_obj that looks ugly problem.

@sun: well, logically I agree. However, language.inc is full of API functions which can be used on a site that does not have locale.module enabled, while locale.inc only has code to be used if locale module is on. (I do think it is a very bad idea to have module specific code in Drupal's main "includes" and want to solve this for Drupal 8). However, that would also mean that this will not get into language.inc after all.

We've just talked briefly in IRC about moving the language configuration parts of locale module (vs. the UI translation and gettext support) to a language.module. Then we'll own the language_* namespace both in terms of a module providing stuff for that and language.inc. This pattern is used elsewhere for API functions that should work without modules. Such as the filter_* functions that are outside filter.module. I think then it would make much more sense to have language module API functions in language module, would it?

I've documented how this fits to overall Druapl 8 plans at http://groups.drupal.org/node/161589.

Any more comments on this? I don't know of any I have not acted on yet. So what do you think is holding this up to get in?

@sun: no, existing clashes are there between global $language and this $language. So I think we should at least migrate away from global $language as a pre-requisite to this issue. I've opened #1222194: Rename global $language to $language_interface to make that happen at least. Then we'll still need to figure out the language identifier problem in #1216094: We call too many things 'language', clean that up so we can actually make a _load() operation happen. I entirely agree with $language_obj is lame, that was totally a heads-up to work around global $language.

To make sure not to branch the conversation, marking this postponed on #1216094: We call too many things 'language', clean that up.

Here is an update to the patch based on discussion in #1216094: We call too many things 'language', clean that up. Until/unless #1222194: Rename global $language to $language_interface is committed, the best we can do to eliminate conflict with the global $language is to use $GLOBALS['language'] (which Drupal core already does at places), so applied that. Where $language was used to reference language code in the tests, I've updated to say $langcode. That had ripple effects for other related variable names, so applied to those too. So the attached patch applies the current short term best practice.

Additionally to that, I've made the following changes:

- removed the group definition for this single function; discussed with jhodgdon and sun in IRC that most of the locale groups don't make sense and we'll reintroduce a group as it makes sense on a larger scope when we have stuff to explain... the bigger API is still forming
- added API docs for the three new hooks, language presave, insert and update; added note to the mutlilignual settings changes hook that it is going away soon

Retitling the issue to keep the task more focused, given that it turns out that implementing a full CRUD is way bigger then what fits comfortably in an issue. I'm hopeful we can move incrementally from here and implement a sensible loader next to be able to do the update and delete parts as well. Currently both update and delete are tightly baked into the Form API. Fun, yeah.

Therefore I expect the full use of this function as well as the removal of the multilingual settings changed hook in a followup.

This should be reviewable and committable as-is now without dependeny on any other issue.

The checkUrl() change was incorrect, url() takes a language object in its options key, not a langcode (like t()). Ugh. Fixed that which should fix the tests.

Patch applied with offsets. Rerolled. Also included a little fix for the phpdoc on the new function. I think this looks good, please review! Thanks!

Looks like a newly added test portion that was not yet there in the previous roll caused this to fail. Converting that too to the new API. Please review so we can get this in and move on :)

Yes, that. We have #1231402: Drupal does not use ISO language codes, iso.inc is misleading which has a similar problem but we could not yet figure out how to name things here separately really. I think it will be the job of followups to #1216094: We call too many things 'language', clean that up once we figure out what is a "language" :) This patch IMHO is already pretty sizable and we can do the overall cleanups once we reach conclusions on language terminology.

@plach: I think @sun et al would want to cut down on system module, and move its responsibilities to separate modules instead of tacking all that stuff together, so "bloating" system with the language list sounds counter to the goals of #1224666: [meta] Unofficial Drupal 8 Framework initiative (which Dries yesterday says he considers making official). We can obviously then following that thinking create a language module (which you and I disputed elsewhere for usability reasons).

All-in-all I think its better to introduce these things now even if moving them to their own module/place we keep on our radar. I think conceptually and according to the #1224666: [meta] Unofficial Drupal 8 Framework initiative ideals, language.module would be the place eventually (where most if not all of the language_* namespace would migrate as well). My approach to this work is as always faster, smaller and reviewable chunks, so we can fire and move on quick. That of course means the chunks should make sense on their own, but also means we don't need to solve every concern at once :)

BTW this problem is even more apparent with #1260510: Introduce a language_load($langcode) and also with #1260528: Introduce API to delete languages.

@franskuipers: definitely not going to happen in this issue. This is about the problem of no standard whatsoever with other APIs when it comes to adding or updating languages and no way to hook into the process. That is it. We do not manage country lists or number formats on these admin pages. Once those have UIs and concepts in core, we can think about merging or not merging them. This has been discussed elsewhere as well, if you want to read up on those discussions: http://groups.drupal.org/node/161589 as well as #1216094: We call too many things 'language', clean that up both of which should be very fun and insightful reading.

@svendecabooter: well, they definitely did not disappear, they just moved to other places. Did you keep the pieces for moved-around code?

The changes look good to me, and it indeed spreads use of the save function as far as it can in the locale admin API. It also deprecates the multilingual_settings_changed which was part of the goal. Final removal can happen with the delete API linked.

Here is an update that simplifies some property logic a great deal and fixes the remaining test fail by dropping our magic to update the old default language from the save function. That was a side effect of the function that caused problems elsewhere, namely in the form submit function that used more up to date data and then was overwritten with the cached data from language_default(). We should avoid that and let the caller deal with updates with up to date data. Therefore the removal of that magic. This should be cleaner and actually pass. :)

Good to go then I guess? :) The latest patch is just a merge of the RTBC patch from above and some application of the code to other places with testbot and reviewers happy.

With the language delete patch from #1260528: Introduce API to delete languages, this will not apply anymore. @svendecabooter, can you help with a reroll?

Agreed. Cross-checked with the delete patch and looks like we are on track to finish the plan to introduce real APIs for save/update/delete for languages with this one. Should go in.

Please commit.

Rerolling. Other patches are waiting on this being committed, so marking major.

Same patch rerolled for the latest predefined language list changes.

Thanks for the review. We discussed the first comment in IRC. The basic summary is follows:

We tack on the ->default property to languages as the locally cached $language->langcode == language_default()->langcode value. This property is not saved to the DB however. We probably want to rework that in the future,and there are various issues around default language handling that would be good to clean up. Our basic problem is that Drupal needs to support language even without locale module enabled and needs to support it differently once locale is enabled. The issue for default language handling improvement is at #1272862: Clean up default language handling.

Notifications on default changes are already achieved, since when the _insert or _update hooks are called the global default is not yet updated, so you can check if the ->default property is TRUE but != language_default()->langcode, in which case you see the new default language. You can update your records accordingly. All this is only relevant for contrib modules, no core use of this. Since we have this functionality in, I've removed the comment.

We've been discussing the cache problem on IRC extensively and could not figure out a sane way to abstract that out, so on request, opened an issue at #1293252: locale_language_insert() and locale_language_update() should only have local cache side effects and added a @todo to the code to figure that out.

Here is a reroll with these (minimal) changes.

Tagging for base language system.

Add parent.