Page MenuHomePhabricator

ZMultiLingualString->internalGetStringForLanguage may return a structure to build the expected value (with or without fallback / placeholder)
Closed, ResolvedPublic

Description

I see that ZMultiLingualString->internalGetStringForLanguage returns a structure with a title and a language string:
~~https://gerrit.wikimedia.org/g/mediawiki/extensions/WikiLambda/+/89c663692f8a4503bf4f0f293af4e3ea290f9fae/includes/ZObjects/ZMultiLingualString.php#205~~

But it sounds to me familiar with ZMonoLingualString (which holds a Z_MONOLINGUALSTRING_LANGUAGE and a Z_MONOLINGUALSTRING_VALUE properties). Does it make sense to return an instance of this class or not? If it does, I can open an issue and provide a pull-request :)


I would tend to return a structure which would look like the Message class, so with it we may:

  • define a placeholder text to use (instead of the use of $isTItle + $returnPlaceholder) using a setPlaceholder($placeholder: string) function;
  • set a flag so we may fallback to English or not (LanguageFallback::MESSAGES VS LanguageFallback::STRICT) ;
  • get the return type we want using a method (like value() to get the string value only and zMonolingualString() to get the whole ZMonolingual instance)

This would remove the flag arguments and remove the getStringForLanguage, getStringForLanguageOrEnglish and getStringAndLanguageCode methods. This would, I would say, make the code more expressive in the call sites.

Here is an example for how we may lookup a string given a locale:

// instead of using getStringAndLanguageCode
$stringAndLanguage = $zMultiLingualString
   ->buildStringForLanguage($lang) // This returns some structure which implements the below methods (`fallbackWithEnglish()`, `placeholderWith()`, `getStringAndLanguageCode()`)
   ->fallbackWithEnglish()
   ->placeholderWith('wikilambda-editor-default-name') // we may otherwise add a `placeholderForTitle()` method
   ->getStringAndLanguageCode();

// No placeholder nor english fallback
$strictStringAndLanguage = $zMultiLingualString->buildStringForLanguage($lang)->getStringAndLanguageCode();

// instead of using getStringForLanguage
$string = $zMultiLingualString
  ->buildStringForLanguage($lang)
  ->placeholderWith('wikilambda-multilingualstring-nofallback') // or `placeholderNoFallback()`
  ->getString();

Event Timeline

Also, I would tend to return a structure which would look like the Message class, so with it we may:

  • define a placeholder text to use (instead of the use of $isTItle + $returnPlaceholder) using a setPlaceholder($placeholder: string) function;
  • set a flag so we may fallback to English or not (LanguageFallback::MESSAGES VS LanguageFallback::STRICT) ;
  • get the return type we want using a method (like value() to get the string value only and zMonolingualString() to get the whole ZMonolingual instance)

This would remove the flag arguments and remove the getStringForLanguage, getStringForLanguageOrEnglish and getStringAndLanguageCode methods. This would, I would say, make the code more expressive in the call sites.

Response from IRC:

Yes, the response is a pair of a string that is a language code and a string which is the label to display for that language's look-up, including language fall-back walking. This is conceptually different from a real ZMonoLingualString, but mostly we don't go through the full ZObject stack for the internal method as that's a lot slower because it cares about other things that aren't needed.

Thanks for your response @Jdforrester-WMF! I don't see the overhead that would be implied when returning a ZMonoLingualString, but it's wise that I trust your analysis :-).

What about this proposal, which still can be implemented: https://phabricator.wikimedia.org/T313712#8101311

As a complement of the comment, here is an example for how we may lookup a string given a locale:

// instead of using getStringAndLanguageCode
$stringAndLanguage = $zMultiLingualString
   ->buildStringForLanguage($lang) // This returns some structure which implements the below methods (`fallbackWithEnglish()`, `placeholderWith()`, `getStringAndLanguageCode()`)
   ->fallbackWithEnglish()
   ->placeholderWith('wikilambda-editor-default-name') // we may otherwise add a `placeholderForTitle()` method
   ->getStringAndLanguageCode();

// No placeholder nor english fallback
$strictStringAndLanguage = $zMultiLingualString->buildStringForLanguage($lang)->getStringAndLanguageCode();

// instead of using getStringForLanguage
$string = $zMultiLingualString
  ->buildStringForLanguage($lang)
  ->placeholderWith('wikilambda-multilingualstring-nofallback') // or `placeholderNoFallback()`
  ->getString();

If you agree with this proposal, I can open another issue. Anyway, I will close this one once I get your answer.

Thanks for your response @Jdforrester-WMF! I don't see the overhead that would be implied when returning a ZMonoLingualString, but it's wise that I trust your analysis :-).

