Skip to content

N°9553 - Fix on LoadLocalizedData in ModuleInstaller #926

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.

Sign up for GitHub

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

Open
v-dumas wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

N°9553 - Fix on LoadLocalizedData in ModuleInstaller #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9443d87
N°9553 - LoadLocalizedData: review feedbacks + improved tests
v-dumas Jun 1, 2026
a8b8e2d
N°9553 - take into account feedbacks from review
v-dumas Jun 1, 2026
b68df9e
N°9553 - fix typo
v-dumas Jun 1, 2026
03fd49f
N°9553 - Refactor methods to be clearer on what it does
v-dumas Jun 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
],
Expand Down Expand Up @@ -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" );
}
}
}
8 changes: 1 addition & 7 deletions datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
}
8 changes: 1 addition & 7 deletions datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
}
16 changes: 2 additions & 14 deletions datamodels/2.x/itop-structure/module.itop-structure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion datamodels/2.x/itop-tickets/module.itop-tickets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
105 changes: 51 additions & 54 deletions setup/moduleinstaller.class.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -321,74 +321,51 @@ 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);

// 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, $sFirstLoadingVersion, '<')
&& version_compare($sFirstLoadingVersion, $sCurrentVersion, '<='))) {

$sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern);
if ($sFileName !== '') {
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 (!self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) {
throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'");
if (utils::IsNullOrEmptyString($sPreviousVersion)) {
self::LoadLocalizedData($oConfiguration, $sFilePattern);
}

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");
}
}

private static function IsValidLocalizedDataVersion(string $sVersion): bool
{
return (preg_match('/^\d+\.\d+(?:\.\d+)?(?:-[A-Za-z0-9]+)?$/', $sVersion) === 1);
}

/**
* @param array|string $sFileName
* @param \XMLDataLoader $oDataLoader
* @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");
}
Expand All @@ -401,19 +378,39 @@ 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
* @throws \ConfigException
* @throws \CoreException
*/
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);
if (!file_exists($sFileName)) {
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);
}
Expand Down
Loading