In #1497230: Use Dependency Injection to handle object definitions , we introduced a basic Language class to keep backwards compatibility with stdClass. We should turn this into a full blown class with proper scope for its variables, and set/get method.

Remaining Tasks

Draft change record text:

LanguageInterface for language value objects

We know have \Drupal\Core\Language\LanguageInterface for classes that represent a language. The primary class that implements this interface is \Drupal\Core\Language\Language. All code that uses this class must be converted to depend on the interface and its methods instead of depending on the class and accessing public properties. Example:
Before (Drupal 8):

<?php
function foo(Language $language) {
  return
$language->direction;
}
?>

After (Drupal 8):

<?php
function foo(LanguageInterface $language) {
  return
$language->getDirection();
}
?>
Files: 
CommentFileSizeAuthor
#65 interdiff.txt2.1 KBXano
#65 drupal_1512424_64.patch15.44 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,471 pass(es).
[ View ]
#44 drupal_1512424_44.patch14.57 KBXano
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

plach’s picture

Issue tags:+D8MI, +language-base
RobLoach’s picture

Status:Postponed» Active
Gábor Hojtsy’s picture

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

RobLoach’s picture

<?php
 
/**
     * Returns the name of the Language.
     *
     * @return string The name of the language.
     */
   
public function getName()
    {
       return
$this->name;
    }

    public function
setName($name) {
      
$this->name = $name;
    }
?>

... for each property.

Gábor Hojtsy’s picture

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.

sun’s picture

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?

RobLoach’s picture

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

Just Encapsulation and better documentation.

sun’s picture

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

RobLoach’s picture

Priority:Major» Normal

That's why this was pushed to a follow up issue rather than doing it in the initial container patch. I wouldn't consider this major either, moving to normal.

ebeyrent’s picture

I'd like to see something like:

<?php
 
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:

<?php
 
$language
= Drupal\Core\Language\Language::getInstance()->setName('English')->setLangcode('en');
?>
pounard’s picture

Or we could simply make an interface:

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

Gábor Hojtsy’s picture

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

pounard’s picture

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.

Gábor Hojtsy’s picture

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

pounard’s picture

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

RobLoach’s picture

Having a LanguageInterface does make sense, but the Language class be a singleton/factory is extremely the wrong way about it. We want to have the object held in the Dependency Injection $container so that there is proper scope within the Drupal context. When the Kernel patch hits, we want to be able to have multiple Drupal instances running at any given time. Using singletons or a factory will block that from happening, hence our moving towards dependency injection.

pounard’s picture

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

ebeyrent’s picture

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.

RobLoach’s picture

Tagging.

Gábor Hojtsy’s picture

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.

ebeyrent’s picture

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

RobLoach’s picture

Status:Active» Needs work
StatusFileSize
new4.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1512424.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is no where near complete, but it is a start. We'd eventually want proper scope for the object, but we might want to do that later on as it'll be touching lots of files.

Notes:

  1. Know if we can use {@inheritDocs} to bring in the documentation from LanguageInterface for those functions? Or do we just use a @see instead?
  2. We will type-hint LanguageInterface in functions and uses of it so that contrib could easily switch the Language classes if they wanted.
  3. What is the method_id? Wasn't sure what to put for the docs there. The others needs lots of docs as well.
Gábor Hojtsy’s picture

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.

beejeebus’s picture

i came to this issue from Gábor's comment over in #1862202: Objectify the language system - just cross linking to make sure we're all on the same page.

plach’s picture

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

plach’s picture

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

Gábor Hojtsy’s picture

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.

ebeyrent’s picture

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

Gábor Hojtsy’s picture

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

YesCT’s picture

jair’s picture

Status:Needs work» Needs review
Issue tags:+Needs reroll

The last submitted patch, 1512424.patch, failed testing.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

penyaskito’s picture

Is this something that we should do before beta?

Gábor Hojtsy’s picture

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

Xano’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 62,915 pass(es).
[ View ]

Re-roll, and I added a PHPUnit test.

Gábor Hojtsy’s picture

Title:Turn Language into a full blown class» Add property setters/getters to Language class
Status:Needs review» Needs work

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.

Xano’s picture

Title:Add property setters/getters to Language class» Add a LanguageInterface, and property setters/getters to Language class
Status:Needs work» Needs review
StatusFileSize
new14.6 KB
PASSED: [[SimpleTest]]: [MySQL] 62,947 pass(es).
[ View ]
new2.14 KB

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

penyaskito’s picture

Status:Needs review» Reviewed & tested by the community

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

penyaskito’s picture

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

tstoeckler’s picture

Status:Reviewed & tested by the community» Needs work

$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()

Gábor Hojtsy’s picture

@tstoeckler has a great point there, agreed :)

Xano’s picture

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

Status:Needs review» Needs work

The last submitted patch, 44: drupal_1512424_44.patch, failed testing.

The last submitted patch, 44: drupal_1512424_44.patch, failed testing.

YesCT’s picture

Assigned:Unassigned» YesCT

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.

YesCT’s picture

Assigned:YesCT» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.04 KB
new14.41 KB
PASSED: [[SimpleTest]]: [MySQL] 64,320 pass(es).
[ View ]

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

Xano’s picture

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.

YesCT’s picture

StatusFileSize
new15.67 KB
PASSED: [[SimpleTest]]: [MySQL] 64,436 pass(es).
[ View ]
new5.83 KB

a.
Re #48 and id(), see #2016679: [Meta] 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.

YesCT’s picture

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?

sun’s picture

Xano’s picture

StatusFileSize
new15.89 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new2.47 KB

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

Status:Needs review» Needs work

The last submitted patch, 53: drupal_1512424_53.patch, failed testing.

Xano’s picture

StatusFileSize
new15.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,250 pass(es).
[ View ]
new4.9 KB

The previous patch did not include all changes.

YesCT’s picture

Status:Needs work» Needs review

lets see if testbot picks up the new patch.

Xano’s picture

55: drupal_1512424_55.patch queued for re-testing.

Xano’s picture

55: drupal_1512424_55.patch queued for re-testing.

YesCT’s picture

I'm reviewing this.

YesCT’s picture

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.

Xano’s picture

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.

tstoeckler’s picture

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.

    <?php
    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()?

tstoeckler’s picture

Status:Needs review» Needs work

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.

csg’s picture

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

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new15.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,471 pass(es).
[ View ]
new2.1 KB

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.

YesCT’s picture

Issue summary:View changes

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.

Xano’s picture

Issue summary:View changes
YesCT’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

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.

sun’s picture

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.

pounard’s picture

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed c242313 and pushed to 8.x. Thanks!

  • Commit c242313 on 8.x by alexpott:
    Issue #1512424 by Xano, YesCT, RobLoach: Add a LanguageInterface, and...

Status:Fixed» Closed (fixed)

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