Move LANGUAGE constants to the Language class

Perhaps the variables should be better "scoped": what about Language::TYPE_(INTERFACE|CONTENT|URL)?

I mean this would leave room for migrating other constants that now live in boostrap.inc. I'm thinking about Language::NOT_SPECIFIED, Language::NOT_APPLICABLE and so on. We probably want to that here as well.

I think the main question still is what do we want to do with #1512424: Add a LanguageInterface, and property setters/getters to Language class? If you language_load('de'), you *don't* get an instance of the language class. See http://drupal.org/node/1512424#comment-6101938 - I think this would make sense if we use the Language class more globally. Currently, its limited to the DI / negotiation layer only.

Opened #1627208: Make language_list() and language_default() return instances of Language for that.

All of those landed, so looks like this can now be started, right?

Moving off the D8MI focus board.

Just started to move the constants to the language class, but left out most of LANGUAGE_NOT_SPECIFIED and LANGUAGE_DEFAULT.

Renaming is easy if you have all use statements in place.

Thanks! Should we prefix them with LANGUAGE_ even though they are under the Language class? Seems superfluous to me. Also, different language constants have very different meanings. Eg. LANGUAGE_TYPE_* are language negotiation types, not types of languages :)

For those I'd go with #1.

<

Thanks! Should we prefix them with LANGUAGE_ even though they are under the Language class? Seems superfluous to me.

I totally agree with you.

Here is a list of constants the current patch moves, with a suggestion of a new naming. We should then probably move this to the issue summary.

Really not sure whether it's worth to convert that now, as it will certainly conflict with all the EntityNG changes.

Let's see what the testbot thinks

@dawehner:

Overall the proposal looks good to me, but:

LANGUAGE_SYSTEM Language::LANGCODE_SYSTEM

These could become simpler: Language::CODE_SYSTEM.

LANGUAGE_TYPE_CONTENT Language::NEGOTIATION_CONTENT

These sould really be prefixed with TYPE: we spent a lot of time discussing and standardizing on a terminology: Language::TYPE_CONTENT.

Agreed with @plach on TYPE, not on CODE :) We also spent a lot of time standardising on "langcode" vs. anything else :D

Right :)

Updated issue summary.

Thank you for the feedback!

Added the feedback from #16 and #17 to the issue summary and updated the patch.

Seriously, I need a better internet connection for these kind of patches :)

This fixes some wrong constant names and rewraps comments to column 80. The interdiff covers only the latter.

Fixes hopefully all problems.

LANGUAGE_DEFAULT is right now not used as constant for language_default() but for the default language of an entity. Still, we could keep the name and use it to refer to the default language in whatever context? Or is that confusing and we should rename it?

@fago: not sure how can LANGUAGE_DEFAULT ('und') accurate to signify defaultness at places where the default might be different. Eg. content types, vocabularies, etc. have a setting of default language for new entities created, and one possible option is 'und', while there are other options. So the default language of those entities when created is not necessarily 'und', however that is one of the possible options as well, so I'm not seeing how it can be used generally as-is. Not sure what it was meant to do, I believe it was introduced with the "property api" patches.

@fago:

Do you mean using a LANGUAGE_DEFAULT constant instead of language_default()?

It's used to refer to the entity default language - what ever it is. It's value is 'und' right now because of the BC-mode, however a new, better BC-mode just got committed. I think with the new BC mode changing the constant should work now - thus we could move it over to a more reasonable value.

Do you mean using a LANGUAGE_DEFAULT constant instead of language_default()?

No, language_default() is getting the site's default language. I'd use the constant to refer to the language default depending on the context, e.g. in site context it's language_default() while in entity context it's $entity->langcode->value.

Gave it a short test - no unfortunately the new BC-mode does not allow to change the constant right now either. We'll need to wait for the BC mode to be removed for changing it.

I also noted we already have a site default language constant. Should we just make it an entity default language constant?

