Skip to content

ci: gate cacti.pot updates on i18n diff instead of regenerating - #150

Merged
TheWitness merged 2 commits into
developfrom
ci/i18n-pot-gate
Oct 5, 2026
Merged

TheWitness merged 2 commits into
developfrom
ci/i18n-pot-gate

Conversation

@TheWitness

Copy link
Copy Markdown
Member

What

Replaces the Verify translation template is up to date CI step. It no longer
regenerates locales/po/cacti.pot with locales/build_gettext.sh and compares
it (ignoring POT-Creation-Date). Instead it inspects the pull request diff and
requires that locales/po/cacti.pot be part of the PR whenever the translatable
strings actually change.

Why

The old check coupled CI to an exact gettext/xgettext toolchain: a different
GNU gettext version reorders or re-wraps the template and the step fails even
when no translatable string changed. The real intent is simply "if you changed
i18n strings, regenerate and commit the template".

How

New script tests/bin/check-i18n-pot.php:

  • Diffs the branch against the PR base SHA (same approach as
    tests/bin/patch-coverage.php).
  • Flags a change when any added/removed line contains one of the Cacti i18n
    calls that build_gettext.sh feeds to xgettext
    (__ __n __x __xn __esc __esc_n __esc_x __esc_xn __date __gettext), limited to
    the files that feed the template (find . -maxdepth 2 -name '*.php').
  • A pure re-indent or move of an existing i18n line is not treated as a
    change (the added/removed i18n lines are compared as sets).
  • If i18n strings changed and locales/po/cacti.pot is not in the PR, the
    step fails with the list of offending files; otherwise it passes.

The step now runs on pull_request events only.

Testing

Validated locally against real branch diffs: adding an __() call without a pot
update fails; adding it with the pot update passes; a non-i18n change passes; a
pure re-indent of an i18n line passes; deleting a template-feeding PHP file that
contained __() without a pot update fails.

The "Verify translation template is up to date" step regenerated
locales/po/cacti.pot with locales/build_gettext.sh and compared it
(ignoring POT-Creation-Date). That couples the check to an exact
gettext/xgettext toolchain and fails on unrelated version drift.

Replace it with tests/bin/check-i18n-pot.php, which inspects the pull
request diff: if any changed line adds, removes, or edits a Cacti i18n
call (__(), __n(), __esc(), ... the same keywords build_gettext.sh
feeds xgettext) in a file that feeds the template, then
locales/po/cacti.pot must also be part of the pull request. The check
fails only when that pot update is missing.

The step now runs only on pull_request events and diffs against the
PR base SHA, mirroring the existing patch-coverage.php approach.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The validator has false-negative and false-positive cases that make the CI gate unreliable.

Review effort: Balanced
Findings: None

What changed in this PR

Replaces toolchain-dependent POT regeneration with PR-diff-based i18n validation.

Changes:

  • Adds a PHP script to detect changed translation calls.
  • Updates CI to run it on pull requests.
File Description
tests/​bin/​check-i18n-pot.php Implements diff-based POT validation.
.github/​workflows/​plugin-ci-workflow.yml Invokes the validator for pull requests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

cigamit
cigamit previously approved these changes Oct 5, 2026
Address the reviewer note that the pot gate had false-positive and
false-negative cases.

The previous check scanned the unified diff line by line and compared
whole normalised lines. That produced:

- false positives: changing unrelated code on a line that also holds an
  i18n call (e.g. a width argument next to __()) demanded a pot update
  even though the translatable string was unchanged; and
- false negatives: a multi-line i18n call whose string literal sits on
  its own line was missed, so changing the string slipped through.

Rebuild the comparison from the tokens instead. For the merge-base and
for HEAD, tokenize every template-feeding PHP file (the same
find . -maxdepth 2 set build_gettext.sh scans), find the Cacti i18n
helper calls, and collect the literal string arguments that actually
form each pot entry (msgctxt/msgid/plural, per the xgettext -k spec).
A call only contributes when those positions are constant strings,
mirroring xgettext. If the extracted set differs between base and HEAD,
locales/po/cacti.pot must be part of the pull request.

Because it reads whole files rather than diff lines, surrounding-code
edits and quote-style-only changes no longer trigger it, and multi-line
calls are handled correctly.
@TheWitness

Copy link
Copy Markdown
Member Author

Addressed the automated review's "false-negative and false-positive" concern in tests/bin/check-i18n-pot.php (commit 080e064).

Root cause

The first version scanned the unified diff line-by-line and compared whole normalised lines containing an i18n call. That is both too loose and too strict. I reproduced both failure modes against real branch diffs:

  • False positive — changing only unrelated code on a line that also holds an i18n call (e.g. html_start_box(__('New Report', 'reportit'), '60%' ...) → '70%') demanded a cacti.pot update even though the translatable string never changed.
  • False negative — a multi-line call whose literal sits on its own line:
    $x = __(
        'Original string',   // change this line only
        'reportit'
    );
    was missed, because the changed line does not itself contain __(.

Fix

The check no longer looks at diff lines. For the merge-base and for HEAD it tokenizes every template-feeding PHP file (the same find . -maxdepth 2 -name '*.php' set build_gettext.sh scans), finds the Cacti i18n helper calls, and collects the literal string arguments that actually form each pot entry — msgctxt/msgid/plural per the xgettext -k spec (__ __n:1,2 __x:1c,2 __xn:1c,2,3 __esc …). A call only contributes when those positions are constant strings, mirroring what xgettext can extract. If the extracted set differs between base and HEAD, locales/po/cacti.pot must be part of the PR.

Because it reads whole files via the tokenizer rather than diff lines:

  • surrounding-code edits next to an i18n call no longer trigger it (no false positive),
  • multi-line calls are handled correctly (no false negative),
  • a pure re-indent or a quote-style-only change ('x' ↔ "x") is not treated as a string change, and
  • method calls / declarations ($o->__(), C::__(), function __) are excluded.

Validation — ran a scenario matrix locally against real commits, all passing:

Scenario Expected Result
surrounding-code change only no pot required PASS
multi-line literal change pot required PASS
add new __(), no pot fail PASS
add new __() with pot pass PASS
non-i18n change pass PASS
pure re-indent of i18n line pass PASS
delete file containing __(), no pot fail PASS
quote-style-only change pass PASS

php -l clean on the pushed blob.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new gate has false positives and can incorrectly accept deletion of the POT file.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread .github/workflows/plugin-ci-workflow.yml
@TheWitness
TheWitness merged commit 1220ba5 into develop Oct 5, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants