N°9553 - Fix on LoadLocalizedData in ModuleInstaller by v-dumas · Pull Request #926 · Combodo/iTop

v-dumas · 2026-06-01T12:46:23Z

Base information

Question	Answer
Related to a Combodo ticket
Type of change?	Bug fix / Enhancement / Translations

Proposed solution (bug and enhancement)

Improve code and tests

Checklist before requesting a review

I have performed a self-review of my code
I have tested all changes I made on an iTop instance
I have added a unit test, otherwise I have explained why I couldn't
Is the PR clear and detailed enough so anyone can understand without digging in the code?

greptile-apps · 2026-06-01T12:47:55Z

Greptile Summary

This PR refactors the ModuleInstallerAPI localized-data loading mechanism, replacing the {{language_code}} placeholder pattern with an explicit .en_us.xml-suffix convention. The LoadLocalizedData method is split into LoadLocalizedDataOnCrossingVersion (version-gated public entry point), a protected LoadLocalizedData (file load), and GetLocalizedFileName (language resolution with en_us fallback). RunTimeEnvironment::LoadData is updated to call GetLocalizedFileName so newly-installed modules also resolve localized files. The itop-tickets data files are renamed to match the new convention, and two non-localizable sample data XML files are removed.

Renamed data files (en_us.data.itop-tickets.xml → data.itop-tickets.en_us.xml etc.) and updated module references to match the new naming convention.
IsVersionCrossed extracted as a private helper with an added sFirstLoadingVersion ≤ sCurrentVersion guard, and the test suite is expanded with parameterised providers for version-crossing scenarios and language fallback.

Confidence Score: 5/5

Safe to merge; the core logic is correct and well-tested, with only minor documentation inaccuracies to clean up

The refactoring correctly handles the language-resolution path, the en_us fallback, version-crossing logic, and idempotent loading. The only findings are a wrong @param type in LoadLocalizedData's docblock and a misleading ?string hint on the public API — neither affects current callers or runtime behaviour.

setup/moduleinstaller.class.inc.php — the stale docstrings and nullable type hints are worth a quick pass before merge

Important Files Changed

Filename	Overview
setup/moduleinstaller.class.inc.php	Core refactor: replaces LoadLocalizedData with LoadLocalizedDataOnCrossingVersion + LoadLocalizedData + GetLocalizedFileName; naming convention changes from {{language_code}} pattern to .en_us.xml suffix-based; one stale @param doc and one misleading ?string type hint noted
setup/runtimeenv.class.inc.php	Adds ModuleInstallerAPI::GetLocalizedFileName call to the LoadData file loop so newly-installed modules resolve a localized data file based on the configured default language
datamodels/2.x/itop-tickets/module.itop-tickets.php	Updates AfterDatabaseCreation to call the new LoadLocalizedDataOnCrossingVersion with the absolute path to the en_us file instead of the old {{language_code}} pattern
tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php	Tests expanded with data providers covering language fallback, version-crossing logic, and idempotent loading; stale language-pattern tests removed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["LoadLocalizedDataOnCrossingVersion"] --> B[AssertLoadLocalizedDataParametersAreValid]
    B -->|invalid params| E[throw CoreUnexpectedValue]
    B --> C{IsVersionCrossed?}
    C -->|sPreviousVersion == ''| F[first install: true]
    C -->|prev lt curr AND prev lt first AND first lte curr| G[upgrade crossing threshold: true]
    C -->|otherwise| H[false: skip load]
    F --> I["LoadLocalizedData(sWishedLanguage, sDefaultFile)"]
    G --> I
    I --> J["GetLocalizedFileName(sLanguage, sOriginalFile)"]
    J -->|ends with .en_us.xml| K{localized file exists?}
    K -->|yes| L[use localized file]
    K -->|no| M[fall back to en_us file]
    J -->|other extension| N[return file as-is]
    L --> O[XMLDataLoader.LoadFile]
    M --> O
    N --> O

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["LoadLocalizedDataOnCrossingVersion"] --> B[AssertLoadLocalizedDataParametersAreValid]
    B -->|invalid params| E[throw CoreUnexpectedValue]
    B --> C{IsVersionCrossed?}
    C -->|sPreviousVersion == ''| F[first install: true]
    C -->|prev lt curr AND prev lt first AND first lte curr| G[upgrade crossing threshold: true]
    C -->|otherwise| H[false: skip load]
    F --> I["LoadLocalizedData(sWishedLanguage, sDefaultFile)"]
    G --> I
    I --> J["GetLocalizedFileName(sLanguage, sOriginalFile)"]
    J -->|ends with .en_us.xml| K{localized file exists?}
    K -->|yes| L[use localized file]
    K -->|no| M[fall back to en_us file]
    J -->|other extension| N[return file as-is]
    L --> O[XMLDataLoader.LoadFile]
    M --> O
    N --> O

