N°9553 upload localised structural data#917
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile |
| $sLang = str_replace(' ', '_', strtolower($sLanguage)); | ||
| $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); | ||
| if (!file_exists($sFileName)) { | ||
| $sLang = 'en_us'; |
There was a problem hiding this comment.
you could add a SetupLog::Debug log to indicate that current default langage file is missing. as a fallback we try us_en.
| throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'"); | ||
| } | ||
|
|
||
| if (utils::IsNullOrEmptyString($sFilePattern)) { |
There was a problem hiding this comment.
this check is useless. next one is covering it as well. if file is empty no pattern {{xxx}} will be found...
| } | ||
|
|
||
| if (substr_count($sFilePattern, '{{language_code}}') !== 1) { | ||
| throw new CoreUnexpectedValue("LoadLocalizedData expects sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); |
There was a problem hiding this comment.
please add $sFilePattern in Exception message. or at least in its context to ease support
| } | ||
| } | ||
|
|
||
| private static function IsValidLocalizedDataVersion(string $sVersion): bool |
There was a problem hiding this comment.
no test covering this method. it could be nice to document what is ok. what is not.
| /** | ||
| * @throws \CoreUnexpectedValue | ||
| */ | ||
| private static function AssertLoadLocalizedDataParametersAreValid(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void |
There was a problem hiding this comment.
no test covering this method
| $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); | ||
| } | ||
|
|
||
| public function LoadLocalizedData_ValidVersionFormatsProvider(): array |
There was a problem hiding this comment.
use this provider to test IsValidLocalizedDataVersion. and keep only one current version to cover LoadLocalizedData
| * @throws \ConfigException | ||
| * @throws \CoreException |
There was a problem hiding this comment.
not sure these exceptions are thrown
|
|
||
| /** | ||
| * @param array|string $sFileName | ||
| * @param \XMLDataLoader $oDataLoader |
There was a problem hiding this comment.
i dont see this parameter anywhere
| if (utils::IsNullOrEmptyString($sPreviousVersion)) { | ||
| // Fresh install | ||
| $sDefaultLanguage = $oConfiguration->GetDefaultLanguage(); | ||
| } else { | ||
| // Upgrade | ||
| $sDefaultLanguage = utils::GetConfig(true)->GetDefaultLanguage(); |
There was a problem hiding this comment.
any idea why we use directly provided $oConfiguration when fresh install. and we use utils::GetConfig(true) otherwise? it does not seem to make sense to me.
| && version_compare($sPreviousVersion, $sFirstLoadingVersion, '<'))) { | ||
|
|
||
| // Note: There is an issue when upgrading, default language cannot be retrieved from the passed configuration, we have to read it from the disk | ||
| if (utils::IsNullOrEmptyString($sPreviousVersion)) { |
There was a problem hiding this comment.
($sPreviousVersion === '') is enough. as $sPreviousVersion cannot be null (in method signature it would be ?string otherwise)
odain-cbd
left a comment
odain-cbd
left a comment
There was a problem hiding this comment.
hope it is not too late for this review
4 participants
Base information
Symptom (bug) / Objective (enhancement)
Add localized structural data for
Proposed solution (bug and enhancement)
Define an helper to load the best structural data file based on the iTop default language
Checklist before requesting a review