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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vasi1186’s picture

Assigned: Unassigned » vasi1186
vasi1186’s picture

Status: Active » Needs review
FileSize
2.23 KB

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.

vasi1186’s picture

let's try again.

vasi1186’s picture

Status: Needs work » Needs review
tstoeckler’s picture

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?

vasi1186’s picture

@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;
}
Gábor Hojtsy’s picture

@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 :)

Gábor Hojtsy’s picture

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 :)

sun’s picture

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.

RobLoach’s picture

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.

vasi1186’s picture

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

vasi1186’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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.

vasi1186’s picture

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.

vasi1186’s picture

Status: Needs review » Needs work
Gábor Hojtsy’s picture

@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.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

Implemented the changes from #14.

vasi1186’s picture

The previous patch was incomplete...

Status: Needs review » Needs work

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

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.9 KB

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.

cosmicdreams’s picture

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

tstoeckler’s picture

+++ 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...

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.39 KB

Implemented the changes from #24.

Status: Needs review » Needs work

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

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.39 KB

Previous patch generated some warnings.

vasi1186’s picture

Gábor Hojtsy’s picture

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.

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.15 KB

Fixed the issues reported in #29

Gábor Hojtsy’s picture

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?

vasi1186’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

Implemented changes from #31

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

Gábor Hojtsy’s picture

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

function example_api(Language $language, ....) {
  // have fun
}
RobLoach’s picture

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.

vasi1186’s picture

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.

plach’s picture

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 :)

vasi1186’s picture

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...

Gábor Hojtsy’s picture

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

vasi1186’s picture

@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...)

Gábor Hojtsy’s picture

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

cosmicdreams’s picture

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.

Gábor Hojtsy’s picture

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

RobLoach’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record
Gábor Hojtsy’s picture

Title: Make language_list() and language_default() return instances of Language » CHANGELOG: Make language_list() and language_default() return instances of Language
Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
630 bytes

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

RobLoach’s picture

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.

Gábor Hojtsy’s picture

Status: Fixed » Needs review
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

:-)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Title: CHANGELOG: Make language_list() and language_default() return instances of Language » Make language_list() and language_default() return instances of Language
Gábor Hojtsy’s picture

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.