_{Reviews (2): Last reviewed commit: "N°9553 - Finalize localized sample and s..." | Re-trigger Greptile}

Molkobain · 2026-06-23T17:06:50Z

@v-dumas I see that Olivier is set as "assignee" to the PR, did you mean "reviewer"?
Only 1 assignee should be assigned to ensure there is an explicit owner :)

v-dumas · 2026-06-24T07:09:02Z

@Molkobain sorry, I fixed my error, you're right he is reviewer.

v-dumas · 2026-07-07T16:15:46Z

@greptile

Copilot

Pull request overview

This PR fixes and hardens the localized structural-data loading mechanism used during module install/upgrade (ModuleInstallerAPI). The previous implementation relied on a {{language_code}} placeholder in the file pattern and a version guard that could load data too early during upgrades. The refactor switches to a .en_us.xml-suffix naming convention with a dedicated GetLocalizedFileName resolver, splits responsibilities into IsVersionCrossed + LoadLocalizedDataOnCrossingVersion, and adds a first <= current clause so data is only loaded when the "first loading version" is actually crossed by the upgrade.

Changes:

Refactor LoadLocalizedData into LoadLocalizedDataOnCrossingVersion (version-gate) + LoadLocalizedData (file load) + GetLocalizedFileName (suffix-based localization with en_us fallback), and correct the version-crossing condition.
Migrate itop-tickets notification query data to localized data.itop-tickets.<lang>.xml files (en_us/fr_fr/de_de) and update the module handler; also apply localization in runtimeenv LoadData.
Remove sample contacttype/networkdevicetype XML files (already provided by structural data) and rewrite the unit tests with data providers.

Reviewed changes

Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
setup/moduleinstaller.class.inc.php	Core refactor of localized data loading and version-crossing logic; docblocks partly stale
setup/runtimeenv.class.inc.php	Resolves localized file names in `LoadData`; clarified log message
datamodels/2.x/itop-tickets/module.itop-tickets.php	Calls new `LoadLocalizedDataOnCrossingVersion` with `.en_us.xml` base path
datamodels/2.x/itop-tickets/data/data.itop-tickets.en_us.xml	New English notification query data (base/fallback file)
datamodels/2.x/itop-tickets/data/data.itop-tickets.fr_fr.xml	New French localized notification query data
datamodels/2.x/itop-tickets/data/data.itop-tickets.de_de.xml	New German localized notification query data
datamodels/2.x/itop-structure/module.itop-structure.php	Drops sample `contacttype` file reference
datamodels/2.x/itop-structure/data/data.sample.contacttype.xml	Deleted (duplicated by structural contact-type data)
datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php	Drops sample `networkdevicetype` file reference
datamodels/2.x/itop-config-mgmt/data/data.sample.networkdevicetype.xml	Deleted (duplicated by structural network-device-type data)
tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php	Rewrites tests with data providers covering new methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 24 out of 27 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 47 out of 48 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 47 out of 48 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 47 out of 48 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 24 out of 27 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 24 out of 27 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 24 out of 27 changed files in this pull request and generated 9 comments.

@@ -0,0 +1,101 @@
+<?xml version="1.0" encoding="UTF-8"?>


@@ -0,0 +1,387 @@
+<?xml version="1.0" encoding="UTF-8"?>


@@ -0,0 +1,219 @@
+<?xml version="1.0" encoding="UTF-8"?>


@@ -0,0 +1,101 @@
+<?xml version="1.0" encoding="UTF-8"?>


@@ -0,0 +1,387 @@
+<?xml version="1.0" encoding="UTF-8"?>


+		if (($sFirstLoadingVersion !== '') && !self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) {
 			throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'");
 		}


 	 * @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): void


