abstract logic for getting language object data into method getDataObject [#2246729]

Problem/Motivation

In ConfigurableLanguageManager getLanguages() is this hunk:

        $data = $language->toArray();
        $langcode = $data['id'];
        // Initialize default property so callers have an easy reference and can
        // save the same object without data loss.
        $data['default'] = ($langcode == $default->id);
        $data['name'] = $data['label'];
        $this->languages[$langcode] = new Language($data);

Proposed resolution

that logic should just be in a method like getDataObject(). then we could just:

$this->languages[$langcode] = $language->getDataObject();

Remaining tasks

git instructions for creating patch | Contributor task documentation for creating a patch
Review patch to check it fixes the issue, the change is properly documented and for coding standards. Make sure patch stays within scope of just this issue. | Contributor task documentation for reviewing patch

User interface changes

API changes

No, just addition.

Related issues

Split off from #2239497: [Meta] Fix ConfigurableLanguageManager getLanguages()
?

Comment	File	Size	Author
#2	2246729-abstract_dataObject-2.patch	3.08 KB	YesCT
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

YesCT CreditAttribution: YesCT commented 21 April 2014 at 20:39

Especially weird is
$data['name'] = $data['label'];
Note though: #2246721: Language class should use property 'label' to be consistent with entities exists

Comment #2

YesCT CreditAttribution: YesCT commented 22 April 2014 at 03:31

Status:

Active

» Needs review

File	Size
2246729-abstract_dataObject-2.patch	3.08 KB

pretty much just guessing here.
other issues that would help are the typehinting #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language and renaming #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.

Comment #3

YesCT CreditAttribution: YesCT commented 22 April 2014 at 03:37

+++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
@@ -407,4 +403,25 @@ public function getLanguageConfigOverride($langcode, $name) {
+    $language = new Language($data);

I think this creates a core Language ... and we might want a language module (configurable) Language.

Comment #4

filijonka CreditAttribution: filijonka commented 13 May 2014 at 15:22

I don't get a good feeling about this; it just moving around code.

The purpose of the foreach is to for every configuration create a language object. To do so we have to make sure that the array the Language constructor gets is of this form eg:

array(
    'id' => 'en',
    'name' => 'English',
    'direction' => 0,
    'weight' => 0,
    'locked' => 0,
    'default' => TRUE,
  );

Due to that the array that config->get gives us is a mismatch to the above array we need to transfer it. One of the strange things in drupal right now is this mismatch beetween label and name. some are using names, some are using labels, some are using both. some say labels means this and namne that vice verse.

Anyway the transfering is done in three rows and to put that into a public function is wrong, simply because the transfering isn't something that should happen everytime a language object is created; by making it public it opens up for wrongly using, should be private but it seems to much to have it in function at all.

Anyting that I would do to this code to perhaps clarify it is

 $this->languages[$langcode] = new Language(array(
    'id' => $data['id'],
    'name' => $data['label'] ,
    'direction' => $data['direction'],
    'weight' => $data['weight'],
    'locked' => $data['locked'],
    'default' => ($data['id'] == $default->id),
  )
);

One advantage of this is that right now the data array that is used in the language constructor still has the label which is never used. It really doesn't matter but might not be the nicest approach.

The comment in #3 is the real issue here I think and by reading around my thought is why the module Language and core Language is named the same.

Comment #5

dawehner

German

CreditAttribution: dawehner commented 19 May 2014 at 22:14

+++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
@@ -407,4 +403,25 @@ public function getLanguageConfigOverride($langcode, $name) {
+   * @param string $default_langcode
+   *   The default language.
...
+  public function getDataObject(array $data, $default_langcode) {

I wonder whether we should not pass the default langcode along but just use $this->getDefaultLanguage()