Add a LanguageInterface, and property setters/getters to Language class

I guess also save, delete, etc? How is this pattern implemented for entities? We could certainly learn from there.

Hm. I am thinking more like the node_save($node) vs. $node->save() as applies to language. Do we have a new standard practice for entities yet? Just trying to be consistent across the board.

Storage methods don't really make sense on the Language object, as you wouldn't know how and where the language is saved to, loaded from, or deleted from.

I'm not sure what the individual methods would buy us. What's the reasoning for introducing them?

Fine, but what does it buy us?

AFAICS, the set* methods won't be used - at least I'm not sure whether there is any code that would set/change a property on a language object after it has been instantiated (as that would potentially have ill effects).

And the get* methods... will make us rewrite all of Drupal core and contrib for the net effect of replacing $language->langcode with $language->getLangcode().

I'm really not opposed to sound improvements, but this seems like a waste of time for everyone (and is not limited to people working on this issue).

I'd like to see something like:

  protected $name = 'English';
  protected $langcode = 'en';
  protected $direction = 0;
  protected $enabled = 1;
  protected $weight = 0;
  protected $default = FALSE;
  protected $method_id = NULL;

  /**
    * Setter method for data member $name
    * @param string $name
    * @return Language - provides fluent interface
    */
  public function setName($name) {
    $this->name = $name;
    return $this;
  }

  public function getName() {
    return $this->name;
  }

  ...

We do need the getters and this provides proper documentation, especially in the IDE for code-completion. I would argue that we also need the setters so we don't do ugly things like calling the constructor with some array and no way to really help out the developer.

Ultimately, we should be able to do:

$language = Drupal\Core\Language\Language::getInstance()->setName('English')->setLangcode('en');

Or we could simply make an interface:

interface Language
{
    public function getLangcode();

    public function getName();

    public function getDirection();

    // ... etc, etc...
}

This allowing core to use a readonly implementation for retrieving hardcoded stuff (array based or such) for a site with no customizations that later modules could override if needed.

AFAICS, the set* methods won't be used - at least I'm not sure whether there is any code that would set/change a property on a language object after it has been instantiated (as that would potentially have ill effects).

I definitely agree with sun here, languages have no reason to be modified at runtime, the only way to modify them is per site-wide configuration, and that's pretty much everything we need. This also make the Language::save() method totally sense-less, only the configuration UI will need to be able to modify those, and for this, it should access to the storage mecanism directly and not this kind of shortcuts. We have no advantages at all to expose publicly those methods: it will confuse developers that will just need the getters (I'm thinking about IDE autocompletion). That's why I think an interface is good, it will allow us to make the implementation evolve easily later if we need to double back without worrying too much about the code that uses the interface only.

EDIT: In order to be able to instanciate those languages, we probably need an external factory that would be tied to the DIC: this would make the HTTP listeners to determine the language and fetch the right instance without worrying about setting internal properties.

@pounard: ok, well, a $language->save() would make as much sense as a $node->save() IMHO. APIs, external modules, etc. might as well add new languages, update, etc. Tests are chock full of languages being added and removed for example. The goal/question was to be most in line with what happens elsewhere in Drupal 8 to follow those patterns and achieve familiarity. There are various hooks for example on saving, deleting, updating, etc. languages. How would those work if the language is a full blown object, etc. These same questions have been explored for other subsystems, so I don't think we need to break new ground here, just align what we do to the rest of Drupal.

I do not agree we should align with entities, for a simple reason: entities are mostly content, and language is configuration. I agree that most content stuff shoud align to each others (the pseudo active record pattern being used by entities is good enough for that) but in our case, the language definitely is not content, and thus has no real technical nor DX reasons to stick on content patterns, but should align on other configuration driven stuff instead.

So what patterns do we have for that that we could apply here? Other core config objects that have methods?

I'm not sure there is a common pattern right now, this should be discussed with other people having the same needs. But as languages are not entities, you still can proceed with custom save handlers into the UI and make the API being a readonly API: this wouldn't break any existing feature and remain a minimal change. The important point is probably to make a first good front API (that's why I'm proposing an interface based API) for users first, the implementation behind can be as messy as we want, any change would be almost zero invasive since the public interfaces would be common usage driven and minimal.

