From 9443d87172cb4d5e296f049ab0c2438adbafcbb3 Mon Sep 17 00:00:00 2001 From: v-dumas Date: Mon, 1 Jun 2026 12:34:18 +0200 Subject: [PATCH 1/4] =?UTF-8?q?N=C2=B09553=20-=20LoadLocalizedData:=20revi?= =?UTF-8?q?ew=20feedbacks=20+=20improved=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/moduleinstaller.class.inc.php | 44 ++----- .../setup/ModuleInstallerAPITest.php | 124 +++++++++++++----- 2 files changed, 107 insertions(+), 61 deletions(-) diff --git a/setup/moduleinstaller.class.inc.php b/setup/moduleinstaller.class.inc.php index aff6a1c1fb..0c143a06d0 100644 --- a/setup/moduleinstaller.class.inc.php +++ b/setup/moduleinstaller.class.inc.php @@ -311,7 +311,7 @@ public static function RenameTableInDB(string $sOrigTable, string $sDstTable) /** * @param \Config $oConfiguration - * @param string $sPreviousVersion The previous version of the module (empty string will force the loading) + * @param string $sPreviousVersion The previous version of the module (empty string in case of first install) * @param string $sCurrentVersion The current version of the module * @param string $sFirstLoadingVersion The first module version for which the data loading should be performed (e.g. '3.0.0') * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml') @@ -321,55 +321,39 @@ public static function RenameTableInDB(string $sOrigTable, string $sDstTable) * @throws \CoreException * @throws \CoreUnexpectedValue */ - public static function LoadLocalizedData(Config $oConfiguration, string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void + public static function LoadLocalizedData(Config $oConfiguration, ?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void { self::AssertLoadLocalizedDataParametersAreValid($sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sFilePattern); - // It's not very clear if it makes sense to test a particular version, - // as the loading mechanism checks object existence using reconc_keys - // and do not recreate them, nor update existing. - // Without test, new entries added to the data files, would be automatically loaded + // The loading is done only if + // - it's a first install of the module + // - or it's an upgrade of that module (PreviousVersion is less than the CurrentVersion), which means that we are really upgrading (and not reinstalling the same version or downgrading), and + // - either the FirstLoadingVersion is between the PreviousVersion and the CurrentVersion + // - or the FirstLoadingVersion is empty, forcing the loading on all upgrades, if (($sPreviousVersion === '') || - (version_compare($sPreviousVersion, $sCurrentVersion, '<') - && 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)) { - // Fresh install - $sDefaultLanguage = $oConfiguration->GetDefaultLanguage(); - } else { - // Upgrade - $sDefaultLanguage = utils::GetConfig(true)->GetDefaultLanguage(); - } + (version_compare($sPreviousVersion, $sCurrentVersion, '<') && + (($sFirstLoadingVersion === '') || version_compare($sPreviousVersion, $sFirstLoadingVersion, '<')))) { + $sDefaultLanguage = $oConfiguration->GetDefaultLanguage(); $sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern); - if ($sFileName !== '') { - self::XMLFileLoad($sFileName); - } + self::XMLFileLoad($sFileName); } } /** * @throws \CoreUnexpectedValue */ - private static function AssertLoadLocalizedDataParametersAreValid(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void + private static function AssertLoadLocalizedDataParametersAreValid(?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void { if (($sPreviousVersion !== '') && !self::IsValidLocalizedDataVersion($sPreviousVersion)) { throw new CoreUnexpectedValue("LoadLocalizedData expects sPreviousVersion to be empty or match x.y[.z][-name], got '{$sPreviousVersion}'"); } - if (!self::IsValidLocalizedDataVersion($sCurrentVersion)) { throw new CoreUnexpectedValue("LoadLocalizedData expects sCurrentVersion to match x.y[.z][-name], got '{$sCurrentVersion}'"); } - - if (!self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) { + if (($sFirstLoadingVersion !== '') && !self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) { throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'"); } - - if (utils::IsNullOrEmptyString($sFilePattern)) { - throw new CoreUnexpectedValue('LoadLocalizedData expects sFilePattern to be a non-empty string'); - } - if (substr_count($sFilePattern, '{{language_code}}') !== 1) { throw new CoreUnexpectedValue("LoadLocalizedData expects sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); } @@ -406,8 +390,6 @@ public static function XMLFileLoad(string $sFileName): void * @param string $sFilePattern The full path+name of the file to localize, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml') * * @return string The localized file name if found, or an empty string if not found - * @throws \ConfigException - * @throws \CoreException */ public static function GetLocalizedFileName($sLanguage, string $sFilePattern): string { diff --git a/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php b/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php index da4a9d2239..0e5c38f0fe 100644 --- a/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php +++ b/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php @@ -286,47 +286,87 @@ public function testMoveColumnInDB_SameTable(): void /** * @covers \ModuleInstallerAPI::LoadLocalizedData + * @dataProvider LoadLocalizedData_RequiredLanguageProvider */ - public function testLoadLocalizedData_LoadsOnFirstInstall(): void + public function testLoadLocalizedData_LoadRequiredLanguageOnFirstInstall(string $sRequiredLanguage, array $aAvailableLanguages, array $aExpectedCountByLanguage): void { // Given - [$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_FirstInstall_', 'fr_fr'); - $this->GivenLocalizedDataFile($sTmpDir, "en_us", $sOrgName); - $this->GivenLocalizedDataFile($sTmpDir, "fr_fr", $sOrgName); + [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_RequiredLanguage_', $sRequiredLanguage, $aAvailableLanguages); + // When no previous version, and current version higher than the first loading version ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.3.0', '3.0.0', $sPattern); + // Then data loaded - $this->AssertOrganizationCountByName($sOrgName, 'en_us', 0); - $this->AssertOrganizationCountByName($sOrgName, 'fr_fr', 1); + foreach ($aExpectedCountByLanguage as $sLanguage => $iExpectedCount) { + $this->AssertOrganizationCountByName($sOrgName, $sLanguage, $iExpectedCount); + } + } + + public function LoadLocalizedData_RequiredLanguageProvider(): array + { + return [ + 'Required fr_fr and file exists' => [ + 'required language' => 'fr_fr', + 'available languages' => ['en_us', 'fr_fr'], + 'expected counts' => ['en_us' => 0, 'fr_fr' => 1], + ], + 'Required en_us and file exists' => [ + 'required language' => 'en_us', + 'available languages' => ['en_us', 'fr_fr'], + 'expected counts' => ['en_us' => 1, 'fr_fr' => 0], + ], + 'Required fr_fr but fallback to en_us' => [ + 'required language' => 'fr_fr', + 'available languages' => ['en_us'], + 'expected counts' => ['en_us' => 1, 'fr_fr' => 0], + ], + 'Required de_de and file exists' => [ + 'required language' => 'de_de', + 'available languages' => ['en_us', 'fr_fr', 'de_de'], + 'expected counts' => ['en_us' => 0, 'fr_fr' => 0, 'de_de' => 1], + ], + 'Required de_de but fallback to en_us' => [ + 'required language' => 'de_de', + 'available languages' => ['en_us', 'fr_fr'], + 'expected counts' => ['en_us' => 1, 'fr_fr' => 0, 'de_de' => 0], + ], + ]; } /** * @covers \ModuleInstallerAPI::LoadLocalizedData + * @dataProvider LoadLocalizedData_VersionConditionNotMetProvider */ - public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(): void + public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion): void { // Given - [$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us'); - $this->GivenLocalizedDataFile($sTmpDir, "en_us", $sOrgName); - - // When a previous version that is lower than the first loading version, but higher or equal to the current version - ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.0.0', '3.1.0', '3.0.0', $sPattern); + [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us', ['en_us']); + // When version gate conditions are not met + ModuleInstallerAPI::LoadLocalizedData($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); // Then no data loaded $this->AssertOrganizationCountByName($sOrgName, 'en_us', 0); } + public function LoadLocalizedData_VersionConditionNotMetProvider(): array + { + return [ + 'Equal versions (reinstall)' => ['3.1.0', '3.1.0', '3.0.0'], + 'Downgrade attempt' => ['3.2.0', '3.1.0', '3.0.0'], + 'Upgrade but first loading version already passed' => ['3.1.0', '3.2.0', '3.0.0'], + 'Upgrade with boundary equality on first loading version' => ['3.0.0', '3.1.0', '3.0.0'], + ]; + } /** * @covers \ModuleInstallerAPI::LoadLocalizedData */ - public function testLoadLocalizedData_FallbacksToEnUsWhenLanguageFileIsMissing(): void + public function testLoadLocalizedData_AlwaysLoadWhenFistLoadingVersionEmpty(): void { - [$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_Fallback_', 'fr_fr'); - // Intentionally create ONLY en_us file - $this->GivenLocalizedDataFile($sTmpDir, 'en_us', $sOrgName); - // When loading localized data in fr_fr, but only en_us file exists - ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.3.0', '3.0.0', $sPattern); + // Given + [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us', ['en_us']); - $this->AssertOrganizationCountByName($sOrgName, 'fr_fr', 0); + // When a previous version that is lower than the first loading version, but higher or equal to the current version + ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.0.0', '3.1.0', '', $sPattern); + // Then no data loaded $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); } @@ -336,8 +376,7 @@ public function testLoadLocalizedData_FallbacksToEnUsWhenLanguageFileIsMissing() */ public function testLoadLocalizedData_AcceptsSupportedVersionFormats(string $sCurrentVersion, string $sFirstLoadingVersion): void { - [$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_ValidVersion_', 'en_us'); - $this->GivenLocalizedDataFile($sTmpDir, 'en_us', $sOrgName); + [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_ValidVersion_', 'en_us', ['en_us']); ModuleInstallerAPI::LoadLocalizedData($oConfig, '', $sCurrentVersion, $sFirstLoadingVersion, $sPattern); @@ -353,6 +392,34 @@ public function LoadLocalizedData_ValidVersionFormatsProvider(): array 'Current version x.y.z-1' => ['1.2.4-1', '1.0.3-2'], ]; } + // Test when a file is loaded twice because of the version conditions, it doesn't create duplicates (idempotent loading) + public function testLoadLocalizedData_IdempotentLoading(): void + { + // Given + [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_Idempotent_', 'en_us', ['en_us']); + + // When LoadLocalizedData is called twice with conditions that would load the file both times + ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.1.0', '3.0.0', $sPattern); + ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.1.0', '3.2.0', '', $sPattern); + + // Then no duplicate data loaded + $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); + } + + /** + * @covers \ModuleInstallerAPI::LoadLocalizedData + * @dataProvider LoadLocalizedData_InvalidParametersProvider + */ + public function testLoadLocalizedData_ThrowsOnInvalidParameters(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sPattern, string $sExpectedMessage): void + { + $oConfig = MetaModel::GetConfig(); + $this->assertNotNull($oConfig); + + $this->expectException(\CoreUnexpectedValue::class); + $this->expectExceptionMessage($sExpectedMessage); + + ModuleInstallerAPI::LoadLocalizedData($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); + } public function LoadLocalizedData_InvalidParametersProvider(): array { @@ -388,13 +455,6 @@ public function LoadLocalizedData_InvalidParametersProvider(): array 'pattern' => $sTmpDir.DIRECTORY_SEPARATOR.'data.{{LANGUAGE_CODE}}.xml', 'message' => "{{language_code}}", ], - 'Parent directory does not exist' => [ - 'previous' => '', - 'current' => '3.2.0', - 'first' => '3.0.0', - 'pattern' => $sTmpDir.DIRECTORY_SEPARATOR.'missing'.DIRECTORY_SEPARATOR.'data.{{language_code}}.xml', - 'message' => 'parent directory', - ], ]; } @@ -403,7 +463,7 @@ public function LoadLocalizedData_InvalidParametersProvider(): array * * @return array{0: Config, 1: string, 2: string, 3: string, 4: string} */ - private function GivenLocalizedDataTestContext(string $sOrgNamePrefix, string $sLanguage): array + private function GivenLocalizedDataTestContext(string $sOrgNamePrefix, string $sLanguage, array $aAvailableLanguages = []): array { $oConfig = MetaModel::GetConfig(); $oConfig->SetDefaultLanguage($sLanguage); @@ -415,7 +475,11 @@ private function GivenLocalizedDataTestContext(string $sOrgNamePrefix, string $s $this->aFileToClean[] = $sTmpDir; $sPattern = $sTmpDir.DIRECTORY_SEPARATOR.'data.{{language_code}}.xml'; - return [$oConfig, $sOrgName, $sTmpDir, $sPattern]; + foreach ($aAvailableLanguages as $sAvailableLanguage) { + $this->GivenLocalizedDataFile($sTmpDir, $sAvailableLanguage, $sOrgName); + } + + return [$oConfig, $sOrgName, $sPattern]; } private function GivenLocalizedDataFile(string $sDir, string $sLang, string $sOrgName): string From a8b8e2d81d95afca91db8f7c385da63c2dd19cd2 Mon Sep 17 00:00:00 2001 From: v-dumas Date: Mon, 1 Jun 2026 12:39:26 +0200 Subject: [PATCH 2/4] =?UTF-8?q?N=C2=B09553=20-=20take=20into=20account=20f?= =?UTF-8?q?eedbacks=20from=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/moduleinstaller.class.inc.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setup/moduleinstaller.class.inc.php b/setup/moduleinstaller.class.inc.php index 0c143a06d0..4761faf625 100644 --- a/setup/moduleinstaller.class.inc.php +++ b/setup/moduleinstaller.class.inc.php @@ -355,7 +355,7 @@ private static function AssertLoadLocalizedDataParametersAreValid(?string $sPrev throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'"); } if (substr_count($sFilePattern, '{{language_code}}') !== 1) { - throw new CoreUnexpectedValue("LoadLocalizedData expects sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); + throw new CoreUnexpectedValue("LoadLocalizedData expects $sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); } } @@ -366,7 +366,6 @@ private static function IsValidLocalizedDataVersion(string $sVersion): bool /** * @param array|string $sFileName - * @param \XMLDataLoader $oDataLoader * * @return void * @throws \Exception @@ -396,6 +395,7 @@ public static function GetLocalizedFileName($sLanguage, string $sFilePattern): s $sLang = str_replace(' ', '_', strtolower($sLanguage)); $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); if (!file_exists($sFileName)) { + etupLog::Debug("No data file found matching the pattern $sFilePattern and language_code $sLang. Trying with 'en_us' as fallback."); $sLang = 'en_us'; $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); } From b68df9ec076d7b417533f346d5f8322cf154eb89 Mon Sep 17 00:00:00 2001 From: v-dumas Date: Mon, 1 Jun 2026 12:47:24 +0200 Subject: [PATCH 3/4] =?UTF-8?q?N=C2=B09553=20-=20fix=20typo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/moduleinstaller.class.inc.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup/moduleinstaller.class.inc.php b/setup/moduleinstaller.class.inc.php index 4761faf625..773525896f 100644 --- a/setup/moduleinstaller.class.inc.php +++ b/setup/moduleinstaller.class.inc.php @@ -395,7 +395,7 @@ public static function GetLocalizedFileName($sLanguage, string $sFilePattern): s $sLang = str_replace(' ', '_', strtolower($sLanguage)); $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); if (!file_exists($sFileName)) { - etupLog::Debug("No data file found matching the pattern $sFilePattern and language_code $sLang. Trying with 'en_us' as fallback."); + SetupLog::Debug("No data file found matching the pattern $sFilePattern and language_code $sLang. Trying with 'en_us' as fallback."); $sLang = 'en_us'; $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); } From 03fd49f68899507a69dc34bd17aef5eecd7b6785 Mon Sep 17 00:00:00 2001 From: v-dumas Date: Tue, 2 Jun 2026 17:36:01 +0200 Subject: [PATCH 4/4] =?UTF-8?q?N=C2=B09553=20-=20Refactor=20methods=20to?= =?UTF-8?q?=20be=20clearer=20on=20what=20it=20does?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../module.itop-config-mgmt.php | 3 +- .../module.itop-service-mgmt-provider.php | 8 +- .../module.itop-service-mgmt.php | 8 +- .../itop-structure/module.itop-structure.php | 16 +--- .../2.x/itop-tickets/module.itop-tickets.php | 2 +- setup/moduleinstaller.class.inc.php | 75 +++++++++++-------- .../setup/ModuleInstallerAPITest.php | 39 ++++------ 7 files changed, 66 insertions(+), 85 deletions(-) diff --git a/datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php b/datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php index 12bea24bd1..972a5c85b8 100755 --- a/datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php +++ b/datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php @@ -26,7 +26,6 @@ ], 'data.struct' => [ 'data/en_us.data.itop-brand.xml', - 'data/en_us.data.itop-networkdevicetype.xml', 'data/en_us.data.itop-osfamily.xml', 'data/en_us.data.itop-osversion.xml', ], @@ -102,6 +101,8 @@ public static function BeforeDatabaseCreation(Config $oConfiguration, $sPrevious */ public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion) { + // load localized data for NetworkDeviceType + static::LoadLocalizedDataOnCrossingVersion($oConfiguration, $sPreviousVersion, $sCurrentVersion,'3.3.0',__DIR__."/data/{{language_code}}.data.itop-networkdevicetype.xml" ); } } } diff --git a/datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php b/datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php index 6ffa45a731..0ef49ab1b0 100755 --- a/datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php +++ b/datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php @@ -88,13 +88,7 @@ public static function BeforeDatabaseCreation(Config $oConfiguration, $sPrevious public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion) { // Load localized structural data: contract types - static::LoadLocalizedData( - $oConfiguration, - $sPreviousVersion, - $sCurrentVersion, - '3.3.0', - __DIR__."/data/{{language_code}}.data.itop-contracttype.xml" - ); + static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contracttype.xml"); } } } diff --git a/datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php b/datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php index ecb3b7d8c0..ef115bb965 100755 --- a/datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php +++ b/datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php @@ -85,13 +85,7 @@ public static function BeforeDatabaseCreation(Config $oConfiguration, $sPrevious public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion) { // Load localized structural data: contact types and document types - static::LoadLocalizedData( - $oConfiguration, - $sPreviousVersion, - $sCurrentVersion, - '3.3.0', - __DIR__."/data/{{language_code}}.data.itop-contracttype.xml" - ); + static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contracttype.xml"); } } } diff --git a/datamodels/2.x/itop-structure/module.itop-structure.php b/datamodels/2.x/itop-structure/module.itop-structure.php index 775d383db5..53884b2b2e 100644 --- a/datamodels/2.x/itop-structure/module.itop-structure.php +++ b/datamodels/2.x/itop-structure/module.itop-structure.php @@ -100,20 +100,8 @@ public static function BeforeDatabaseCreation(Config $oConfiguration, $sPrevious public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion) { // Load localized structural data: contact types and document types - static::LoadLocalizedData( - $oConfiguration, - $sPreviousVersion, - $sCurrentVersion, - '3.3.0', - __DIR__."/data/{{language_code}}.data.itop-contacttype.xml" - ); - static::LoadLocalizedData( - $oConfiguration, - $sPreviousVersion, - $sCurrentVersion, - '3.3.0', - __DIR__."/data/{{language_code}}.data.itop-documenttype.xml" - ); + static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contacttype.xml"); + static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-documenttype.xml"); // Default language will be used for actions // Note: There is a issue when upgrading, default language cannot be retrieved from the passed configuration, we have to read it from the disk diff --git a/datamodels/2.x/itop-tickets/module.itop-tickets.php b/datamodels/2.x/itop-tickets/module.itop-tickets.php index 17e8e8f241..b4e018b775 100755 --- a/datamodels/2.x/itop-tickets/module.itop-tickets.php +++ b/datamodels/2.x/itop-tickets/module.itop-tickets.php @@ -61,6 +61,6 @@ public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousV } } // Load localized structural data: predefined query phrases for notifications - static::LoadLocalizedData($oConfiguration, $sPreviousVersion, $sCurrentVersion, '3.0.0', __DIR__."/data/{{language_code}}.data.itop-tickets.xml"); + static::LoadLocalizedDataOnCrossingVersion($oConfiguration, $sPreviousVersion, $sCurrentVersion, '3.0.0', __DIR__."/data/{{language_code}}.data.itop-tickets.xml"); } } diff --git a/setup/moduleinstaller.class.inc.php b/setup/moduleinstaller.class.inc.php index 773525896f..fb2b33630a 100644 --- a/setup/moduleinstaller.class.inc.php +++ b/setup/moduleinstaller.class.inc.php @@ -321,9 +321,9 @@ public static function RenameTableInDB(string $sOrigTable, string $sDstTable) * @throws \CoreException * @throws \CoreUnexpectedValue */ - public static function LoadLocalizedData(Config $oConfiguration, ?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void + public static function LoadLocalizedDataOnCrossingVersion(Config $oConfiguration, ?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void { - self::AssertLoadLocalizedDataParametersAreValid($sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sFilePattern); + self::AssertLoadLocalizedDataParametersAreValid($sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion); // The loading is done only if // - it's a first install of the module @@ -331,47 +331,41 @@ public static function LoadLocalizedData(Config $oConfiguration, ?string $sPrevi // - either the FirstLoadingVersion is between the PreviousVersion and the CurrentVersion // - or the FirstLoadingVersion is empty, forcing the loading on all upgrades, if (($sPreviousVersion === '') || - (version_compare($sPreviousVersion, $sCurrentVersion, '<') && - (($sFirstLoadingVersion === '') || version_compare($sPreviousVersion, $sFirstLoadingVersion, '<')))) { + (version_compare($sPreviousVersion, $sCurrentVersion, '<') + && version_compare($sPreviousVersion, $sFirstLoadingVersion, '<') + && version_compare($sFirstLoadingVersion, $sCurrentVersion, '<='))) { - $sDefaultLanguage = $oConfiguration->GetDefaultLanguage(); - $sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern); - self::XMLFileLoad($sFileName); + self::LoadLocalizedData($oConfiguration, $sFilePattern); } } - /** - * @throws \CoreUnexpectedValue - */ - private static function AssertLoadLocalizedDataParametersAreValid(?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void + * @param \Config $oConfiguration + * @param string $sPreviousVersion The previous version of the module (empty string in case of first install) + * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml') + * + * @return void + */ + public static function LoadLocalizedDataOnNewInstall(Config $oConfiguration, ?string $sPreviousVersion, string $sFilePattern): void { - if (($sPreviousVersion !== '') && !self::IsValidLocalizedDataVersion($sPreviousVersion)) { - throw new CoreUnexpectedValue("LoadLocalizedData expects sPreviousVersion to be empty or match x.y[.z][-name], got '{$sPreviousVersion}'"); - } - if (!self::IsValidLocalizedDataVersion($sCurrentVersion)) { - throw new CoreUnexpectedValue("LoadLocalizedData expects sCurrentVersion to match x.y[.z][-name], got '{$sCurrentVersion}'"); - } - if (($sFirstLoadingVersion !== '') && !self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) { - throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'"); - } - if (substr_count($sFilePattern, '{{language_code}}') !== 1) { - throw new CoreUnexpectedValue("LoadLocalizedData expects $sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); + if (utils::IsNullOrEmptyString($sPreviousVersion)) { + self::LoadLocalizedData($oConfiguration, $sFilePattern); } } - private static function IsValidLocalizedDataVersion(string $sVersion): bool - { - return (preg_match('/^\d+\.\d+(?:\.\d+)?(?:-[A-Za-z0-9]+)?$/', $sVersion) === 1); - } - /** - * @param array|string $sFileName + * @param \Config $oConfiguration + * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml') * * @return void * @throws \Exception */ - public static function XMLFileLoad(string $sFileName): void + protected static function LoadLocalizedData(Config $oConfiguration, string $sFilePattern): void { + if (substr_count($sFilePattern, '{{language_code}}') !== 1) { + throw new CoreUnexpectedValue("LoadLocalizedData expects $sFilePattern to contain the exact placeholder '{{language_code}}' exactly once"); + } + $sDefaultLanguage = $oConfiguration->GetDefaultLanguage(); + $sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern); if (!file_exists($sFileName)) { throw new Exception("File $sFileName not found"); } @@ -384,13 +378,34 @@ public static function XMLFileLoad(string $sFileName): void $oDataLoader->EndSession(); } + /** + * @throws \CoreUnexpectedValue + */ + private static function AssertLoadLocalizedDataParametersAreValid(?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion): void + { + if (($sPreviousVersion !== '') && !self::IsValidLocalizedDataVersion($sPreviousVersion)) { + throw new CoreUnexpectedValue("LoadLocalizedData expects sPreviousVersion to be empty or match x.y[.z][-name], got '{$sPreviousVersion}'"); + } + if (!self::IsValidLocalizedDataVersion($sCurrentVersion)) { + throw new CoreUnexpectedValue("LoadLocalizedData expects sCurrentVersion to match x.y[.z][-name], got '{$sCurrentVersion}'"); + } + if (($sFirstLoadingVersion !== '') && !self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) { + throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'"); + } + } + + private static function IsValidLocalizedDataVersion(string $sVersion): bool + { + return (preg_match('/^\d+\.\d+(?:\.\d+)?(?:-[A-Za-z0-9]+)?$/', $sVersion) === 1); + } + /** * @param string $sLanguage The language code to use for localization (e.g. 'EN US') * @param string $sFilePattern The full path+name of the file to localize, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml') * * @return string The localized file name if found, or an empty string if not found */ - public static function GetLocalizedFileName($sLanguage, string $sFilePattern): string + private static function GetLocalizedFileName($sLanguage, string $sFilePattern): string { $sLang = str_replace(' ', '_', strtolower($sLanguage)); $sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern); diff --git a/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php b/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php index 0e5c38f0fe..da890b36c9 100644 --- a/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php +++ b/tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php @@ -285,7 +285,7 @@ public function testMoveColumnInDB_SameTable(): void } /** - * @covers \ModuleInstallerAPI::LoadLocalizedData + * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion * @dataProvider LoadLocalizedData_RequiredLanguageProvider */ public function testLoadLocalizedData_LoadRequiredLanguageOnFirstInstall(string $sRequiredLanguage, array $aAvailableLanguages, array $aExpectedCountByLanguage): void @@ -294,7 +294,7 @@ public function testLoadLocalizedData_LoadRequiredLanguageOnFirstInstall(string [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_RequiredLanguage_', $sRequiredLanguage, $aAvailableLanguages); // When no previous version, and current version higher than the first loading version - ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.3.0', '3.0.0', $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', '3.3.0', '3.0.0', $sPattern); // Then data loaded foreach ($aExpectedCountByLanguage as $sLanguage => $iExpectedCount) { @@ -334,7 +334,7 @@ public function LoadLocalizedData_RequiredLanguageProvider(): array } /** - * @covers \ModuleInstallerAPI::LoadLocalizedData + * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion * @dataProvider LoadLocalizedData_VersionConditionNotMetProvider */ public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion): void @@ -342,7 +342,7 @@ public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(st // Given [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us', ['en_us']); // When version gate conditions are not met - ModuleInstallerAPI::LoadLocalizedData($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); // Then no data loaded $this->AssertOrganizationCountByName($sOrgName, 'en_us', 0); } @@ -354,31 +354,20 @@ public function LoadLocalizedData_VersionConditionNotMetProvider(): array 'Downgrade attempt' => ['3.2.0', '3.1.0', '3.0.0'], 'Upgrade but first loading version already passed' => ['3.1.0', '3.2.0', '3.0.0'], 'Upgrade with boundary equality on first loading version' => ['3.0.0', '3.1.0', '3.0.0'], - ]; - } - /** - * @covers \ModuleInstallerAPI::LoadLocalizedData - */ - public function testLoadLocalizedData_AlwaysLoadWhenFistLoadingVersionEmpty(): void - { - // Given - [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us', ['en_us']); + 'Upgrade but first loading version empty' => ['3.1.0', '3.2.0', ''], - // When a previous version that is lower than the first loading version, but higher or equal to the current version - ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.0.0', '3.1.0', '', $sPattern); - // Then no data loaded - $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); + ]; } /** - * @covers \ModuleInstallerAPI::LoadLocalizedData + * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion * @dataProvider LoadLocalizedData_ValidVersionFormatsProvider */ public function testLoadLocalizedData_AcceptsSupportedVersionFormats(string $sCurrentVersion, string $sFirstLoadingVersion): void { [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_ValidVersion_', 'en_us', ['en_us']); - ModuleInstallerAPI::LoadLocalizedData($oConfig, '', $sCurrentVersion, $sFirstLoadingVersion, $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', $sCurrentVersion, $sFirstLoadingVersion, $sPattern); $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); } @@ -387,8 +376,8 @@ public function LoadLocalizedData_ValidVersionFormatsProvider(): array { return [ 'Current version with suffix' => ['3.2-dev', '3.0.0'], - 'Current version x.y.z' => ['1.2.4', '1.0'], - 'Current version x.y.z-suffix' => ['2.3.3-beta', '2.0.0'], + 'Current version x.y.z' => ['10.12.140-Tagada34', '1.0'], + 'Current version x.y.z-suffix' => ['2.3.3-beta', '2.3.3-alpha'], 'Current version x.y.z-1' => ['1.2.4-1', '1.0.3-2'], ]; } @@ -399,15 +388,15 @@ public function testLoadLocalizedData_IdempotentLoading(): void [$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_Idempotent_', 'en_us', ['en_us']); // When LoadLocalizedData is called twice with conditions that would load the file both times - ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.1.0', '3.0.0', $sPattern); - ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.1.0', '3.2.0', '', $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', '3.1.0', '3.0.0', $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '3.1.0', '3.2.0', '', $sPattern); // Then no duplicate data loaded $this->AssertOrganizationCountByName($sOrgName, 'en_us', 1); } /** - * @covers \ModuleInstallerAPI::LoadLocalizedData + * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion * @dataProvider LoadLocalizedData_InvalidParametersProvider */ public function testLoadLocalizedData_ThrowsOnInvalidParameters(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sPattern, string $sExpectedMessage): void @@ -418,7 +407,7 @@ public function testLoadLocalizedData_ThrowsOnInvalidParameters(string $sPreviou $this->expectException(\CoreUnexpectedValue::class); $this->expectExceptionMessage($sExpectedMessage); - ModuleInstallerAPI::LoadLocalizedData($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); + ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern); } public function LoadLocalizedData_InvalidParametersProvider(): array