Over in #2226533: Changes to the Language class due to the LanguageInterface (followup)

we have made the mistake of creating a monster patch that needs a daily reroll -

it is upto 300 comments and the patch size is an unreviewable 260Kb

Other issues have been spawned to cut down the beast... but from here

https://www.drupal.org/node/2226533#comment-9162179

I want to break out all the annotation improvements into a more digestible form,

CommentFileSizeAuthor
#5 interdiff-3-5.txt873 bytesmartin107
#5 language-2341927-5.patch5.43 KBmartin107
#3 interdiff-1-3.txt2.23 KBmartin107
#3 language-2341927-3.patch5.13 KBmartin107
#1 language-2341927-1.patch4.23 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
language-2341927-1.patch4.23 KB

All looks ok to me....famous last words.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

This looks suspicious to me:

   /**
    * List of langcodes.
    *
-   * @var array
+   * @var \Drupal\Core\Language\LanguageInterface[]
    */
   protected $langcodes = array();

Is this perhaps string[] ?

And until we change our coding standards

+  /**
+   * @var \Drupal\Core\Language\LanguageInterface[]
+   */
   protected $languages;

this isn't a proper doc block... needs one-line description.

The rest looks good to me...

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review
FileSize
language-2341927-3.patch5.13 KB
interdiff-1-3.txt2.23 KB

I have fixed both issues ...

Regarding, the correct annotation for $langcode,

The situation is horribly confused,... and so thank you for making me double check.

If you look in the setup function, at the assignment of $langcode items ...

$this->$langcodes[$i] = $language

.. which is a line builds up an array of \Drupal\Core\Language\Language objects

which implies the that

* @var \Drupal\Core\Language\LanguageInterface[]

is the correct annotation .... BUT the real objection is WHY is a store of language objects called 'langcodes'?

the answer is that $langcode array is only ever used to pull out what is in effect the langcode property from within the selected language. ( the id )

So my fix is simplify and to store only what we want, not store a container holding what we want.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » language.module

Well technically these are both class member variables and should have doc blocks, not one line @var statements (which are supposed to be only for local variables):

+  /** @var \Drupal\Core\Language\LanguageInterface[] */
   protected $languages;
   protected $languageManager;

Other than that, this looks fine. It's not just documentation any more though.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Title: Language module annotation improvements. » Language module annotation improvements [ plus minor followup ]
FileSize
language-2341927-5.patch5.43 KB
interdiff-3-5.txt873 bytes

@jhodgdon .. thanks again.

The next d8MI meeting is not for a week... I will try and get other eyes on this before then.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

This all looks good to me now. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed d4b7bc7 on 8.0.x 
    Issue #2341927 by martin107: Language module annotation improvements [...

Status: Fixed » Closed (fixed)

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