pkp/pkp-lib#10729 fix I9707_WeblateUILocales, consider locales in arr… #10734
Conversation
| protected string $CONTEXT_TABLE = ''; | ||
| protected string $CONTEXT_SETTINGS_TABLE = ''; | ||
| protected string $CONTEXT_COLUMN = ''; |
There was a problem hiding this comment.
I think the pattern at the other migration of this PR is better (i.e. you're forced to provide the values):
| protected string $CONTEXT_TABLE = ''; | |
| protected string $CONTEXT_SETTINGS_TABLE = ''; | |
| protected string $CONTEXT_COLUMN = ''; | |
| abstract protected function getContextTable(): string; | |
| abstract protected function getContextSettingsTable(): string; | |
| abstract protected function getContextIdColumn(): string; |
| DB::select( | ||
| " | ||
| SELECT DISTINCT TABLE_NAME | ||
| FROM INFORMATION_SCHEMA.COLUMNS | ||
| WHERE COLUMN_NAME = ? AND {$schemaLocName} = ?", |
There was a problem hiding this comment.
Given the database has few tables/columns to loop through, then the Schema::getAllTables() (or the DB::getDoctrineSchemaManager()->listXXX()) together with the Schema::getColumns() will provide a better compatibility and simplify the code.
It's also possible to cache the return of getAllTables()/getColumns() to have less database requests.
| $affectedLocales = $this->getAffectedLocales(); | ||
| if (!in_array($localeValue, array_keys($affectedLocales))) { | ||
| return null; | ||
| } | ||
| return $affectedLocales[$localeValue]; |
There was a problem hiding this comment.
| $affectedLocales = $this->getAffectedLocales(); | |
| if (!in_array($localeValue, array_keys($affectedLocales))) { | |
| return null; | |
| } | |
| return $affectedLocales[$localeValue]; | |
| return $this->getAffectedLocales()[$localeValue] ?? null; |
Or a safer option with fallback (requires reviewing if it makes sense on the call sites)
| $affectedLocales = $this->getAffectedLocales(); | |
| if (!in_array($localeValue, array_keys($affectedLocales))) { | |
| return null; | |
| } | |
| return $affectedLocales[$localeValue]; | |
| return $this->getAffectedLocales()[$localeValue] ?? $localeValue; |
| $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()])) |
There was a problem hiding this comment.
Given the database has few tables/columns to loop through, then the Schema::getAllTables() (or the DB::getDoctrineSchemaManager()->listXXX()) together with the Schema::getColumns() will provide a better compatibility and simplify the code.
It's also possible to cache the return of getAllTables()/getColumns() to have less database requests.
| $this->updateArrayLocale($site->installed_locales, 'site', 'installed_locales'); | ||
|
|
||
| // users locales | ||
| $migration = $this; |
There was a problem hiding this comment.
I think this isn't needed, you can just use $this inside the function.
| foreach ($affectedLocales as $uiLocale => $weblateLocale) { | ||
| DB::table($sc->TABLE_NAME ?? $sc->table_name)->where($l, '=', $uiLocale)->update([$l => $weblateLocale]); | ||
| } |
There was a problem hiding this comment.
This loop can be turned into a single query:
...->whereIn($l, $uiLocales)->update([$l => DB::raw("CASE {$l} WHEN X THEN A WHEN Y THEN B END")]);.
| DB::table('users')->chunkById(1000, function ($users) use ($migration) { | ||
| foreach ($users as $user) { | ||
| $migration->updateArrayLocale($user->locales, 'users', 'locales', null, 'user_id', $user->user_id); | ||
| } | ||
| }, 'user_id'); |
There was a problem hiding this comment.
As these "array" columns are using JSON, then the updateArrayLocale() method could be updated to issue a single query, basically a sequence of self-enclosed replaces (e.g. locales = REPLACE(REPLACE(locales, '"X"', "A"), '"Y"', "B")).
Some installations have thousands of users, due to spammers, so anything that avoids looping through the users is worth in my opinion.
| ->first(); | ||
|
|
||
| if (isset($blockLocalizedContent)) { | ||
| $this->updateArrayKeysLocaleSetting($blockLocalizedContent->setting_value, 'plugin_settings', 'plugin_setting_id', $blockLocalizedContent->plugin_setting_id); |
There was a problem hiding this comment.
I reviewed fast, but it seems to me that this method is doing something similar to the updateArrayLocale().
…ays and setting values
s. #10729