Rename global $language to $language_interface

mmm, "ui" is yet another abbreviation. :-/

#1216094: We call too many things 'language', clean that up could have used some more discussion, agreement, and buy-in, before attempting to create a patch, IMHO. But anyway...

I currently see:

/**
 * The type of language used to define the content language.
 */
define('LANGUAGE_TYPE_CONTENT', 'language_content');

/**
 * The type of language used to select the user interface.
 */
define('LANGUAGE_TYPE_INTERFACE', 'language');

/**
 * The type of language used for URLs.
 */
define('LANGUAGE_TYPE_URL', 'language_url');

So the most logical thing would be to have:

/**
 * The type of language used to select the user interface.
 */
define('LANGUAGE_TYPE_INTERFACE', 'language_interface');

and thus, $language_interface.

Coming back later

I reviewed the patch and aside from the stupid thing below it looks ready to go. The bot is doing a far better job at testing it than the one I could do myself. We are still missing an update function, however. About this I found another variable that should be updated: locale_language_providers_weight_language should become locale_language_providers_weight_language_interface.

+++ b/modules/locale/locale.module
@@ -637,11 +637,11 @@ function locale($string = NULL, $context = NULL, $langcode = NULL) {
+  // cache after $language_interface has been set to avoid an unnecessary cache rebuild.

Comment is not wrapping at column 80.

+++ b/modules/locale/locale.install
@@ -262,3 +262,30 @@ function locale_schema() {
+    $types['language_interface'] = $types['language'];
+    unset($types['language']);

I'm a bit worried by this: at a first read it seems that the original key order is not preserved. Since the key order determines the order in which languages are initialized, and content language inherits from interface language, this should lead content language to get an empty value. I guess the bot is not failing because we have no test coverage for D7 > D8 upgrade yet.

+++ b/modules/locale/locale.install
@@ -262,3 +262,30 @@ function locale_schema() {
+  if (!is_null($setting)) {

Why is_null and not $setting === NULL which should be faster?

29 days to next Drupal core point release.

Now it looks good :)

I'll test the upgrade path myself as soon as I can.

um, did you guys already hear about PHP's latest awesome function http://www.php.net/manual/en/function.isset.php ?

Never will set language_types to $new_types due to misspelling in

if (!empty($types) && isset($types['lanuguage'])) {

Cheers!

@sun:

yes, I read something about it, not that good after all ;)

We are almost there but not yet: without the fixes below the update won't work.

+++ b/modules/locale/locale.install
@@ -262,3 +262,33 @@ function locale_schema() {
+function locale_update_7000() {

This should be

locale_update_8000() {

Moreover I realized that also the interface language switcher block delta must be renamed from 'language' to 'language_interface'.

Additionally while running the update, if language negotiation settings are present, notices are thrown all around since the global $language_interface is not initialized yet. These can be fixed by adding the following line to update.php (see also the attached patch):

<?php
$GLOBALS[LANGUAGE_TYPE_INTERFACE] = language_default();
?>

29 days to next Drupal core point release.

Yes, I only made the minimal fixes needed to make the upgrade run smoothly. This way I can review the final patch :)

I tested #27 with the fix below and everything looks good. If we need upgrade path tests this is NW otherwise if they need a separate issue this one is RTBC.

+++ b/modules/locale/locale.install
@@ -262,3 +262,46 @@ function locale_schema() {
+      ->condition('module', 'block')

The 'module' column should be 'locale' :)

22 days to next Drupal core point release.

I guess this needs to be done soonish.

#28: language-1222194-28.patch queued for re-testing.

This was ugly to reroll. Hope I didn't miss anything.

+++ b/core/modules/locale/locale.install
@@ -207,6 +207,49 @@ function locale_schema() {
+function locale_update_8000() {

Wrong update number. This should also go in the proper PHP doc group.

20 days to next Drupal core point release.

There were a couple of missing places.

(Hopefully) The last fix.

Even if this passes I think we need update testing for it which would include some of the values touched in the update and ensure it updates properly.

I ain't sure about this, I think tests would fail if we messed something up in the update function. Let's try without locale_update_8001.

Nice!

However it seems that block upgrade is not test covered, I think a simple assertion about the presence of the language switcher in the page would be enough, right?

drupal-7.language.database.php does not currently have it enabled though, so that would need to change too

Aw, I saw some activity around it in the latest patches. Is there a resource or something I can read to learn the proper way to create/alter the DB dump?

Oh, wonderful :)

Testing again the upgrade path coverage.

Ok, the results look good now.

This no longer seems to apply and may need a quick re-roll.

vortex:drupal-head dries$ git apply ../f.patch 
error: patch failed: core/modules/locale/locale.module:745
error: core/modules/locale/locale.module: patch does not apply
error: patch failed: core/modules/locale/locale.test:1101
error: core/modules/locale/locale.test: patch does not apply
error: patch failed: core/modules/path/path.test:251
error: core/modules/path/path.test: patch does not apply
error: patch failed: core/modules/simpletest/tests/upgrade/upgrade.language.test:40
error: core/modules/simpletest/tests/upgrade/upgrade.language.test: patch does not apply
error: patch failed: core/modules/translation/translation.test:179
error: core/modules/translation/translation.test: patch does not apply

#54: language-1222194-54.patch queued for re-testing.

Give this another review and it still looks great. No more lingering around; committed to 8.x. Thanks Gabor.

Dries, if you use single quotes the commit messages won't get messed up. Or, if you have to use double quotes because of apostrophes, don't use `git commit -m` and let it drop you into an editor.

This
- Patch #1222194 by Gábor Hojtsy, plach: Rename global $language to $language_interface().
Became
- Patch #1222194 by Gábor Hojtsy, plach: rename global to ().

Update function for the d8 site with 3 languages are failed for me

locale module
Update #8001
Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'bartik-locale-language_interface' for key 'tmd': UPDATE {block} SET delta=:db_update_placeholder_0 WHERE (delta = :db_condition_placeholder_0) AND (module = :db_condition_placeholder_1) ; Array ( [:db_update_placeholder_0] => language_interface [:db_condition_placeholder_0] => language [:db_condition_placeholder_1] => locale ) in locale_update_8001() (line 273 of /var/www/core8/core/modules/locale/locale.install).

Suppose this caused that I've visit site before running update and blocks already been rebuild so I have duplicates

Suppose better to check is there any blocks and clean-up possible auto-created rows

+++ b/core/modules/locale/locale.install
@@ -264,13 +264,20 @@ function locale_update_8001() {
+    if (db_query_range("SELECT 1 FROM {" . $table . "} WHERE delta = :delta AND module = :module", 0, 1,
+      array(':delta' => 'language', ':module' => 'locale'))->fetchField()) {

Please, can we have a separate line for the query to improve readability? Moreover a comment to explain why we are performing this check could be useful too.

+++ b/core/modules/locale/locale.install
@@ -264,13 +264,20 @@ function locale_update_8001() {
+      db_update($table)
+        ->fields(array(
+          'delta' => 'language_interface',
+        ))
+        ->condition('delta', 'language')
+        ->condition('module', 'locale')
+        ->execute();

If I'm not missing something the update should be outside the if statement as it should always be executed.

7 days to next Drupal core point release.

If I'm not missing something the update should be outside the if statement as it should always be executed.

I suppose that if there's no locale's blocks so no update needed

I suppose that if there's no locale's blocks so no update needed

Right, I misread the test. It should really be on a separate line.

Added comment for db_delete()
db_query() changed to one line as all over core we have

Previous patch was wrong

Cleaned up comments and readability, otherwise untouched.

@Gábor Hojtsy it seems block_rehash still bogus.

Steps to reproduce

- git checkout edabf1fc3add074b9818b6fb125e287f6091ab2c
- install core, drush en locale
- git checkout a58940f111b76df8141eaa9a8096ef087dd2944c
- visit admin/structure/block/list/seven and admin/structure/block
drush sqlc
mysql> select * from block where module='locale';
+-----+--------+--------------------+--------+--------+--------+--------+--------+------------+-------+-------+-------+
| bid | module | delta              | theme  | status | weight | region | custom | visibility | pages | title | cache |
+-----+--------+--------------------+--------+--------+--------+--------+--------+------------+-------+-------+-------+
|  31 | locale | language           | bartik |      0 |      0 | -1     |      0 |          0 |       |       |    -1 |
|  32 | locale | language           | seven  |      0 |      0 | -1     |      0 |          0 |       |       |    -1 |
|  33 | locale | language_interface | bartik |      0 |      0 | -1     |      0 |          0 |       |       |    -1 |
|  34 | locale | language_interface | seven  |      0 |      0 | -1     |      0 |          0 |       |       |    -1 |
+-----+--------+--------------------+--------+--------+--------+--------+--------+------------+-------+-------+-------+
4 rows in set (0.01 sec)

OK I asked Gabor in IRC why the updated didn't come with tests, but it's apparently only possible to hit this if you hit your site while also upgrading. The change is small enough and the bug obscure enough that I've gone ahead and committed this.