Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkp/pkp-lib#9707 use weblate locales for ui #10569

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

bozana
Copy link
Collaborator

@bozana bozana commented Oct 29, 2024

s. #9707

tests/classes/i18n/LocaleTest.php Show resolved Hide resolved
classes/i18n/Locale.php Show resolved Hide resolved
Comment on lines +384 to +385
$filterByLocales = array_keys(array_intersect_key($locales, array_flip($filterByLocales)));
$locales = array_intersect_key($locales, array_flip($filterByLocales));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is similar, so if we inverse the variables, the value can be reused :)

Suggested change
$filterByLocales = array_keys(array_intersect_key($locales, array_flip($filterByLocales)));
$locales = array_intersect_key($locales, array_flip($filterByLocales));
$locales = array_intersect_key($locales, array_flip($filterByLocales));
$filterByLocales = array_keys($locales);

classes/i18n/Locale.php Show resolved Hide resolved
{
return $this->weblateLocaleNames ??= (function (): array {
$file = Core::getBaseDir() . '/' . PKP_LIB_PATH . '/lib/weblateLanguages/languages.json';
$key = __METHOD__ . self::MAX_SUBMISSION_LOCALES_CACHE_LIFETIME . filemtime($file);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How likely is this file to change? The current value is 1 year.

Suggested change
$key = __METHOD__ . self::MAX_SUBMISSION_LOCALES_CACHE_LIFETIME . filemtime($file);
$key = __METHOD__ . static::MAX_WEBLATE_LOCALES_CACHE_LIFETIME. filemtime($file);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file comes with the installation/upgrade. I suppose then (at installation or upgrade) also the cache will be cleared?
I do not think the users will get it in between.
What would you then suggest the lifetime to be?

classes/i18n/LocaleMetadata.php Show resolved Hide resolved
classes/i18n/LocaleMetadata.php Show resolved Hide resolved
classes/i18n/LocaleMetadata.php Show resolved Hide resolved
classes/i18n/interfaces/LocaleInterface.php Show resolved Hide resolved
];

$schemaLocName = (DB::connection() instanceof PostgresConnection) ? 'TABLE_CATALOG' : 'TABLE_SCHEMA';
$renameLocale = fn (string $l) => collect(DB::select("SELECT DISTINCT TABLE_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE COLUMN_NAME = ? AND {$schemaLocName} = ?", [$l, DB::connection()->getDatabaseName()]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to use the Laravel syntax here (e.g. DB::table()), it will already return a collection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think it's possible to retrieve the metadata using the Laravel stuff (e.g. DB::connection()->getSchemaBuilder()->getTables()), just sounds more flexible than writing custom queries.

* This can be ambiguous if several locales are defined for the same language. In this case we'll use the primary locale to disambiguate.
* If that still doesn't determine a unique locale then we'll choose the first locale found.
*/
public static function getLocaleFrom3LetterIso(?string $iso3Letter): ?string
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is then deprecated from 3.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants