As discussed in #1512424: Add a LanguageInterface, and property setters/getters to Language class, after #1497230: Use Dependency Injection to handle object definitions introduced the language class, and made language negotiation return instances of this class, it makes most sense to do the same for language_list() and language_default() (and any other places where we return language, if we know more :). language_load() just reuses language_list() so this should fix that too.

Sounds like this could be as simple as language_list() initializing a new instance of Language with every record and language_default() doing the same based on the variable values. Currently the Language object is "backwards compatible" with the anonymous data objects we used before (as in we still use at places), so we can just pass the values in the constructor and leave the actual data use as-is for now. More would come later as we work out #1512424: Add a LanguageInterface, and property setters/getters to Language class

Files: 
CommentFileSizeAuthor
#46 language-class-changelog.txt630 bytesGábor Hojtsy
#32 drupal-language_instance-1627208-32.patch6.06 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,841 pass(es).
[ View ]
#30 drupal-language_instance-1627208-30.patch6.15 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es).
[ View ]
#27 drupal-language_instance-1627208-27.patch6.39 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,848 pass(es).
[ View ]
#25 drupal-language_instance-1627208-25.patch6.39 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] 36,848 pass(es), 0 fail(s), and 451 exception(s).
[ View ]
#21 drupal-language_instance-1627208-21.patch6.9 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] 36,819 pass(es), 2 fail(s), and 451 exception(s).
[ View ]
#19 drupal-language_instance-1627208-19.patch6.19 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] 36,817 pass(es), 3 fail(s), and 463 exception(s).
[ View ]
#18 drupal-language_instance-1627208-18.patch3.29 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#12 drupal-language_instance-1627208-10.patch3.72 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#4 drupal-language_instance-1627208-4.patch3.13 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]
#2 drupal-language_instance-1627208-2.patch2.23 KBvasi1186
FAILED: [[SimpleTest]]: [MySQL] 36,809 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Comments

Assigned:Unassigned» vasi1186

Status:Active» Needs review
StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 36,809 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Attached a patch that makes the language_list() and language_default() function return Language objects.

Status:Needs review» Needs work

The last submitted patch, drupal-language_instance-1627208-2.patch, failed testing.

StatusFileSize
new3.13 KB
PASSED: [[SimpleTest]]: [MySQL] 36,824 pass(es).
[ View ]

