Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface

Discussed the following with @tim.plunkett in IRC:

Core's LanguageInterface has getName() and getId(), while EntityInterface has id() and label(). Since ConfigurableEntityInterface will extend both, we should be consistent. Since EntityInterface is the more prominent one @tim.plunkett suggested renamed LanguageInterface::getId() and ::getName() to id() and label(), respectively.

Since I think getId() and getName() are the better names in this case, I initially proposed leaving the duplicated methods in place, but I will defer to @tim.plunkett.

Per the test result, it seems only LanguageUnitTest needs to be updated. That's proof that setName() and setId() are in fact not needed. Nice!

Hm, not sure how I feel about this issue. For one, it looks appealing that the configurable language would be an extension of the runtime language, OTOH the runtime language has other properties (eg. the negotiation method) that are runtime computed and not saved.

Re #11: Yes, now that we actually have a LanguageInterface we should get to a point where all language code gets either a \Drupal\Core\Language\Language or a \Drupal\language\Entity\Language and doesn't care which one. (Yes, we're still far from that...)

We specifically have ConfigEntityInterface::toArray() to support the use-case of dynamic properties that should not get saved. I.e. we could unset the negotiation method in there. However, even easier (and better!) is if we make the negotiation method and the default property protected, as that way they won't get saved by default anyway.

So I think #11 is a very good point and we should make sure that it works that way, but at least to me that's not an argument against this issue.

14: 2246679-rename_to_configurable-14.patch queued for re-testing.

Updating title

Working on this issue at TCDrupal 2014.

This patch makes LanguageInterface class in/core/modules/language/src/LanguageInterface.php extend \Drupal\Core\Language\LanguageInterface.

I also noticed that in Drupal\Core\Language\Language $direction is set like this:

public $direction = self::DIRECTION_LTR;

but in Drupal\language\Entity\Language it's set to:

public $direction = ‘';

I'm not sure if this is something that needs to be addressed and if it is in scope of this issue.

This needs a line added back in. I'm working with alimac on it.

This patch adds in the missing line.

Patch no longer applied.

YAY TCDrupal, advanced so many issues... and caused so many reroll ..boo

But Yays trump boos any time :)

Just a straight reroll - triggered by https://www.drupal.org/node/2271005

... I will write tests while the testbot is busy.

Drupal\language\Tests\ConfigurableLanguageEntityTest is a new test class for all the new getters/setters, with the exception of getId/setId.

its a unit test, well that is it extends KernelTestBase, and runs 6 tests in 1sec. The guts of the code are trivial :-

    $setName = 'Set Name'; // string
    $configurable_language->setName($setName);
    $this->assertEqual($configurable_language->getName(), $setName, 'Name written and read correctly.');

    $setDirection = 0x1234; // integer
    $configurable_language->setDirection($setDirection);
    $this->assertEqual( $configurable_language->getDirection(), $setDirection,  'Direction written and read correctly.');

    $setWeight = 0x5678; // integer
    $configurable_language->setWeight($setWeight);
    $this->assertEqual( $configurable_language->getWeight(), $setWeight,  'Weight written and read correctly.');

    $setMethod = 'Set Negotiation Method Identifier'; // String
    $configurable_language->setNegotiationMethodId($setMethod);
    $this->assertEqual( $configurable_language->getNegotiationMethodId(), $setMethod,  'Negotiation Method Identifier written and read correctly.');

Just minor nitpicks