@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="UTF-8"?>


CombodoApplicationsAccount added the internal Work made by Combodo label Jun 1, 2026

CombodoApplicationsAccount assigned v-dumas Jun 1, 2026

greptile-apps Bot reviewed Jun 1, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php Outdated

Comment thread setup/moduleinstaller.class.inc.php Outdated

v-dumas assigned odain-cbd Jun 1, 2026

v-dumas requested a review from odain-cbd June 24, 2026 07:07

v-dumas unassigned odain-cbd Jun 24, 2026

v-dumas force-pushed the feature/9553-ModuleInstaller-Feedbacks branch from b7b3f94 to cfc019e Compare July 7, 2026 16:14

Copilot AI review requested due to automatic review settings July 7, 2026 16:14

Copilot started reviewing on behalf of v-dumas July 7, 2026 16:14 View session

Copilot AI reviewed Jul 7, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php Outdated

Comment thread setup/moduleinstaller.class.inc.php Outdated

Comment thread tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php Outdated

Copilot AI review requested due to automatic review settings July 8, 2026 07:43

Copilot started reviewing on behalf of v-dumas July 8, 2026 07:43 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/runtimeenv.class.inc.php

Comment thread setup/moduleinstaller.class.inc.php Outdated

Comment thread tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php Outdated

Copilot AI review requested due to automatic review settings July 8, 2026 07:50

Copilot started reviewing on behalf of v-dumas July 8, 2026 07:50 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php Outdated

Copilot AI review requested due to automatic review settings July 8, 2026 08:03

Copilot started reviewing on behalf of v-dumas July 8, 2026 08:03 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php

Copilot AI review requested due to automatic review settings July 8, 2026 11:47

Copilot started reviewing on behalf of v-dumas July 8, 2026 11:48 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php

Copilot AI review requested due to automatic review settings July 8, 2026 11:56

Copilot started reviewing on behalf of v-dumas July 8, 2026 11:57 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php

Copilot AI review requested due to automatic review settings July 8, 2026 12:14

Copilot AI review requested due to automatic review settings July 8, 2026 12:21

Copilot started reviewing on behalf of v-dumas July 8, 2026 12:21 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 8, 2026 12:28

Copilot started reviewing on behalf of v-dumas July 8, 2026 12:28 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread datamodels/2.x/itop-tickets/module.itop-tickets.php

Copilot AI review requested due to automatic review settings July 8, 2026 12:44

Copilot started reviewing on behalf of v-dumas July 8, 2026 12:45 View session

v-dumas and others added 13 commits July 8, 2026 14:52

N°9553 - LoadLocalizedData: review feedbacks + improved tests

93fb185

N°9553 - take into account feedbacks from review

4033151

N°9553 - fix typo

4f7b7ae

N°9553 - Refactor methods to be clearer on what it does

b5575b3

N°9553 - Support of localized sample and structural files

3d3c9f6

N°9553 - Finalize localized sample and structural data

65d1578

Fix change from pattern to en_us.xml hardcoded

80f44b0

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix method header

45849ad

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix test method header comment

3caea82

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

N°9553 - Fix method signature

495aac8

N°9553 - Add german structural and sample data

912915c

N°9553 - Fix file renaming

ed5dfaf

N°9553 - rename structural Query files

24713bc

v-dumas force-pushed the feature/9553-ModuleInstaller-Feedbacks branch from b6f4b31 to 24713bc Compare July 8, 2026 12:53

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread datamodels/2.x/itop-tickets/module.itop-tickets.php

Copilot AI review requested due to automatic review settings July 8, 2026 12:55

Copilot started reviewing on behalf of v-dumas July 8, 2026 12:55 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread setup/moduleinstaller.class.inc.php

Restore Default language retrieval in case of Upgrade

e3d799c

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 8, 2026 13:45

Copilot started reviewing on behalf of v-dumas July 8, 2026 13:46 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Uh oh!

Conversation

v-dumas commented Jun 1, 2026

Base information

Proposed solution (bug and enhancement)

Checklist before requesting a review

Uh oh!

greptile-apps Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Molkobain commented Jun 23, 2026

Uh oh!

v-dumas commented Jun 24, 2026

Uh oh!

v-dumas commented Jul 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

greptile-apps Bot commented Jun 1, 2026 •

edited

Loading