Problem/Motivation

Documentation at the top of core/lib/Drupal/Core/StringTranslation/TranslationManager.php uses the wrong syntax. See:

Proposed resolution

Fix @var definition to match Drupal standards (see above)

Remaining tasks

Fix @var definition to match Drupal standards (see above)

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

koppie created an issue. See original summary.

koppie’s picture

Status: Active » Needs review
FileSize
697 bytes

Old:

   * @var array
   *   Array of \Drupal\Core\StringTranslation\Translator\TranslatorInterface objects

New:

   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface[]

This complies with Drupal coding standards.

Edit: However, I suspect the documentation is wrong in content and not just in form. It looks like it might actually be an array of arrays. Can anyone provide input?

cilefen’s picture

Component: base system » documentation
Category: Task » Bug report
Issue tags: +rc eligible

We usually open wrong documentation as bugs.

YesCT’s picture

Assigned: koppie » Unassigned
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -18,8 +18,7 @@ class TranslationManager implements TranslationInterface, TranslatorInterface {
+   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface[]

The standards change looks perfect.

but since we are changing this line anyway, I wanted to check to see if that was actually the type of the thing.

looking at

  public function addTranslator(TranslatorInterface $translator, $priority = 0) {
    $this->translators[$priority][] = $translator;

makes me wonder if this is an array (keyed by integers) of arrays of TranslatorInterface objects...

looking into this more.

koppie’s picture

I'm modifying my own patch. It turns out the documentation as it was written wasn't correct. $translators isn't an array; it's an array of arrays. To whit:

   * An associative array. The keys are integers that indicate priority.
   * Values are arrays of TranslatorInterface objects.

I've rolled a new patch against HEAD. I also rolled an interdiff to show the difference from #2.

YesCT’s picture

Status: Needs review » Needs work

thanks for the interdiff.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -16,10 +16,10 @@
-   * An array of active translators keyed by priority.
+   * An associative array. The keys are integers that indicate priority.
+   * Values are arrays of TranslatorInterface objects.

yeah, as @xjm mentioned (irl), we need a one line summary, and then a paragraph with more information.

https://www.drupal.org/node/1354#drupal

after making the one line, and the paragraph, let's make sure lines are wrapped as closely to 80 chars as possible

-

also, I think there are similar places in this class that need the same change like:
$sortedTranslators
sortTranslators

koppie’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
1.96 KB

I agree with everything @yesct said so I'm re-rolling the patch. Summary of changes from HEAD:

  1. Improved documentation for $translators
  2. Improved documentation for $sortedTranslators
  3. Improved documentation for sortTranslators()

All documentation follows Drupal & PHP standards and also wraps as close as possible to 80 columns. I'm also including a new interdiff so you can see what's different from the previous patch.

Status: Needs review » Needs work

The last submitted patch, 7: correct_syntax_for_data-2600078-7.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

No interdiff. Patch in #7 failed because of not pulling head before creating patch.

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -16,20 +16,25 @@
-   * @var array
-   *   An array of path processor objects.
+   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface[][]

@@ -75,8 +80,8 @@ public function addTranslator(TranslatorInterface $translator, $priority = 0) {
-   * @return array
-   *   A sorted array of translators objects.
+   * @return \Drupal\Core\StringTranslation\Translator\TranslatorInterface[][]
+   *   A sorted array of arrays of translators objects.

I think putting the type in the @return is a good thing. But, I think this is not an array of arrays of things. I think the merge in this function makes the result an array of things.

kgoel’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
1.72 KB

@jhodgdon and I looked into sortTranslators() and other methods in TranslationManager.php class. The previous comment is correct. foreach loop in sortTranslators() is flattening the array inputs into array_merge() so the result is one level array.

YesCT’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -34,7 +34,7 @@ class TranslationManager implements TranslationInterface, TranslatorInterface {
-   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface[][]
+   * @var null|\Drupal\Core\StringTranslation\Translator\TranslatorInterface[]

@@ -80,8 +79,8 @@ public function addTranslator(TranslatorInterface $translator, $priority = 0) {
+   * @return \Drupal\Core\StringTranslation\Translator\TranslatorInterface[]
+   *   A sorted array of translator objects.

can the sorted array be null also?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

The sorted member variable is set to NULL sometimes to indicate "Time to sort again". But the return value of the sort method is always an array (sometimes could be empty, but it is still array() and not NULL).

The last submitted patch, 7: correct_syntax_for_data-2600078-7.patch, failed testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -16,20 +16,25 @@
    -   * An array of active translators keyed by priority.
    +   * Holds the array of translators - unsorted.
    

    We are losing the word "active" here. Also, member variables actually don't need to start with a verb per our standards -- just classes, functions, and methods -- so I am not sure if this change is necessary. Reference: https://www.drupal.org/node/1354#var

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -16,20 +16,25 @@
    +   * An associative array of TranslatorInterface objects. It is unkeyed. If this
    +   * is NULL a rebuild will be triggered.
    

    If it is associative, that seems to be the opposite of unkeyed?

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -16,20 +16,25 @@
    +   * An associative array. The keys are integers that indicate priority. Values
    +   * are arrays of TranslatorInterface objects.
    +   *
    +   * @var \Drupal\Core\StringTranslation\Translator\TranslatorInterface[][]
    

    We discussed this \Object[][] type syntax at length yesterday.

    • Conceptually, this indicates this is an array that contains arrays of TranslatorInterface objects. Which is correct (I think).
    • We have a few places in core that do this already.
    • @alexpott confirmed that it is implicitly supported in PHPStorm (using this allows PHPStorm to make context-appropriate method suggestions when this is used).
    • @alexpott also checked and found that it is not explicitly supported in the PHPdoc standards. Reference: http://www.phpdoc.org/docs/latest/references/phpdoc/types.html#arrays
    • @dawehner checked the PSR-5 standards and it says this:
      
      type             = class-name / keyword / array
      array            = (type / array-expression) "[]" / generic
      array-expression = "(" type-expression ")"
      generic          = collection-type "<" [type-expression "," *SP] type-expression ">"
      

      https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpd...

      So parsing through that, it does look like the syntax is at least implicitly supported.

    Based on all that, I'm in favor of using this, especially with the clear earlier paragraph that explains the structure in words as well.

NW for points 1 and 2. Thanks!

YesCT’s picture

I'll work on this right now.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
1.16 KB

First I just changed the docs, attaching an interdiff for that.

=====
but then I started thinking.... and reading: http://php.net/manual/en/function.array-merge.php

I'm still not sure about the unkeyed part. looks like it is keyed when I try a sample example.

https://3v4l.org/tSid3

$priority = 1;
$translator = "translator_1";
$translators[$priority][] = $translator;
$priority = 2;
$translator = "translator_2";
$translators[$priority][] = $translator;
$priority = 1;
$translator = "translator_1a";
$translators[$priority][] = $translator;
var_dump($translators);
 
$sorted = array();
krsort($translators);

foreach ($translators as $each_translators) {
 $sorted = array_merge($sorted, $each_translators);
}
var_dump($sorted);

gives output:

array(2) {
  [1]=>
  array(2) {
    [0]=>
    string(12) "translator_1"
    [1]=>
    string(13) "translator_1a"
  }
  [2]=>
  array(1) {
    [0]=>
    string(12) "translator_2"
  }
}
array(3) {
  [0]=>
  string(12) "translator_2"
  [1]=>
  string(12) "translator_1"
  [2]=>
  string(13) "translator_1a"
}

so $sorted is keyed, just starts at zero and counts up. (not numbers representing priority)

is there test coverage on the sort function?

find usages says only two calls:
\Drupal\Core\StringTranslation\TranslationManager::getStringTranslation

  public function getStringTranslation($langcode, $string, $context) {
    if ($this->sortedTranslators === NULL) {
      $this->sortedTranslators = $this->sortTranslators();
    }
...

which triggers "a rebuild" ... which might mean just resort.

and similar in
\Drupal\Core\StringTranslation\TranslationManager::reset

  public function reset() {
    if ($this->sortedTranslators === NULL) {
      $this->sortedTranslators = $this->sortTranslators();
    }
...

and no test coverage... directly.

But getStringTranslation() seems pretty important, that it find the first translation available, which should depend on the order of $this->sortedTranslators

and it has two usages (not in tests)
\Drupal\Core\StringTranslation\TranslationManager::doTranslate
and
\Drupal\Core\StringTranslation\TranslationManager::getStringTranslation
but... that call in there, is not to itself, but to things that implement TranslatorInterface, we have two in core
\Drupal\Core\StringTranslation\Translator\StaticTranslation::getStringTranslation
\Drupal\locale\LocaleTranslation::getStringTranslation

and... not finding calls to those directly in tests either, but at this point, it is not working on an array of arrays anyway, but on one each.

========

so, I think we need to make sure this doc is correct and think some more and maybe get some input on how it is working and where test coverage might be.

YesCT’s picture

ok. so the sorted array, element zero will be (one of) the highest priority translator. which is why it is a krsort. cause the sorted array is sorted by priority, not by weight. it is keyed numerically, starting with 0 and go down. ... which is what it is intended to do: find the highest priority translator first. (In my example, translator_2 had a priority of 2, and the others had a priority of 1. so translator_2 has the highest priority and is the value for the 0th element in the sorted array)

so... first a patch that writes that down.

YesCT’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -16,20 +16,28 @@
-   * Holds the array of translators sorted by priority.
+   * An array of translators, sorted by priority.
+   *
+   * The array is keyed numerically, but the keys are not equivalent to the
+   * priority values. For example, array element 0 will be the highest priority
+   * translator.
    *

if good documentation is taking out words while still being correct and understandable, then maybe this "The array is keyed.." paragraph should be removed. Because it is implied by the one line before it.

though I would lean toward leaving the paragraph in, because I think there could be some confusion that the keys are the priority, and they are not literally the priority values.

YesCT’s picture

ok, I did some more little test php snipits and decided I was just explaining how php works... so I took it out.

I think this is really ready now.

Kazanir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me! :)

The last submitted patch, 7: correct_syntax_for_data-2600078-7.patch, failed testing.

jhodgdon’s picture

+1 Thanks!

  • catch committed 72ae69d on 8.1.x
    Issue #2600078 by YesCT, koppie, kgoel: Correct syntax for data type...

  • catch committed e9c618e on
    Issue #2600078 by YesCT, koppie, kgoel: Correct syntax for data type...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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