let's try again.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/includes/bootstrap.inc
@@ -2678,17 +2679,18 @@ function language_list() {
+        $languages[$langcode] = new Language((array)$language);

There should be a space after the cast. Maybe we can simply fetch arrays directly, that would be cleaner IMO.

+++ b/core/includes/bootstrap.inc
@@ -2734,18 +2736,23 @@ function language_name($langcode) {
+    $options = (array) variable_get('language_default', array(

Hmm... this looks strange. Instead of casting here can we instead update the code that saves this to directly save an array?

@tstoeckler:
Agree with one, I'll change it.
For the second, how about changing the constructor of the Language class and make the cast inside it?, something like:

foreach ((array) $options as $name => $value) {
   $this->$name = $value;
}

@vasi1186: We usually try to stay away from APIs that can be invoked with different data structures like that I think. We already have language_default updates in bootstrap.inc and tests for those updates, so changing it to an array should be a few more lines in the update code hopefully :)

Oh, and storing an array in language_default would also be great so that nobody mistakes it for a Language class instance. It will not be an object and that should show it :)

The language_default variable bugs me for a long time already. Ideally, it should only contain a langcode string, nothing else. If people want to override or customize the resulting language object, then they should do so properly.

There is no way we could just outright remove the language_default variable, could we? Seems like if you're running Drupal in a different language, you'd have the Language module installed anyway, which has its own handling of the default language.

StatusFileSize
new3.72 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Attached a new patch that should always save the language_default as an array.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/includes/update.incundefined
@@ -213,11 +213,11 @@ function update_prepare_d8_language() {
     $language_default = variable_get('language_default');
     if (!empty($language_default)) {
-      if (isset($language_default->language)) {
-        $language_default->langcode = $language_default->language;
-        unset($language_default->language);
+      if (isset($language_default['language'])) {
+        $language_default['langcode'] = $language_default['language'];
+        unset($language_default['language']);
       }
-      unset($language_default->enabled);
+      unset($language_default['enabled']);
       variable_set('language_default', $language_default);
     }

I don't think this would actually change the variable to an array :) This runs in the update and works with the Drupal 7 database, at which point this is still an object. So looks like all the changes here would need to be undone and the variable_set at the end needs the (array) cast and moved out of the condition AFAIS.

Status:Needs work» Needs review

One thing to clarify about a new method in the Language class, that is called toArray(). The reason why I created the method is that I would not rely on the defaut cast operation that PHP can do, because in the future this class will have probably lots of changes, and it could happen that some of its attributes will become private. In that case, the cast operation will fail.

Status:Needs review» Needs work

@sun: the langcode thing has a nice issue at #1272862: Clean up default language handling and can be done regardless of making these things result in instantiated classes.

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Implemented the changes from #14.

StatusFileSize
new6.19 KB
FAILED: [[SimpleTest]]: [MySQL] 36,817 pass(es), 3 fail(s), and 463 exception(s).
[ View ]

The previous patch was incomplete...

Status:Needs review» Needs work

The last submitted patch, drupal-language_instance-1627208-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.9 KB
FAILED: [[SimpleTest]]: [MySQL] 36,819 pass(es), 2 fail(s), and 451 exception(s).
[ View ]

Previous patch failed testing probably because of a small thing in testDependencyInjectedNewDefaultLanguage().

Status:Needs review» Needs work

The last submitted patch, drupal-language_instance-1627208-21.patch, failed testing.

Looks like you made some progress here I'll try to review and help out so that you guys can have a better starting point tomorrow

+++ b/core/includes/bootstrap.inc
@@ -2677,18 +2678,19 @@ function language_list() {
+      foreach ($languages as $langcode => $language) {
+        $language['default'] = ($langcode == $default->langcode);
+        $languages[$langcode] = new Language($language);

Minor, but I would prefer $info here instead of $language.

+++ b/core/includes/bootstrap.inc
@@ -2734,18 +2736,23 @@ function language_name($langcode) {
+  $options = variable_get('language_default', array(
...
+  $options['default'] = TRUE;
...
+  if (!isset($languages[$options['langcode']])) {
+    $languages[$options['langcode']] = new Language($options);
...
+  return $languages[$options['langcode']];

Same here. $options doesn't really make sense to me.

+++ b/core/includes/update.inc
@@ -218,8 +218,8 @@ function update_prepare_d8_language() {
+    variable_set('language_default', (array) $language_default);

This deserves a comment.

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -50,4 +50,16 @@ class Language {
+  /**
+   * Returns an array with some of the attributes of the class.
+   */
+  public function toArray() {

Can we leave this out for now? If we want kill the 'language_default' variable saving an array anyway...

Status:Needs work» Needs review
StatusFileSize
new6.39 KB
FAILED: [[SimpleTest]]: [MySQL] 36,848 pass(es), 0 fail(s), and 451 exception(s).
[ View ]

Implemented the changes from #24.

Status:Needs review» Needs work

The last submitted patch, drupal-language_instance-1627208-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.39 KB
PASSED: [[SimpleTest]]: [MySQL] 36,848 pass(es).
[ View ]

Previous patch generated some warnings.

Status:Needs review» Needs work

+++ b/core/includes/bootstrap.incundefined
@@ -2734,18 +2736,23 @@ function language_name($langcode) {
/**
  * Returns the default language used on the site.
  *
+ *
  * @return
  *   A language object.
  */

Extra newline should not be there.

+++ b/core/includes/bootstrap.incundefined
@@ -2734,18 +2736,23 @@ function language_name($langcode) {
function language_default() {
-  $default = variable_get('language_default', (object) array(
+  $info = variable_get('language_default', array(
     'langcode' => 'en',
     'name' => 'English',
     'direction' => 0,
     'weight' => 0,
   ));
-  $default->default = TRUE;
-  return $default;
+  $info['default'] = TRUE;
+  $languages = &drupal_static(__FUNCTION__);
+  if (!isset($languages[$info['langcode']])) {
+    $languages[$info['langcode']] = new Language($info);
+  }
+  return $languages[$info['langcode']];

Why are you doing a list of languages here? It looks very, very odd.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.phpundefined
@@ -57,7 +57,7 @@ class LanguageDependencyInjectionTest extends WebTestBase {
-    $expected = new Language((array) language_default());
+    $expected = language_default();
     $result = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);

New code looks much better than old code.

Status:Needs work» Needs review
StatusFileSize
new6.15 KB
PASSED: [[SimpleTest]]: [MySQL] 36,861 pass(es).
[ View ]

Fixed the issues reported in #29

Status:Needs review» Needs work

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageDependencyInjectionTest.phpundefined
@@ -72,7 +72,7 @@ class LanguageDependencyInjectionTest extends WebTestBase {
   function testDependencyInjectedNewDefaultLanguage() {
     // Change the language default object to different values.
-    $new_language_default = (object) array(
+    $new_language_default = array(
       'langcode' => 'fr',
       'name' => 'French',
       'direction' => 0,
@@ -89,7 +89,7 @@ class LanguageDependencyInjectionTest extends WebTestBase {
@@ -89,7 +89,7 @@ class LanguageDependencyInjectionTest extends WebTestBase {
     $expected = $new_language_default;
     $result = drupal_container()->get(LANGUAGE_TYPE_INTERFACE);
     foreach ($expected as $property => $value) {
-      $this->assertEqual($expected->$property, $result->$property, t('The dependency injected language object %prop property equals the default language object %prop property.', array('%prop' => $property)));
+      $this->assertEqual($expected[$property], $result->$property, t('The dependency injected language object %prop property equals the default language object %prop property.', array('%prop' => $property)));
     }

This looks pretty strange to compare the object properties with an array. Can we instantiate a Language() instance for this instead?

Status:Needs work» Needs review
StatusFileSize
new6.06 KB
PASSED: [[SimpleTest]]: [MySQL] 36,841 pass(es).
[ View ]

Implemented changes from #31

Status:Needs review» Reviewed & tested by the community

Cool, looks good now :) Thanks for working it all out.

Status:Reviewed & tested by the community» Fixed

Nice clean-up. Committed to 8.x. Thanks.

This should allow us to do nice things like type hinting for Language on functions, where that is required, things like:

<?php
function example_api(Language $language, ....) {
 
// have fun
}
?>

Type-hinting would be a great addition! We'd probably want to have a LanguageInterface in #1512424: Add a LanguageInterface, and property setters/getters to Language class so that people are not limited to Language class implementations and people could provide their own Language classes if they wanted. Great idea, Gabor.

Agree with Rob. I think that every class in Drupal should implement an Interface and not force people to use only certain implementation of a class.

Swappable implementations have a cost, albeit not huge. I think we should not inconditionally put interfaces everywhere we have a class. Probably there are some cases where moving towards a slightly less flexible but more performant system would make sense.

That said I ain't sure about this particular case. Form the top of my head I cannot see the advantage of swapping the language implementation, but it might very well be lack of fantasy :)

Maybe "every" class is a bit of an extreme case, that's true :)
But, if we talk about the Language class for example, let's say that somewhere we have to display a list of languages. In my opinion, the output of each Language should be handled by the class itself (for example a toString() like method). So, in this case I can imagine that people would want to display the Language differently in some applications. I know that this can be also done now using the theme system, but I just think that extending the Language class would be just a bit cleaner...

@vasi1186: hm, I don't think I get that use case.

@Gabor: for example if we have a block that lists all the available languages on the site (or in general, a list with languages). The output of each language (that usually is the language name, like English, German, etc..) could be overwritten in a class that would extend the Language class (for example a different display would be "English - en", "German - de", etc...)

Yeah, hm, I think overriding the language class for that sounds "a bit" hackish.

The strongest use case I can think of for having an interface for Language is tests. It may become easier to unit test all of language by providing a mock implementation of the Language Interface.

We should have a changelog entry for this as well as a change notice node. Anybody up for those?

Status:Fixed» Needs work
Issue tags:+Needs change record

Title:Make language_list() and language_default() return instances of LanguageCHANGELOG: Make language_list() and language_default() return instances of Language
Status:Needs work» Needs review
Issue tags:-Needs change record
StatusFileSize
new630 bytes

Reviewed and updated a bit. Looks good. Let's get into changelog too.

Status:Needs review» Fixed

Looks good with me! I'll reference the same changelog from #1512424: Add a LanguageInterface, and property setters/getters to Language class so we can update it accordingly once that's in.

Status:Fixed» Needs review

Status:Needs review» Reviewed & tested by the community

:-)

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Committed and pushed to 8.x. Thanks!

Title:CHANGELOG: Make language_list() and language_default() return instances of LanguageMake language_list() and language_default() return instances of Language

Issue tags:-sprint

Removing sprint tag, #1512424: Add a LanguageInterface, and property setters/getters to Language class is where the action should be at.

Automatically closed -- issue fixed for 2 weeks with no activity.