No, language_default() is getting the site's default language. I'd use the constant to refer to the language default depending on the context, e.g. in site context it's language_default() while in entity context it's $entity->langcode->value.

Ok, now I get it :)

Not sure about this, I think we should try and see how it looks while addressing the active language/active translation issue. See for instance #1807692-13: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget (EntityTranslationController.php and related hunks): the current solution is a hack, we should try to find a better one. For instance passing along an EntityTranslation object instead of an Entity object. Anyway we'll need an issue to dicuss this.

Just another rerole.

.

Another rerole, hopefully I catched all of them.

Less fails, yeah!

+++ b/core/includes/bootstrap.incundefined
@@ -2770,13 +2686,13 @@ function language_multilingual() {
+ *   It can be: Language::STATE_CONFIGURABLE, Language::STATE_LOCKED, Language::STATE_ALL.

+++ b/core/includes/language.incundefined
@@ -528,18 +530,18 @@ function language_url_split_prefix($path, $languages) {
+    // Get languages ordered by weight, add Language::LANGCODE_NOT_SPECIFIED at the end.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -245,9 +246,9 @@ protected function attachPropertyData(array &$entities, $load_revision = FALSE)
+        // Field values in default language are stored with Language::LANGCODE_DEFAULT as

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -118,28 +119,28 @@ public function &__get($name) {
+      // Language::LANGCODE_DEFAULT by mapping the values to Language::LANGCODE_DEFAULT. This is
...
+      // Language::LANGCODE_DEFAULT while field API expects them to be keyed by langcode.

@@ -160,18 +161,18 @@ public function __set($name, $value) {
+        // with Language::LANGCODE_DEFAULT while field API expects them to be keyed by

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -38,7 +38,7 @@ class EntityNG extends Entity {
+   * are keyed by language code, whereas Language::LANGCODE_DEFAULT is used for values in

@@ -140,12 +140,12 @@ public function uuid() {
+    // Values in default language are always stored using the Language::LANGCODE_DEFAULT

@@ -161,9 +161,9 @@ protected function getTranslatedField($property_name, $langcode) {
+      // Non-translatable fields are always stored with Language::LANGCODE_DEFAULT as key.

@@ -287,15 +287,15 @@ public function language() {
+    // If the default language is Language::LANGCODE_NOT_SPECIFIED, the entity is not

@@ -347,15 +347,15 @@ public function getTranslationLanguages($include_default = TRUE) {
+    // We include the default language code instead of the Language::LANGCODE_DEFAULT

+++ b/core/lib/Drupal/Core/Language/Language.phpundefined
@@ -20,13 +20,97 @@ class Language {
+   * @todo: Change value to differ from Language::LANGCODE_NOT_SPECIFIED once field API
+   * leverages the property API.

+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -115,7 +115,7 @@ public function getLanguage($type) {
+   *   Language::TYPE_INTERFACE, or NULL to reset all language types. Defaults to

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.phpundefined
@@ -40,7 +40,7 @@ public function getTranslationLanguages($include_default = TRUE);
+   *   The language code of the translation to get or Language::LANGCODE_DEFAULT to get

+++ b/core/modules/field/field.multilingual.incundefined
@@ -19,10 +21,10 @@
+ * - For untranslatable fields this set only contains Language::LANGCODE_NOT_SPECIFIED.
...
+ *   installed languages with the addition of Language::LANGCODE_NOT_SPECIFIED. This

@@ -102,7 +104,7 @@ function field_language_delete() {
+ * language codes will be returned, otherwise only Language::LANGCODE_NOT_SPECIFIED will

@@ -125,7 +127,7 @@ function field_available_languages($entity_type, $field) {
+    // of enabled languages, otherwise we return just Language::LANGCODE_NOT_SPECIFIED.

@@ -258,7 +260,7 @@ function field_valid_language($langcode, $default = TRUE) {
+ * display language code to be used is just Language::LANGCODE_NOT_SPECIFIED, as no other

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -188,11 +189,11 @@ public function query($use_groupby = FALSE) {
+        // By the same reason as field_language the field might be Language::LANGCODE_NOT_SPECIFIED in reality so allow it as well.

@@ -846,11 +847,11 @@ function field_langcode(EntityInterface $entity) {
+      // (or Language::LANGCODE_NOT_SPECIFIED), in case the field has no data for the selected language.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.phpundefined
@@ -129,11 +130,11 @@ function testPathLanguageConfiguration() {
+    // Assign a custom path alias to second node with Language::LANGCODE_NOT_SPECIFIED.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -420,7 +420,7 @@ function locale_translate_batch_build($files, $options) {
+ *   contain a language parameter (other than Language::LANGCODE_NOT_SPECIFIED). This

+++ b/core/modules/views/views.moduleundefined
@@ -634,14 +635,14 @@ function views_add_contextual_links(&$render_element, $location, ViewExecutable
+ *   It can be: Language::STATE_CONFIGURABLE, Language::STATE_LOCKED, Language::STATE_ALL.

linebreak.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNewIndicatorTest.phpundefined
@@ -53,8 +55,8 @@ public function testCommentNewCommentsIndicator() {
diff --git a/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php

diff --git a/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php
index 7910196..8b9a09a 100644

index 7910196..8b9a09a 100644
--- a/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php

--- a/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.php
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.phpundefined

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.phpundefined
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNodeAccessTest.phpundefined
@@ -7,8 +7,6 @@

@@ -7,8 +7,6 @@
 
 namespace Drupal\comment\Tests;
 
-use Drupal\simpletest\WebTestBase;
-
 /**
  * Tests comments with node access.

Let's keep this small fix out of the patch.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.phpundefined
@@ -23,7 +25,7 @@ public static function getInfo() {
diff --git a/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php

diff --git a/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php
index 46d0318..252d06e 100644

index 46d0318..252d06e 100644
--- a/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php

--- a/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.php
+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined

+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
+++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/DefaultViewRecentComments.phpundefined
@@ -7,8 +7,7 @@

@@ -7,8 +7,7 @@
 
 namespace Drupal\comment\Tests\Views;
 
-use Drupal\entity\DatabaseStorageController;
-use  Drupal\views\Tests\ViewTestBase;
+use Drupal\views\Tests\ViewTestBase;
 
 class DefaultViewRecentComments extends ViewTestBase {

dito

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.phpundefined
@@ -60,7 +62,7 @@ function testBreadCrumbs() {
diff --git a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php

diff --git a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php
index cd432a7..56f7ef9 100644

index cd432a7..56f7ef9 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php

--- a/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.phpundefined

+++ b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.phpundefined
+++ b/core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.phpundefined
@@ -148,7 +148,6 @@ function testLookupPath() {

@@ -148,7 +148,6 @@ function testLookupPath() {
     // Test the situation where the alias and language are the same, but
     // the source differs. The newer alias record should be returned.
     $pathObject->save('user/2', 'bar');
-    $this->assertEqual($aliasManager->getSystemPath('bar'), 'user/2', 'Newer alias record is returned when comparing two LANGUAGE_NOT_SPECIFIED paths with the same alias.');
   }
 

Damn, this was probably part of the merge conflict.

+++ b/core/modules/system/system.installundefined
@@ -1396,8 +1397,8 @@ function system_update_8004() {
+    // language. Our best guess is to set all of them to Language::LANGCODE_NOT_SPECIFIED.

linebreak.

+++ b/core/modules/system/tests/upgrade/drupal-7.language.database.phpundefined
@@ -422,7 +422,7 @@
+// is a simple node with Language::LANGCODE_NOT_SPECIFIED as language code. The second

linebreak.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1200,7 +1201,7 @@ public function optionsSummary(&$categories, &$options) {
+      Language::LANGCODE_NOT_SPECIFIED => t('Language neutral'),

Indentation.

It seems to make sense to skip the line breaks in this issue, as they have the potential to blow up this patch even more. Would do you think?

This patch should be really green now.

I can reproduce the error locally, it's something quite odd: it tells you that the ConfigStorageController uses the LANGUAGE_NOT_SPECIFIED constant, even this constant is not used anywhere.

My theory is that this const is somewhere stored in APC and then PHP get's confused. This one is based upon the experimental behavior of going away when restarting apache.

#42: drupal-1620010-42.patch queued for re-testing.

dreditor crashed on me:/
anyways, good job, i just found that several comments now exceed 80 chars

Wrapped the 80 chars.

Rerolled against a views change.

ok, went through most of it again, looks legit, i grep'ed, couldnt find anything forgotten
good job!
i hope bot agrees and you dont reroll this many times till commit, although i doubt:/

sorry, my grep was overengineered:P
found some left

./core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.php:    $this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGUAGE_SYSTEM.'));


./core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.php:    $this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGUAGE_SYSTEM.'));

./core/modules/node/lib/Drupal/node/Tests/PagePreviewTest.php:      'langcode' => LANGUAGE_NOT_SPECIFIED,
./core/modules/node/lib/Drupal/node/Tests/PagePreviewTest.php:      'langcode' => LANGUAGE_NOT_SPECIFIED,

./core/modules/language/lib/Drupal/language/Form/NegotiationSelectedForm.php:      '#languages' => LANGUAGE_CONFIGURABLE | LANGUAGE_SITE_DEFAULT,

./core/modules/language/lib/Drupal/language/Form/NegotiationSelectedForm.php:      '#languages' => LANGUAGE_CONFIGURABLE | LANGUAGE_SITE_DEFAULT,

Aren't all of those not comments, so 80 chars is not an issue?

Back to needs review.

testbot failed..also i dont talk about wraps:)

$this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGUAGE_SYSTEM.'));
should be
$this->assertTrue(t($name, array(), array('langcode' => Language::LANGCODE_SYSTEM)) == $name, t('t() works for Language::LANGCODE_SYSTEM.'));

or

./core/modules/node/lib/Drupal/node/Tests/PagePreviewTest.php: 'langcode' => LANGUAGE_NOT_SPECIFIED,
should be Language::LANGCODE_NOT_SPECIFIED

I seriously hope we will get this in sooner or later.

git add -p isn't fun on that patch :)

We will definitely needs avoid commit conflicts on this.

I fear that this requires a LOT of other patches to be rerolled, so rerolling just this over time, seems better for the total community.

Yep, also because we may want to move some constants to the language manager as proposed in #1862202: Objectify the language system.

@plach
Do you have a prefered order of getting it in? I'm fine with rerolling both of it.

Well, I think it depends on when I'm able to get back to that issue. If this is ready and there are no big RTBC patches around I guess we can simply go on and reroll #1862202: Objectify the language system afterwards. Or we can simply move the proper constants that to the language manager directly here (don't remember exactly which ones).

@dawehner: agreed with this, removing conflict avoidance tag.

I fear that this requires a LOT of other patches to be rerolled, so rerolling just this over time, seems better for the total community.

Weird... tag was not actually removed

tagging with needs reroll

@plach, @gabor
If you think it's time for that patch, just comment here, and I will do another rerole.

#58: drupal-1620010-58.patch queued for re-testing.

Doesn't apply anymore, but if a reroll passes, I will RTBC this so hard.

Removed the unused statements in access controllers.

#72: drupal-1620010-72.patch queued for re-testing.

for your rtbc pleasure

#72 missed LANGUAGE_NOT_APPLICABLE in bootstrap.inc

Crosspost. RTBC as per #68 and #71

#77: drupal-1620010-76.patch queued for re-testing.

#77: drupal-1620010-76.patch queued for re-testing.

a forgotten use statement, and well a class named Language:)

Arg

RTBC if green.

Committed f2d710c and pushed to 8.x. Thanks!

Added change notice at https://drupal.org/node/2011418

Awesome :)