1. +++ b/core/modules/language/src/ConfigurableLanguageInterface.php
   @@ -12,6 +12,6 @@
   +interface ConfigurableLanguageInterface extends ConfigEntityInterface, \Drupal\Core\Language\LanguageInterface  {
   

   please do not use pull path here

2. +++ b/core/modules/language/src/Tests/ConfigurableLanguageEntityTest.php
   @@ -0,0 +1,57 @@
   +    $this->assertEqual( $configurable_language->getDirection(), $setDirection,  'Direction written and read correctly.');
   ...
   +    $this->assertEqual( $configurable_language->getWeight(), $setWeight,  'Weight written and read correctly.');
   ...
   +    $this->assertEqual( $configurable_language->getNegotiationMethodId(), $setMethod,  'Negotiation Method Identifier written and read correctly.');
   

   extra space

3. +++ b/core/modules/language/src/Tests/ConfigurableLanguageEntityTest.php
   @@ -0,0 +1,57 @@
   \ No newline at end of file
   

   that's should be fixed too

Thanks fixed.

Hi @martin107, thanks for the patch. I fixed some coding standards errors picked out by the Coder Sniffer, and also set the testing variables to reasonable values e.g. set the language's direction variable to ConfigurableLanguage::DIRECTION_RTL instead of 0x1234.

This patch was made at the Drupal London Code Sprint, 17th August 2014.

1. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -235,4 +248,84 @@ protected static function getDefaultLangcode() {
   +    return $this->label;
   

   Lets use the label() method here.

2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -235,4 +248,84 @@ protected static function getDefaultLangcode() {
   +    return $this->id;
   

   Let's use the id() method

3. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -235,4 +248,84 @@ protected static function getDefaultLangcode() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setId($id) {
   +    $this->id = $id;
   +
   +    return $this;
   +  }
   

   This method should not be on the language interface and should not exist. It is only used for testing purposes.

4. +++ b/core/modules/language/src/Tests/ConfigurableLanguageEntityTest.php
   @@ -0,0 +1,74 @@
   +class ConfigurableLanguageEntityTest extends KernelTestBase {
   

   This test should be a PHPUnit test no need for it to be a KernelTestBase test.

@alexpott.. Can I convince you that MOST of the getters and setters test can be unit tested BUT one cannot.

Unit testing for setName/getName rapidly becomes complex.

ConfugurableLangage::getName() depends on Entity::label()

  public function label() {
    $label = NULL;
    $entity_type = $this->getEntityType();
    if (($label_callback = $entity_type->getLabelCallback()) && is_callable($label_callback)) {
      $label = call_user_func($label_callback, $this);
    }
    elseif (($label_key = $entity_type->getKey('label')) && isset($this->{$label_key})) {
      $label = $this->{$label_key};
    }
    return $label;

which means mocking the entityManager->getDefinition function, $entity::getEntityType and a whole avalanche of other functions

I have found a similar unit test which tries something similar ( see ResponsiveImageMappingEntityTest ) My code is largely based on this.

What I have does not work yet... but after a pause and a review of the code .. specifically only ConfigurableLanguageEntityTest::testNameMethod() it looks to me like this is an inefficient dive down a rabbit hole...I think I have created a sprawling mess of code when the old test strategy could be justified as a integration test...

I hope you would indicate which route you prefer.

Well actually do we even need the setName() method? The only usage is a test. And like the id - the name of the language should not change a runtime.

The use case for changing the name in-request is submission of a language editing form. It does not use setName() I guess...

Yep entity form does not use set methods it uses the set method. If it ever does then there will be a general setLabel method that would not be the responsibility of this interface or object.

I want to file a minor housekeeping change, just so this issue moves forward in small stable steps.

A) From #38 all review changes, other than the test issue are resolved.

B) All the uncomplicated unit tests that exist and work have been moved into \Drupal\language\Tests\ConfigurableLanguageUnitTest
There are @covers annotations that I want to keep and using testing those methods is entirely appropriate.

As for #40 and #41

Well actually do we even need the setName() method?

and what we do about that ... well that is outside a little housekeeping fixup! ..

For followers half reading this issue... changing the name property of a ConfigurableLanguage Entity ...uses the \Drupal\language\Form\LanguageEditForm whose submit() uses the old procedural way of doing things that is language.module's language_save() which bypasses the setName() with the conversion of label to name with the following

$configurable_language->label = isset($language->name) ? $language->name : '';

regarding #42
I have no strong opinion about how to proceed with the label/name issue. Yes much of this seems outside the scope of an issue titled.

Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface

For followers half reading this issue... changing the name property of a ConfigurableLanguage Entity ...uses the \Drupal\language\Form\LanguageEditForm whose submit() uses the old procedural way of doing things that is language.module's language_save()

Do we have an issue for that? If not, maybe we should create one.

I'm not sure if we have full test coverage in that unit test, but I'm not a big fan of unit testing setters and getters.
Current patch looks OK in terms of code standards. Very tempted to RTBC it.

As this issue is blocking some others, what else do we need to move forward?

Let's get this in. Looks like we got what we have in issue summary, though the renaming(LanguageInterface => ConfigurableLanguageInterface) has been done already.

Ok So this looks like I lacked perspective when YesCT asked for testing in #30
I put them in blindly...

There is significant but not complete overlap between the testing that this issues adds ( \Drupal\language\Tests\ConfigurableLanguageUnitTest ) and the existing unit testing in \Drupal\Tests\Core\Language\LanguageUnitTest.

I agree with @penyaskito in #44

I'm not a big fan of unit testing setters and getters.

But we have them now, and I think they should stay, so I need to merge the two unit tests together and remove the duplicate ones.

\Drupal\Tests\Core\Language\LanguageUnitTest. is the most comprehensive test class but does not test weight setter and getters which ConfigurableLanguageUnitTest does.

the start of the week is busy .. so I will not be able to make a start on this until maybe as late as next weekend.

If this is holding up other things then others might beat me to this task ... or else a merge unit testing issue can be opened up which I will jump on..

No, we shouldn't merge both. As different classes both deserve their own tests.
What I meant is that we lack tests for get/setName and get/setNegotiationMethodId, but I'm not sure if that is mandatory and I don't have any problem with not adding them. Let's see what core committers think about it.

Put on sprint for tracking.

Committed and pushed to 8.x. Thanks!