Drupal::languageManager()->getLanguage(Language::TYPE_INTERFACE) is a bit ugly to use.

Let's add helper methods for the hardcoded types (the Language::TYPE_* constants), e.g. getInterfaceLanguage(). Method should be very simple, just call getLanguage($type) internally.

This saves a use of the Language class where we just need it for that and is much easier to understand with method autocompletion.

CommentFileSizeAuthor
#14 language-type_ui-2020301-7.patch916 bytesBerdir
#7 language-type_ui-2020301-7.do_not_test.patch916 bytesplach
#4 language-type_ui-2020301-3.do_not_test.patch842 bytesplach
#3 language-type_ui-2020301-2.patch616 bytesplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Active

I was thinking about something similar too, but since most calls refer to the interface language, what about just making the $type parameter optional and the defaulting to it? Among the rest this would avoid special-casing the three core language types, since contrib ones couldn't have a dedicated method.

Things would look like this in (all?) most core procedural code:

<?php
$language = Drupal::languageManager()->getLanguage();
?>
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +language-base

That's fine with me as well. The only disadvantage that I can come up with is that people might be less aware of the fact that there can be multiple languages.. interface, content, ...But as you said, in 9x% of the cases, you want the interface language and the people that need to explicitly access the content language or so probably know what they are doing :)

That makes this issue even easier, just add a default value to the method.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
language-type_ui-2020301-2.patch616 bytes

Could be as easy as this, if we don't want to piss everyone off with yet another language patch screwing all the others on the queue ;)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
language-type_ui-2020301-3.do_not_test.patch842 bytes

Fixing PHP docs would be even better ;)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I completely agree with #2, reached the same conclusions.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/lib/Drupal/Core/Language/LanguageManager.phpundefined
@@ -76,12 +76,12 @@ public function setRequest(Request $request) {
-   *   The language type, e.g. Language::TYPE_INTERFACE.
+   *   (optional) The language type. Defaults to Language::TYPE_INTERFACE.

I think the reference here should be fully qualified even when it's in the same namespace.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: Add helper methods to simplify usage of LanguageManager::getLanguage($type) » Make the $type parameter of LanguageManager::getLanguage() optional to improve DX
Status: Active » Needs review
FileSize
language-type_ui-2020301-7.do_not_test.patch916 bytes

rrright

Status: Needs review » Needs work
Issue tags: -Novice, -D8MI, -language-base

The last submitted patch, language-type_ui-2020301-2.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
Issue tags: +Novice, +D8MI, +language-base

#3: language-type_ui-2020301-2.patch queued for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

I knew you were kidding me, bot :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

RTBC from my side. Maybe Gabor wants to RTBC it, so that we have a third person in here?

Also, a bit unusual to use do-not-test for actual patches, even if they just change documentation. I can cancel patches if you ping me in IRC :) Or a few other people.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +sprint

Sounds good, putting on sprint for him.

I can cancel patches if you ping me in IRC :) Or a few other people.

noted :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

#7 should be reposted so testbot tests it. That prior versions passed earlier (with identical code changes) does not actually prove that it would currently pass.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
language-type_ui-2020301-7.patch916 bytes

Re-uploading that patch. All credit goes to @plach :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Reviewed & tested by the community

Looks like a good DX improvement.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs review

Do we need a follow-up to remove the argument from existing calls? We need one to convert language() to Drupal::languageManager()->getLanguage() anyway, can do it as part of that I guess?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Crosspost, I think.

Do we need a follow-up to remove the argument from existing calls? We need one to convert language() to Drupal::languageManager()->getLanguage() anyway, can do it as part of that I guess?

Yep, makes sense.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Patch looks fine, as does converting all the calls in a follow-up. Committed/pushed to 8.x, thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Make the $type parameter of LanguageManager::getLanguage() optional to improve DX » Change notice: Make the $type parameter of LanguageManager::getLanguage() optional to improve DX
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

https://drupal.org/node/1450578 looks quite outdated and should be updated, not just for this change but also Drupal::languageManager(). There might be additional change notices related to this as well..

elachlan’s picture

elachlan CreditAttribution: elachlan commented
Status: Active » Needs work

I changed it a little bit to what I was told in IRC. Thanks Berdir!!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

What other changes are needed? https://drupal.org/node/1450578/revisions/view/2574684/2733887 was the change for reference.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

We might want to note that different language types can be passed to LanguageManager::getLanguage().

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes, there's still a reference to the old constant in the description and we should probably list the types that core supports as plach said. Searching for the constant, there are a also two other change notices that should be updated: https://drupal.org/list-changes?keywords_description=LANGUAGE_TYPE_INTER...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Change notice: Make the $type parameter of LanguageManager::getLanguage() optional to improve DX » Make the $type parameter of LanguageManager::getLanguage() optional to improve DX
Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record

Done: https://drupal.org/node/1450578/revisions/view/2733907/2734655

Also updated the references in the other two.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +Needs change record

Looks good, thanks.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -Needs change record
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

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