The need right now is 1) user public API (as the interface I proposed, reading language properties) and 2) the DIC usability (that's why having a simple language factory interface would be enough too right now). All we need is a readonly API (writing is not necessary at all since the only place we would want to write language is via the UI at configuration time, basically only once in a site's lifetime).

The idea was to build a factory to be help by the container, not a singleton factory.

If the language is configuration and is meant to be read-only, the variables in the interface should be protected instead of public, and the getter methods should be well-documented.

My main question/problem here is when you now get a language from drupal_container(), it will an instance of the Language class. Cool. When you get a language object from language_load() or language_list() it will not be. So we need to figure out what we would want to use this language class for. To me it sounds like we want to use it universally since the current difference does not make sense.

I agree - if we're going to return an object, make it a named object. If we're going to return a named object in some cases, we should be consistent across all the APIs.

Opened #1627208: Make language_list() and language_default() return instances of Language for that then :) Will try to find someone to help work on that.

So now #1627208: Make language_list() and language_default() return instances of Language is committed, wohoo! So we use Language class instances when you language_load() or get an item from language_list() as well. Back here to figure out what else should this class define/do and how to use it around Drupal :)

Rob: method_id is the id of the language negotiation method that choose the language in the negotiation process. Useful to identify how you got the language. Used for testing but can also be used for other things. I don't know if @inheritdocs can work with api.d.o, have not seen it before, but that is not a guarantee either way.

Is this issue supposed to move language to configuration entities or that is another one?

I think I was referring to #1754246: Languages should be configuration entities. Should we mark this one as a duplicate? I guess so.

Not necessarily. Depends on how #1754246: Languages should be configuration entities turns them into config entities (given the circular dependencies). If it is going to be a parallel object system to actual languages to avoid the circular dependency, then we still need this one.

+++ b/core/lib/Drupal/Core/Language/Language.php
@@ -16,7 +16,7 @@ namespace Drupal\Core\Language;
   public $name = 'English';
   public $langcode = 'en';

If these are meant to be read-only and not changeable, they should be protected, not public.

@ebeyrent: changing that to not-public might need much more extensive changes in core at all the places where language properties are set/accessed. That is the ultimate goal, we might or might not want to get there in one step in this patch right away.

related #2035007: Add docs for properties in Drupal\Core\Language\Language

Is this something that we should do before beta?

I don't know. The language manager now has some OO code for this, but not directly on the language config entity. Hum.

Re-roll, and I added a PHPUnit test.

Retitled to scope. The only thing I found is we never camelcase LangCode like that, always Langcode, we consider it one word. If we'd consider it multiple words, we'd have $lang_code when using lowercase names. So not LangCode. Otherwise looks fine to me.

I renamed LangCode to Langcode. All PHPUnit tests pass locally.

Looks good to me. The only thing that Gábor found has been fixed, so I would say it looks good to him too.

Will we need a follow-up for making the properties private and use the setters/getters everywhere?

$language->id (vs. $language→langcode) Was specificallyintroduced to cope with the Iangcode property of language config entities. So with this we'd have $language->getLangcode() != $language->langcode. think it would make more sense to stick to ID and have the method named getId()

@tstoeckler has a great point there, agreed :)

that fail looks not random. :)

PHP Fatal error:  Call to undefined method Drupal\Core\Language\Language::setLangcode() in /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php on line 60

I will take a try at fixing it.

ran the phpunit tests locally and they pass. (https://drupal.org/node/2116239)

just changed setLangcode to setId() and getId() in the unit test.

setting to needs review for the bot to run.

----
there is a pretty common ->id() method in other classes, that it has been decided will not be changed (yet?) to getId(). I dont know if that will effect us here.

also I think we can make some improvements similar to #2122175: String translation does not honor language fallback (around comment #56 and #57)
for example @group

there is a pretty common ->id() method in other classes, that it has been decided will not be changed (yet?) to getId(). I dont know if that will effect us here.

That's mostly on entities, those methods (id(), label(), etc) were introduced before we started using getters and setters for everything and prefixing those methods as such. This is not an entity, so we can do it right form the start without breaking anything.

a.
Re #48 and id(), see #2016679: Expand Entity Type interfaces to provide methods, protect the properties

@Xano, ok. :)

1. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -78,7 +74,7 @@ class Language {
   -  public $method_id = NULL;
   +  public $method_id;
   

   should we change this property to methodId while we are here? Or a separate follow-up?

   https://drupal.org/node/608152#naming
   "Methods and class properties should use lowerCamel naming."

   I did not change this.

2. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +88,124 @@ class Language {
   +   * Language constructor builds the default language object.
   ...
   +  public function __construct(array $options = array()) {
   

   this one line summary line is being changed (mm added) anyway, so might as well make it agree with https://drupal.org/node/1354#classes "Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides..."."

3. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +88,124 @@ class Language {
   +  public function setDefault($default) {
   +    $this->default = $default;
   +
   +    return $this;
   +  }
   

   this doesn't check if any other language is also set as the default, should that be checked as part of this set? could we end up with two that think they are defaults?

4. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * Get the name of the Language object.
   +   */
   +  public function getName();
   

   Gets (third person verb for methods again).

   Also missing @return.

5. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * Sets the name.
   +   *
   +   * @param string $name
   +   *
   +   * @return $this
   +   */
   +  public function setName($name);
   

   the @param needs a description
   https://drupal.org/node/1354#param

   (but @return $this has an exception and does not need a description. https://drupal.org/coding-standards/docs#types)

6. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * Gets the ID (language code).
   +   *
   +   * @return string
   +   */
   

   @return needs a description.

7. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * @param string $langcode
   +   *
   +   * @return $this
   +   */
   +  public function setId($id);
   

   I think the signature should match setId($id), so @param changed to be $id

8. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * Sets the direction of the language.
   +   *
   +   * @param string $direction
   +   *
   +   * @return $this
   

   $direction is an int, and should specify allowed values here also, like in getDirection.

9. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -0,0 +1,189 @@
   +   * Gets whether this language is the default language.
   +   *
   +   * @return bool
   +   */
   +  public function isDefault();
   

   Returns whether is more usual for isWhatever() methods.

10. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
    @@ -0,0 +1,93 @@
    +   * {@inheritdoc
    

    missing }

11. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
    @@ -0,0 +1,93 @@
    +  /**
    +   * @covers \Drupal\Core\Language\Language::getName()
    +   * @covers \Drupal\Core\Language\Language::setName()
    +   */
    +  public function testGetName() {
    

    gave these one line descriptions. https://drupal.org/node/1354#functions

12. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
    @@ -0,0 +1,93 @@
    +  }
    +}
    

    since this is new, might as well follow https://drupal.org/node/608152#indenting "Leave an empty line between end of method and end of class definition:"

13. Language.php

      /**
       * The language negotiation method used when a language was detected.
       *
       * @var bool
       *
       * @see language_types_initialize()
       */
      public $method_id;
    

    what? it's a boolean?
    is it the method that was used?
    or is it whether a negotiation method was used?
    there can be more than two negotiation methods... contrib can add them.

    also, there is no language_types_initialize().
    maybe it means language_language_types_info()?

    I think it's string and it's the TYPE constants that are moved by this patch to LanguageInterface.

    Maybe like:

       * @return string
       *   Types like TYPE_CONTENT, TYPE_INTERFACE, TYPE_URL.
    

    So I think we need to do a search and replace for updating all Language::WHATEVER_CONST's to LanguageInterface::WHATEVER_CONST's that were moved.

    Or.. maybe something like
    LanguageNegotiatorInterface::METHOD_ID

    Anyway, I've conviced myself it's a string, and I went with that METHOD_ID example. Need feedback on what this should be.

14. I didn't do a
    @coversDefaultClass \Drupal\Core\Language\Language
    on the class, and change all the @covers to shorter notation like
    * @covers ::getName()
    but we could.

Tests locally:
cd core
./vendor/bin/phpunit --group Language --strict
OK (12 tests, 38 assertions)

Next steps,
i.
get some answers/confirmations,
on points 1. 3. 13.

ii.
and try a version where the properties are protected to see if we are using the methods or accessing the properties directly elsewhere.
patch is small so I suspect we will do the conversion to using the methods in a follow-up? Also, change Language type hints to LanguageInterface type hints..
[update] I see @penyaskito asked the same in #41. We should write that in the summary.

weird. it was unpublished. I published it.

maybe it had to do with when I went to save, it warned me someone else had saved since me, and I should resave. I did the resave. Maybe the resave unpublished it?

#2193857: Issue node gets unpublished in case of a cross-post conflict

should we change this property to methodId while we are here? Or a separate follow-up?

We are not touching the Language class. We'll change implementations in a follow-up, so we can finally, possibly, rename those properties and make them protected.

this one line summary line is being changed (mm added) anyway, so might as well make it agree with (...)

Done.

this doesn't check if any other language is also set as the default, should that be checked as part of this set? could we end up with two that think they are defaults?

I'd say that's out of scope as it's not related to introducing the interface.

So I think we need to do a search and replace for updating all Language::WHATEVER_CONST's to LanguageInterface::WHATEVER_CONST's that were moved.

That's for the follow-up as well.

I didn't do a
@coversDefaultClass \Drupal\Core\Language\Language
on the class, and change all the @covers to shorter notation like
* @covers ::getName()
but we could.

Done.

1. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -70,7 +70,9 @@ class Language implements LanguageInterface {
   +   * The method ID, for example, LanguageNegotiatorInterface::METHOD_ID.
   

   Always use fully-qualified class names.

2. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -97,7 +97,10 @@
   +   *   The human readable English name of the Language.
   

   The word "language" should not be capitalized.

3. +++ b/core/lib/Drupal/Core/Language/LanguageInterface.php
   @@ -114,13 +118,15 @@ public function setName($name);
   +   *   The langcode.
   

   "langcode" is not a word. When writing human-readable texts, use "language code".

The previous patch did not include all changes.

lets see if testbot picks up the new patch.

55: drupal_1512424_55.patch queued for re-testing.

55: drupal_1512424_55.patch queued for re-testing.

I'm reviewing this.

Only thing I noticed was:

1. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -51,11 +47,11 @@ class Language {
   -   * Defined using constants, either DIRECTION_LTR or const DIRECTION_RTL.
   +   * Defined using constants, either self::DIRECTION_LTR or self::DIRECTION_RTL.
       *
       * @var int
       */
   -  public $direction = Language::DIRECTION_LTR;
   +  public $direction = self::DIRECTION_LTR;
   

   I'm not sure we do self like this.
   Maybe just LanguageInterface

   checked https://drupal.org/node/1354
   but didn't see it there.

   might be a bit related to #2158497: [policy, no patch] Use "$this" and "static" in documentation for @return types

do we want to get someone else to check the unit tests or something?

@csg is making the follow-ups.
we need someone to update the change record: https://drupal.org/node/1647388 "language_list() and language_default() return Language objects"

otherwise, once those are done, I think this could be rtbc.

I'm not sure we do self like this.
Maybe just LanguageInterface

I disagree, as that would make renaming the class much harder in the future.

we need someone to update the change record: https://drupal.org/node/1647388

Do we update existing change records before issues are fixed? That is confusing.

I agree with #61, self::DIRECTION_LTR and so forth are fine, to my mind.

I have some remarks:

1. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -74,11 +70,12 @@ class Language {
   +  public $method_id;
   

   Why not methodId?

2. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +89,124 @@ class Language {
   +    // Set all the provided properties for the language.
   +    foreach ($options as $name => $value) {
   +      $this->{$name} = $value;
   +    }
   

   There's not *that* much stuff there, right? Can we just be explicit and apply only those values that make sense?

   Maybe that would be too much work but we could also change to multiple constructor arguments, i.e. one argument for each value?

3. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +89,124 @@ class Language {
   +    if (!isset($options['name']) || !isset($options['direction'])) {
   +      $predefined = LanguageManager::getStandardLanguageList();
   

   I think this optimization to call ::getStandardLanguageList() only conditionally makes the code quite a bit harder to read. Are we sure that that is actually necessary?

4. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +89,124 @@ class Language {
   +      if (isset($predefined[$this->id])) {
   

   This for instance bothers me, because $this->id is not being set above regarding the lack of explicitness above: it's just being assumed that 'id' has been passed in $options. Or am I missing something?

5. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +89,124 @@ class Language {
   +          $this->name = $predefined[$this->id][0];
   ...
   +          $this->direction = $predefined[$this->id][2];
   

   Since we have called ::getStandardLanguageList() anyway, can we make this more clear with the list() syntax?

   I.e.

   list($name, $whatever, $direction) = LM::gSL();
   

6. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -92,112 +89,124 @@ class Language {
   -  public function __construct(array $options = array()) {
   -    // Set all the provided properties for the language.
   -    foreach ($options as $name => $value) {
   -      $this->{$name} = $value;
   -    }
   -    // If some options were not set, set sane defaults of a predefined language.
   -    if (!isset($options['name']) || !isset($options['direction'])) {
   -      $predefined = LanguageManager::getStandardLanguageList();
   -      if (isset($predefined[$this->id])) {
   -        if (!isset($options['name'])) {
   -          $this->name = $predefined[$this->id][0];
   -        }
   -        if (!isset($options['direction']) && isset($predefined[$this->id][2])) {
   -          $this->direction = $predefined[$this->id][2];
   -        }
   -      }
   -    }
   

   Ahh, so you just copied that over. Feel free to ingore in that case, then.

   (But also feel free to fix if you want :-))

7. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
   @@ -0,0 +1,107 @@
   +/**
   + * @coversDefaultClass \Drupal\Core\Language\Language
   + *
   + * @group Drupal
   + * @group Language
   + */
   +class LanguageUnitTest extends UnitTestCase {
   

   At least a one-line description would be nice.

8. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
   @@ -0,0 +1,107 @@
   +      'description' => '',
   

   ?

9. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
   @@ -0,0 +1,107 @@
   +    $this->assertSame(spl_object_hash($this->language), spl_object_hash($this->language->setName($name)));
   ...
   +    $this->assertSame(spl_object_hash($this->language), spl_object_hash($this->language->setId($language_code)));
   ...
   +    $this->assertSame(spl_object_hash($this->language), spl_object_hash($this->language->setDirection($direction)));
   ...
   +    $this->assertSame(spl_object_hash($this->language), spl_object_hash($this->language->setDefault($default)));
   ...
   +    $this->assertSame(spl_object_hash($this->language), spl_object_hash($this->language->setNegotiationMethodId($method_id)));
   

   Is there any reason for the spl_object_hash()?

Just talked to @Xano again. So 1. is because we're still leaving the BC shiv in with this issue and want to convert usages in a follow-up. Once $method_id is protected we can then rename it.

2.-5. is invalidated by 6.

@Xano likes 9. because the error messages are nicer. I disagree that that's a good reason, but I don't really care either, so I'm fine with it.

We should fix 7. and 8., however.

I have created the follow-up issue, and already started working on it: #2226533: Changes to the Language class due to the LanguageInterface (followup)

At least a one-line description would be nice.

We already have the annotation that says exactly what the method does.

?

Descriptions should never be required if there is a title or a label already, and the test name returned by getInfo() already says what the test does. However, Simpletest is still dumb enough as to require a description technically, so that's why it's set, but empty. You can ask @dawehner for a second opinion.

Is there any reason for the spl_object_hash()?

It's something I taught myself to prevent Simpletest from crashing when serializing and storing large objects, but for PHPUnit it's not that necessary. I removed the usage so the asserts compare objects instead of their hashes.

With regards to everything else: this issue is about introducing the interface. We will convert all dependents in follow-ups and remove public property access and rename properties after core no longer depends on them. For the same reason I did not clean up the constructor.

Re #61, about if we need to update the change record before marking this rtbc...

Yeah, that is an *existing* change record, so we are not making a "draft".
In this case though the body is pretty much saying see this issue.

So lets put the text that we will be putting on that change record into this issue summary.
Then we will know before committing what will be there and that it will already be written.

OK. looks like all the concerns have been agreed to, or addressed, and we have the draft change to the change record, and also the follow-up to use this.
Thanks @tstoeckler for the review.

Is there any reason for the spl_object_hash()?

It's something I taught myself to prevent Simpletest from crashing when serializing and storing large objects, but for PHPUnit it's not that necessary. I removed the usage so the asserts compare objects instead of their hashes.

Yes, in PHPUnit tests, just compare the objects. (Although PHPUnit provides many nice assertion helper methods, perhaps you find a more suitable one.)

Btw, not sure whether assertSame() performs the intended comparison here? (strict identical comparison vs. lossy equality)

FYI: In Simpletest tests, you should use the assertIdenticalObject() method.

assertSame() means strict equality: http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-t...

Committed c242313 and pushed to 8.x. Thanks!