Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.2600078.18.20.txt | 787 bytes | YesCT |
#20 | 2600078.20.patch | 2.33 KB | YesCT |
#18 | interdiff.2600078.16.18.txt | 804 bytes | YesCT |
#18 | 2600078.18.patch | 2.5 KB | YesCT |
#17 | interdiff.2600078.11.16.txt | 1.16 KB | YesCT |
Comments
Comment #2
koppie CreditAttribution: koppie at Pantheon commentedOld:
New:
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?
Comment #3
cilefen CreditAttribution: cilefen commentedWe usually open wrong documentation as bugs.
Comment #4
YesCT CreditAttribution: YesCT commentedThe 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
makes me wonder if this is an array (keyed by integers) of arrays of TranslatorInterface objects...
looking into this more.
Comment #5
koppie CreditAttribution: koppie at Pantheon commentedI'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:
I've rolled a new patch against HEAD. I also rolled an interdiff to show the difference from #2.
Comment #6
YesCT CreditAttribution: YesCT commentedthanks for the interdiff.
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
Comment #7
koppie CreditAttribution: koppie at Pantheon commentedI agree with everything @yesct said so I'm re-rolling the patch. Summary of changes from HEAD:
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.
Comment #9
kgoel CreditAttribution: kgoel as a volunteer commentedNo interdiff. Patch in #7 failed because of not pulling head before creating patch.
Comment #10
YesCT CreditAttribution: YesCT commentedI 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.
Comment #11
kgoel CreditAttribution: kgoel as a volunteer commented@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.
Comment #12
YesCT CreditAttribution: YesCT commentedcan the sorted array be null also?
Comment #13
jhodgdonThe 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).
Comment #15
xjmWe 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
If it is associative, that seems to be the opposite of unkeyed?
We discussed this
\Object[][]
type syntax at length yesterday.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!
Comment #16
YesCT CreditAttribution: YesCT commentedI'll work on this right now.
Comment #17
YesCT CreditAttribution: YesCT commentedFirst 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
gives output:
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
which triggers "a rebuild" ... which might mean just resort.
and similar in
\Drupal\Core\StringTranslation\TranslationManager::reset
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.
Comment #18
YesCT CreditAttribution: YesCT commentedok. 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.
Comment #19
YesCT CreditAttribution: YesCT commentedif 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.
Comment #20
YesCT CreditAttribution: YesCT commentedok, 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.
Comment #21
Kazanir CreditAttribution: Kazanir commentedLooks good to me! :)
Comment #23
jhodgdon+1 Thanks!
Comment #26
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!