What about this proposal, which still can be implemented: https://phabricator.wikimedia.org/T313712#8101311

As a complement of the comment, here is an example for how we may lookup a string given a locale:

// instead of using getStringAndLanguageCode
$stringAndLanguage = $zMultiLingualString
   ->buildStringForLanguage($lang) // This returns some structure which implements the below methods (`fallbackWithEnglish()`, `placeholderWith()`, `getStringAndLanguageCode()`)
   ->fallbackWithEnglish()
   ->placeholderWith('wikilambda-editor-default-name') // we may otherwise add a `placeholderForTitle()` method
   ->getStringAndLanguageCode();

// No placeholder nor english fallback
$strictStringAndLanguage = $zMultiLingualString->buildStringForLanguage($lang)->getStringAndLanguageCode();

// instead of using getStringForLanguage
$string = $zMultiLingualString
  ->buildStringForLanguage($lang)
  ->placeholderWith('wikilambda-multilingualstring-nofallback') // or `placeholderNoFallback()`
  ->getString();

If you agree with this proposal, I can open another issue. Anyway, I will close this one once I get your answer.

Yeah, you've convinced me, let's do this. We can keep this task open.

Teleosteen renamed this task from ZMultiLingualString->internalGetStringForLanguage may return a ZMonoLingualString to ZMultiLingualString->internalGetStringForLanguage may return a structure to build the expected value (with or without fallback / placeholder).Jul 27 2022, 7:16 AM
Teleosteen claimed this task.
Teleosteen updated the task description. (Show Details)

Thanks for your feedback @Jdforrester-WMF, I edited the title and the description of the task for more clarity and assigned it to myself. I'll work on it during the week.

Thanks for your feedback @Jdforrester-WMF, I edited the title and the description of the task for more clarity and assigned it to myself. I'll work on it during the week.

That sounds great; thank you!

Change 822633 had a related patch set uploaded (by Teleosteen; author: Teleosteen):

[mediawiki/extensions/WikiLambda@master] Rework the way we get a localized string from a ZMultiLingualString

https://gerrit.wikimedia.org/r/822633

phpcs report these errors:

FILE: /var/www/html/w/extensions/WikiLambda/includes/ZObjects/ZMultiLingualString.php
----------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------
 199 | ERROR | Only one object structure is allowed in a file
     |       | (Generic.Files.OneObjectStructurePerFile.MultipleFound)
 219 | ERROR | Only one object structure is allowed in a file
     |       | (Generic.Files.OneObjectStructurePerFile.MultipleFound)
----------------------------------------------------------------------------------------------------------------------

The structures I declared are meant to be used only by ZMultiLingualString. I think to declare them as a kind of inner class (namespace Mediawiki\...\ZMultiLingualString).
In what file can I put these structure? (in includes/ZObjects/ZMultiLingualString/?)

Thanks in advance :)

The structures I declared are meant to be used only by ZMultiLingualString. I think to declare them as a kind of inner class (namespace Mediawiki\...\ZMultiLingualString).
In what file can I put these structure? (in includes/ZObjects/ZMultiLingualString/?)

Thanks in advance :)

You can't put multiple structures in the same file, per Wikimedia's coding standards. PHP doesn't support inner classes, sorry. :-( I'd just put them in their own files in the root of includes/ for now.

OK, I removed the interface and did as you suggested.

I still don't understand why Sonar does not recognized the coverage of lines of the newly introduced class: https://sonarcloud.io/component_measures?metric=new_coverage&selected=mediawiki-extensions-WikiLambda%3Aincludes%2FStringForLanguageBuilder.php&view=list&branch=822633-5&id=mediawiki-extensions-WikiLambda

And despite the tests that use the methods of StringForLanguageBuilder (with the @covers tags): https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikiLambda/+/822633/6/tests/phpunit/integration/ZMultiLingualStringTest.php

Any help would be highly appreciated! :)

Don't worry about Sonar too much; it gets confused easily.

Our coverage test spots the changes just fine:

00:02:22.467 +-------------------------------------------+--------+--------+
00:02:22.467 | Filename                                  | Old %  | New %  |
00:02:22.467 +-------------------------------------------+--------+--------+
00:02:22.467 | includes/StringForLanguageBuilder.php     | 0      |  77.00 |
00:02:22.467 | includes/ZObjectContent.php               |  77.00 |  78.00 |
00:02:22.467 | includes/ZObjects/ZMultiLingualString.php |  86.00 |  81.00 |
00:02:22.467 +-------------------------------------------+--------+--------+

I think this is good to land, so I shall. Thank you!

Change 822633 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Rework the way we get a localized string from a ZMultiLingualString

https://gerrit.wikimedia.org/r/822633