1.7.0 Alpha 1 - #6
Merged
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideImplements a new structured reporting layer for module operations, replaces ad‑hoc HTML log sanitizing with a parse‑then‑render pipeline, centralizes language handling with robust fallbacks, hardens module-logo URL handling, improves RTL and accessibility in the admin UI, and wires the new APIs into existing services while keeping full backward compatibility. Sequence diagram for legacy HTML log to safe HTML and structured resultsequenceDiagram
actor Admin
participant AdminBulkPage
participant ModuleActionService
participant Core as xoops_module_install
participant Result as ModuleActionResult
participant Adapter as LegacyModuleReportAdapter
participant HtmlRenderer as LogEventHtmlRenderer
participant OpResult as ModuleOperationResult
Admin ->> AdminBulkPage: submit bulk install
AdminBulkPage ->> ModuleActionService: runMany(ACTION_INSTALL, dirnames)
loop per dirname
ModuleActionService ->> Core: xoops_module_install(dirname)
Core -->> ModuleActionService: legacy HTML log (string)
ModuleActionService ->> Result: new ModuleActionResult(dirname, status, message, action)
end
ModuleActionService -->> AdminBulkPage: array ModuleActionResult
rect rgb(235,235,255)
note over AdminBulkPage,HtmlRenderer: Legacy HTML rendered safely for admin UI
loop per Result
AdminBulkPage ->> Result: messageHtml()
Result ->> Adapter: parse(message)
Adapter -->> Result: events : array LogEvent
Result ->> HtmlRenderer: render(events)
HtmlRenderer -->> AdminBulkPage: safe HTML fragment
end
end
rect rgb(235,255,235)
note over ModuleActionService,OpResult: New structured API for non-HTML consumers
AdminBulkPage ->> ModuleActionService: operationResult(action, dirname)
ModuleActionService ->> Result: runOne(action, dirname)
Result -->> ModuleActionService: ModuleActionResult
ModuleActionService ->> Result: toOperationResult()
Result ->> Adapter: parse(message)
Adapter -->> Result: events
Result -->> ModuleActionService: ModuleOperationResult
ModuleActionService -->> OpResult: ModuleOperationResult
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe release adds structured module-operation reports, legacy transcript parsing, localized fallback messages, RTL-aware admin rendering, validated module-logo URLs, updated admin CSS, expanded tests, and 1.7.0 Alpha 1 release documentation. ChangesReporting and localization
Admin presentation and release metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminBulkPage
participant ModuleActionService
participant ModuleOperationResult
participant LogEventHtmlRenderer
AdminBulkPage->>ModuleActionService: Request operation results
ModuleActionService->>ModuleOperationResult: Return structured results
AdminBulkPage->>LogEventHtmlRenderer: Render result events
LogEventHtmlRenderer-->>AdminBulkPage: Return escaped HTML
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="class/AdminBulkPage.php" line_range="620-629" />
<code_context>
+ public static function moduleLogoUrl(string $dirname, ?array $info): ?string
</code_context>
<issue_to_address>
**question:** The dirname validation in moduleLogoUrl() may be too strict and could drop logos for modules with dots in their directory name.
`moduleLogoUrl()` currently rejects any `$dirname` not matching `/^[a-zA-Z0-9_-]+$/`. Some existing XOOPS modules use dots in dirnames (e.g. `foo.bar`), so their logos would silently disappear even though their manifests/files are valid. If core/module catalog/installer already enforce this character set, then this is consistent; otherwise, consider either (a) aligning the regex with what those systems accept, or (b) clearly documenting that logos are suppressed for non-matching dirnames to avoid perceived regressions on upgrade.
</issue_to_address>
### Comment 2
<location path="class/Lang.php" line_range="94-108" />
<code_context>
+ * empty. The blankness test is on the cast value, which gets both of these
+ * right for the same reason.
+ */
+ private static function usable(string $constant): ?string
+ {
+ if (! \defined($constant)) {
+ return null;
+ }
+
+ $value = \constant($constant);
+ if (! \is_scalar($value)) {
+ return null;
+ }
+
+ $value = (string) $value;
+
+ return '' === \trim($value) ? null : $value;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Using trim() without a charlist means NBSP-only constants are treated as valid translations; verify this is intended.
Because `usable()` relies on `(string)$value` and `trim()` without a charlist, NBSP-only constants (e.g. ` ` or `"\u{00A0}"`) will be treated as non-empty and used, even though they render as visually blank. If that’s not desired, consider either adding NBSP to the trim charlist or doing an explicit NBSP-only check (e.g. a `/^\s*\x{00A0}*$/u` guard) before accepting the value.
```suggestion
private static function usable(string $constant): ?string
{
if (! \defined($constant)) {
return null;
}
$value = \constant($constant);
if (! \is_scalar($value)) {
return null;
}
$value = (string) $value;
// Treat NBSP-only (and whitespace-only) constants as unusable by including NBSP in the trim charlist
$trimmed = \trim($value, " \t\n\r\0\x0B\xC2\xA0");
return '' === $trimmed ? null : $value;
}
```
</issue_to_address>
### Comment 3
<location path="docs/lang_diff.txt" line_range="50-52" />
<code_context>
+ was untranslatable regardless of the language pack installed.
+- Every lookup goes through class/Lang.php, which carries the English text as a
+ fallback: a language pack that omits one of these degrades to English instead of
+ fataling (an undefined constant is a fatal Error on PHP 8, not a notice).
+- Translators: %1$s / %2$s ordering may be reordered freely; Lang::format() passes
+ the arguments positionally.
</code_context>
<issue_to_address>
**issue (typo):** Consider replacing the non-standard verb "fataling" with a clearer phrase like "causing a fatal error".
Using the non-standard term here may confuse readers; consider phrasing it as "instead of causing a fatal error" while leaving the rest of the sentence unchanged.
```suggestion
- Every lookup goes through class/Lang.php, which carries the English text as a
- fallback: a language pack that omits one of these degrades to English instead of
- causing a fatal error (an undefined constant is a fatal Error on PHP 8, not a notice).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+620
to
+629
| public static function moduleLogoUrl(string $dirname, ?array $info): ?string | ||
| { | ||
| $dirname = \trim($dirname); | ||
| if ( | ||
| null === $info | ||
| || 1 !== \preg_match('/^[a-zA-Z0-9_-]+$/', $dirname) | ||
| ) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
question: The dirname validation in moduleLogoUrl() may be too strict and could drop logos for modules with dots in their directory name.
moduleLogoUrl() currently rejects any $dirname not matching /^[a-zA-Z0-9_-]+$/. Some existing XOOPS modules use dots in dirnames (e.g. foo.bar), so their logos would silently disappear even though their manifests/files are valid. If core/module catalog/installer already enforce this character set, then this is consistent; otherwise, consider either (a) aligning the regex with what those systems accept, or (b) clearly documenting that logos are suppressed for non-matching dirnames to avoid perceived regressions on upgrade.
Comment on lines
+94
to
+108
| private static function usable(string $constant): ?string | ||
| { | ||
| if (! \defined($constant)) { | ||
| return null; | ||
| } | ||
|
|
||
| $value = \constant($constant); | ||
| if (! \is_scalar($value)) { | ||
| return null; | ||
| } | ||
|
|
||
| $value = (string) $value; | ||
|
|
||
| return '' === \trim($value) ? null : $value; | ||
| } |
There was a problem hiding this comment.
suggestion: Using trim() without a charlist means NBSP-only constants are treated as valid translations; verify this is intended.
Because usable() relies on (string)$value and trim() without a charlist, NBSP-only constants (e.g. or "\u{00A0}") will be treated as non-empty and used, even though they render as visually blank. If that’s not desired, consider either adding NBSP to the trim charlist or doing an explicit NBSP-only check (e.g. a /^\s*\x{00A0}*$/u guard) before accepting the value.
Suggested change
| private static function usable(string $constant): ?string | |
| { | |
| if (! \defined($constant)) { | |
| return null; | |
| } | |
| $value = \constant($constant); | |
| if (! \is_scalar($value)) { | |
| return null; | |
| } | |
| $value = (string) $value; | |
| return '' === \trim($value) ? null : $value; | |
| } | |
| private static function usable(string $constant): ?string | |
| { | |
| if (! \defined($constant)) { | |
| return null; | |
| } | |
| $value = \constant($constant); | |
| if (! \is_scalar($value)) { | |
| return null; | |
| } | |
| $value = (string) $value; | |
| // Treat NBSP-only (and whitespace-only) constants as unusable by including NBSP in the trim charlist | |
| $trimmed = \trim($value, " \t\n\r\0\x0B\xC2\xA0"); | |
| return '' === $trimmed ? null : $value; | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR prepares the 1.7.0 Alpha 1 release by introducing a structured reporting model for module operations, adding a safe/robust language lookup layer with English fallbacks, and updating the admin UI/CSS for improved RTL/accessibility while updating release metadata and documentation.
Changes:
- Add
class/Report/*structured report types plus a legacy HTML-log adapter and safe HTML renderer. - Introduce
Langhelper for safe translation lookup/formatting with reliable English fallbacks; migrate admin strings away from raw constant references. - Update admin UI rendering/CSS (RTL isolation, removal of inline styles, safer logo handling) and bump release/version documentation.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| xoops_version.php | Bumps module version/status/date to 1.7.0 Alpha 1. |
| README.md | Updates current version info and links to lang_diff.txt. |
| language/english/admin.php | Adds new translatable admin/report strings used by services/UI. |
| docs/TUTORIAL.md | Updates tutorial version header and adds language-diff reference. |
| docs/readme.txt | Updates version header and language-diff reference. |
| docs/lang_diff.txt | Documents newly added language constants for 1.7.0. |
| docs/changelog.txt | Adds 1.7.0 Alpha 1 changelog entry and clarifies older headings. |
| class/Set/ModuleSetResolver.php | Replaces hardcoded notices with Lang::text() lookups. |
| class/Set/ModuleSetApplier.php | Makes snapshot/reason strings translatable via Lang::*. |
| class/Report/Outcome.php | Adds enum for operation outcomes with CSS class mapping. |
| class/Report/ModuleOperationResult.php | Adds structured result value object (events/outcome/severity). |
| class/Report/LogSeverity.php | Adds severity enum with CSS class mapping for rendering. |
| class/Report/LogFragment.php | Adds fragment model for plain-text + emphasis runs. |
| class/Report/LogEventHtmlRenderer.php | Adds escape-once renderer that emits fixed markup only. |
| class/Report/LogEvent.php | Adds event model (severity/fragments/depth) and helpers. |
| class/Report/LegacyModuleReportAdapter.php | Parses legacy core HTML logs into structured events/results. |
| class/ModuleActionService.php | Migrates messages to Lang::* and adds structured result APIs. |
| class/ModuleActionResult.php | Adds event/text/projection APIs and switches HTML rendering to renderer. |
| class/Lang.php | Adds safe translation lookup/formatting with lazy loading and fallbacks. |
| class/AdminBulkPage.php | Updates admin bulk UI rendering: safer strings, logo validation, RTL isolation, structured report layout. |
| CHANGELOG.md | Adds 1.7.0 Alpha 1 release notes. |
| assets/css/admin.css | Adds new report layout/styles, RTL logical properties, and utility classes (e.g., .installer-hidden). |
| admin/sets.php | Removes inline styles, uses validated logo URLs, improves RTL/isolation in summary, and reuses translatable labels. |
| admin/index.php | Wraps sprintf-rendered count sentences in <bdi> for RTL correctness. |
| admin/admin_footer.php | Replaces inline style with CSS class for footer centering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
class/AdminBulkPage.php (2)
620-666: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueValidation chain is sound.
I traced the guards against the two relevant threats and both are blocked:
- Scheme and absolute-URL injection:
'https://evil/x.png'splits into segments containing empty strings, which line 637 rejects.'javascript:alert(1)'passes the segment check but fails therealpath()andis_file()check at lines 647-650.- Traversal:
'..'segments are rejected at line 637, and therealpath()containment check at lines 655-658 is an independent second layer. Becauserealpath()resolves symlinks before the prefix comparison, a symlink out of the module directory is also rejected.The per-segment
rawurlencode()at line 663 is correct:rawurlencode()does not encode/, so encoding each segment and rejoining preserves the path structure. All three call sites escape the returned URL before writing it into thesrcattribute.One optional hardening: the function proves the target is a file inside the module directory but does not restrict the file type, so a manifest may point at any file in its own directory. That directory is already web-served, so this exposes nothing new. An extension allowlist would still narrow the surface.
🔒️ Optional extension allowlist
$logoPath = \realpath($moduleRoot . \DIRECTORY_SEPARATOR . \implode(\DIRECTORY_SEPARATOR, $segments)); if (false === $logoPath || ! \is_file($logoPath)) { return null; } + + // A logo is an image. Anything else in the module directory is not one, and + // an <img> pointing at it can only be a manifest mistake or an attempt. + $allowed = ['png', 'gif', 'jpg', 'jpeg', 'svg', 'webp', 'ico']; + if (! \in_array(\mb_strtolower(\pathinfo($logoPath, \PATHINFO_EXTENSION)), $allowed, true)) { + return null; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@class/AdminBulkPage.php` around lines 620 - 666, Optionally harden moduleLogoUrl by validating the resolved logo file’s extension against an explicit allowlist of supported image types before returning its URL; preserve the existing path validation and URL construction for allowed files.
343-347: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist
$toggleTitleout of the row loop.
$toggleTitledoes not depend on the current row. The code recomputes the language lookup and the escaping for every module row. Move it above theforeachat line 299.♻️ Proposed change
$count = 0; $even = false; + $toggleTitle = \htmlspecialchars( + Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), + \ENT_QUOTES | \ENT_SUBSTITUTE, + 'UTF-8' + ); foreach ($dirnames as $file) {$content .= "<tr id='" . $dirnameEsc . "' class='" . $rowClass . "' data-search=\"" . $searchBlob . '"' . ">\n"; - $toggleTitle = \htmlspecialchars( - Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), - \ENT_QUOTES | \ENT_SUBSTITUTE, - 'UTF-8' - ); $content .= " <td class='img installer-mod-toggle' onclick=\"" . $toggleJs . "\" title='" . $toggleTitle . "'>";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@class/AdminBulkPage.php` around lines 343 - 347, Move the `$toggleTitle` initialization, including the `Lang::text` lookup and `htmlspecialchars` call, before the `foreach` loop that begins near line 299 in the admin bulk page rendering flow. Keep the existing value and row output behavior unchanged while ensuring it is computed only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/css/admin.css`:
- Around line 188-191: Update the --installer-badge-notinst color used by
.installer-log-warning to a darker value such as `#b35309`, ensuring warning
transcript text and white warning-badge text meet the required contrast.
In `@class/AdminBulkPage.php`:
- Around line 349-352: Update the logo markup in the row-rendering code around
the image construction to use an empty alt attribute, matching renderReport()’s
decorative-image treatment. Preserve the existing escaped module name in the
adjacent cell and leave the image source handling unchanged.
In `@class/Lang.php`:
- Around line 181-192: Update the private static load() method to wrap both
Xmf\Language::load() and the xoops_loadLanguage() fallback in try/catch
\Throwable, swallowing the load failure so callers such as text() and core() can
use the existing English fallback. Preserve the current class/function checks
and attempt-tracking behavior without retrying.
In `@class/Report/LogSeverity.php`:
- Around line 24-41: Run the repository formatter on all four affected files. In
class/Report/LogSeverity.php lines 24-41 and class/Report/Outcome.php lines
24-39, remove declaration and match-arm alignment padding; in
class/Report/ModuleOperationResult.php lines 107-109 expand isOk(), isSkipped(),
and isFailed(), and remove padding in severity() and toArray(); in
class/Report/LegacyModuleReportAdapter.php lines 37-38 remove constant and
local-assignment padding and add required blank lines around the switch cases at
lines 151-171. Preserve all logic.
In `@docs/changelog.txt`:
- Around line 39-41: Reconcile the translation-key count between
docs/changelog.txt lines 39-41 and docs/lang_diff.txt lines 12-44: verify the
final set of definitions, then either correct the changelog count or remove
stale entries from the list so both documents report the same count.
In `@docs/TUTORIAL.md`:
- Line 1: Update the tutorial title at the document heading to use the exact
prerelease label “1.7.0 Alpha 1,” matching the release identifier in
xoops_version.php and docs/changelog.txt.
---
Nitpick comments:
In `@class/AdminBulkPage.php`:
- Around line 620-666: Optionally harden moduleLogoUrl by validating the
resolved logo file’s extension against an explicit allowlist of supported image
types before returning its URL; preserve the existing path validation and URL
construction for allowed files.
- Around line 343-347: Move the `$toggleTitle` initialization, including the
`Lang::text` lookup and `htmlspecialchars` call, before the `foreach` loop that
begins near line 299 in the admin bulk page rendering flow. Keep the existing
value and row output behavior unchanged while ensuring it is computed only once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90d1a613-1608-4986-8dd0-ca592495f92a
📒 Files selected for processing (25)
CHANGELOG.mdREADME.mdadmin/admin_footer.phpadmin/index.phpadmin/sets.phpassets/css/admin.cssclass/AdminBulkPage.phpclass/Lang.phpclass/ModuleActionResult.phpclass/ModuleActionService.phpclass/Report/LegacyModuleReportAdapter.phpclass/Report/LogEvent.phpclass/Report/LogEventHtmlRenderer.phpclass/Report/LogFragment.phpclass/Report/LogSeverity.phpclass/Report/ModuleOperationResult.phpclass/Report/Outcome.phpclass/Set/ModuleSetApplier.phpclass/Set/ModuleSetResolver.phpdocs/TUTORIAL.mddocs/changelog.txtdocs/lang_diff.txtdocs/readme.txtlanguage/english/admin.phpxoops_version.php
Comment on lines
+39
to
+41
| Internationalisation and admin UI | ||
| - Added 27 translatable admin-report strings and Lang helpers with safe English | ||
| fallbacks for missing, blank, non-scalar, or malformed translations. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the documented translation-key count.
docs/lang_diff.txt lists 30 new definitions, but docs/changelog.txt says 27. Update the summary or the list before publishing.
docs/changelog.txt#L39-L41: correct the reported count.docs/lang_diff.txt#L12-L44: remove stale entries or align the list with the final count.
📍 Affects 2 files
docs/changelog.txt#L39-L41(this comment)docs/lang_diff.txt#L12-L44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/changelog.txt` around lines 39 - 41, Reconcile the translation-key count
between docs/changelog.txt lines 39-41 and docs/lang_diff.txt lines 12-44:
verify the final set of definitions, then either correct the changelog count or
remove stale entries from the list so both documents report the same count.
Alignment padding on enum cases and match arms, and single-line method bodies, are both rejected by the repository fixer configuration. The QA gate runs cs:check first and exits non-zero, so every downstream step (analyse, rector, test) was being skipped on all four PHP versions. No logic change in any of these files.
The module row built its toggle handler by concatenating an
HTML-escaped dirname into a JavaScript string literal. A browser
HTML-decodes an attribute value before compiling it as JavaScript, so
ENT_QUOTES turning an apostrophe into ' does not help: it decodes
back to a bare quote and closes the literal. A folder named
foo'); alert(1);//
therefore produced a live call to alert() on every admin bulk page.
XoopsLists::getDirListAsArray() applies no character filter, so the
name reaches the row renderer verbatim, and an apostrophe is legal on
both NTFS and ext4. json_encode() now emits the JS literal and the HEX
flags keep angle brackets, ampersands and both quote marks out of the
attribute entirely.
Also in moduleLogoUrl(): the control-character guard tested preg_match()
for truth, but preg_match() returns int|false and its false-on-error is
falsy, so the guard failed open exactly when it mattered. It now tests
against 1, matching the dirname check above it. Containment proves the
target sits inside the module but not that it is an image, so an
extension allowlist was added — a manifest could otherwise point the
<img> at a contained .php and have the browser execute it server-side.
The dirname allowlist is documented rather than widened: it is the
module's dirname contract, applied identically by
ModuleCatalog::existsOnDisk() and ModuleSet::withDirnames(), so a dotted
folder is rejected before a row is ever built.
Row logos now use alt='' to match renderReport(); the module name is
rendered in the adjacent cell, so a described image made a screen reader
announce it twice per row. The toggle title is hoisted out of the loop.
Both load routes include third-party PHP. A language file is ordinary PHP, so it can throw, and a parse error in one is a catchable ParseError on PHP 8; Xmf\Language::loadFile() additionally throws outright on a control character in the path. Uncaught, any of these propagated out of text() and core() and took the admin page down — precisely the failure this class exists to prevent, since a pack bad enough to throw is the pack whose fallback matters most. The attempt flags are set by the callers before load() runs, so swallowing here cannot cause a retry. usable() also treated an NBSP-only constant as a real translation, because trim() knows only ASCII whitespace. A lone NBSP is what a translation tool emits for "intentionally empty" and it renders as invisibly as a space, so it now falls back. That check is a pattern, deliberately, and not a wider trim() charlist: trim() matches BYTES, and 0xA0 is an ordinary UTF-8 continuation byte. Trimming "\xC2\xA0" truncates any value ending in a character that happens to end in 0xA0 — a dagger, U+2020, becomes invalid UTF-8 — which would put a mojibake bug in the one class meant to survive bad packs. The adapter already documents this same trap. preg_match() returns false on invalid UTF-8, which leaves such a value usable, matching the answer the ASCII test already gave it.
ModuleActionResult carried a formal @deprecated tag while runOne() and runMany() — the primary public API of ModuleActionService — still return it and AdminBulkPage still renders from it. Marking a type deprecated that no caller in the module has migrated away from made every internal use a deprecation violation: 116 of the 138 analysis errors this release introduced. The prose stays, so the direction is still documented; the tag goes on when the internal consumers have moved. The fragment flush in the transcript parser was a by-reference closure rebuilt on every line of every transcript. Because static analysis reads a closure body against the state at its declaration, where the buffer has just been set to '', its emptiness guard was reported as always false. The guard is correct at runtime — it was the shape that could not be verified — so it is now a method.
--installer-badge-notinst was the Bootstrap orange #fd7e14. That value is a fill colour, and .installer-log-warning also draws warning transcript text in it: 2.6:1 against the white report background for normal-size body text, and 2.4:1 for the white text on the badge. Both are under the 4.5:1 AA floor. #b35309 clears it at 5.05:1 either way. The var() fallbacks further down already named this colour. They never applied — a fallback is used only when the custom property is undefined, and the :root block always defines it.
The report parser, renderer, structured result and language helper shipped without a single test. The parser is the module's trust boundary — everything upstream of it is core's unescaped HTML — and its contract is a round trip, so it is reviewed by re-running fixtures rather than by re-reading regexes. Adds 91 tests: transcript parsing (line splitting, emphasis, severity spans that wrap a line break, unclosed and stray spans, NBSP indent including the odd-unit remainder, marker forging, script content, CRLF, entity collapsing), an end-to-end check that hostile markup cannot reach rendered HTML, the renderer's escape-once and no-attribute-injection properties, every ModuleOperationResult factory and severity branch, the module-logo rejection contract, and the language fallback policy including the UTF-8 boundary cases. The logo positive path and the extension allowlist are not covered: both need a real file under XOOPS_ROOT_PATH/modules/<dirname>/, which a unit run has nowhere to write. They belong to the integration suite.
The 1.7.0 section adds 30 language constants, not 27; lang_diff.txt was right and both changelogs were wrong. The suite claim was 169 tests against an actual 18, and named crash and timeout coverage that does not exist anywhere in tests/ — it now states the real figure and scope. Also: the tutorial heading dropped the prerelease label that every other file carries, and a note in lang_diff used "fataling" for what is a fatal error.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
class/Report/LegacyModuleReportAdapter.php (1)
92-99: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve pairing for red error spans.
Line 97 converts every
</span>into an error-close marker. A nested non-error span can then close an active red span.For example,
<span style="color:red">bad <span>detail</span><br>still bad</span>marksstill badasInfo. This can remove an error event and cause an otherwise successfulModuleOperationResultto reportSuccessinstead ofWarning.Emit an error-close marker only for the closing tag that matches a recognized red span. Add a regression case with a nested ordinary span and a line break.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@class/Report/LegacyModuleReportAdapter.php` around lines 92 - 99, Update the span-processing logic in LegacyModuleReportAdapter so closing markers are emitted only for recognized red error spans, not every </span> tag; preserve nesting so ordinary nested spans cannot close the active error span. Add a regression case covering a red span containing a nested ordinary span and a line break, ensuring the resulting ModuleOperationResult remains Warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/class/AdminBulkPageLogoUrlTest.php`:
- Around line 79-85: Update absentModuleDirectoryIsRejected to generate a
genuinely unique nonexistent module directory for each run, replacing the
deterministic bin2hex('x') suffix with a suitable runtime uniqueness mechanism
while preserving the assertion that AdminBulkPage::moduleLogoUrl returns null.
---
Outside diff comments:
In `@class/Report/LegacyModuleReportAdapter.php`:
- Around line 92-99: Update the span-processing logic in
LegacyModuleReportAdapter so closing markers are emitted only for recognized red
error spans, not every </span> tag; preserve nesting so ordinary nested spans
cannot close the active error span. Add a regression case covering a red span
containing a nested ordinary span and a line break, ensuring the resulting
ModuleOperationResult remains Warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4eb94b2-3222-43f3-bc7d-d0dfeccc1596
📒 Files selected for processing (18)
CHANGELOG.mdassets/css/admin.cssclass/AdminBulkPage.phpclass/Lang.phpclass/ModuleActionResult.phpclass/ModuleActionService.phpclass/Report/LegacyModuleReportAdapter.phpclass/Report/LogSeverity.phpclass/Report/ModuleOperationResult.phpclass/Report/Outcome.phpdocs/TUTORIAL.mddocs/changelog.txtdocs/lang_diff.txttests/Unit/class/AdminBulkPageLogoUrlTest.phptests/Unit/class/LangTest.phptests/Unit/class/Report/LegacyModuleReportAdapterTest.phptests/Unit/class/Report/LogEventHtmlRendererTest.phptests/Unit/class/Report/ModuleOperationResultTest.php
💤 Files with no reviewable changes (1)
- class/ModuleActionService.php
🚧 Files skipped from review as they are similar to previous changes (11)
- class/Report/Outcome.php
- docs/TUTORIAL.md
- class/Report/LogSeverity.php
- CHANGELOG.md
- docs/changelog.txt
- assets/css/admin.css
- class/ModuleActionResult.php
- class/Report/ModuleOperationResult.php
- docs/lang_diff.txt
- class/Lang.php
- class/AdminBulkPage.php
The constructor assigns $this->modules unconditionally before any read, so the = [] initialiser was dead. RemoveDefaultValueFromAssignedPropertyRector flags it. No behaviour change. This is a pre-existing issue in a file this release does not otherwise touch, and it surfaced only now: the QA script runs cs:check first and that gate was failing, so rector never ran in CI. With formatting and static analysis clean, rector finally got its turn. It broke without a code change because composer.lock is not tracked, so every CI run resolves dependencies afresh — rector 2.6 is newer than the version the previous green run used. Worth deciding separately whether to commit the lock, or to pin the QA tools, so that a tool release cannot turn an untouched branch red.
…layer
Two defects, both in the escaping work from earlier in this branch.
renderYesNoRadios() built its selectModule() handler the old way, with
htmlspecialchars() into a JavaScript string literal. That is the same
defect already fixed for toggleModuleRow(), in a call site the first
pass did not look for — the fix went where the report pointed instead of
sweeping the file.
The first fix was also incomplete. JSON_HEX_QUOT escapes quotes inside
the encoded string, but not json_encode's own delimiters, so the result
still began and ended with a literal double quote. Interpolated into
onclick="…" that closed the attribute early and turned the remainder
into stray attributes on the element. The earlier check missed it by
running html_entity_decode() over the string without ever parsing it as
HTML, which cannot show an attribute ending in the wrong place.
Both layers are now applied in one place: json_encode() with the HEX
flags for the JavaScript, then htmlspecialchars() over the whole call
for the attribute. Both handlers go through jsCall(), so a third one
cannot pick a different answer.
Verified by parsing the rendered markup with DOMDocument: zero parse
errors, the input carries exactly its six intended attributes with no
injected extras, and the DOM reports the handler as
selectModule("foo'); alert(1);\/\/", this) — payload inside the
literal. The new tests assert against the decoded attribute rather than
the raw markup, because raw-markup assertions pass against this bug.
… run
Only red spans carry severity, but every span shares one closing tag, and
the parser counted depth instead of tracking which span was which. A
</span> therefore ended whatever was innermost — so an ordinary span
nested inside a red one closed the red one:
<span style="color:red">bad <span>detail</span><br>still bad</span>
marked "still bad" as Info. Core nests spans routinely, so this was the
common case, not a hostile input. The visible cost is larger than a
mis-coloured line: dropping the error event also takes the result from
Warning back to Success, so a report whose transcript contained failures
read as clean.
Spans are now a stack of "was this one red", pushed by both the red and
the ordinary open marker and popped by each close, so every close ends
its own span. A close with nothing open pops nothing, which keeps the
existing tolerance for core's unbalanced fragments.
Three regression cases: an ordinary span nested in a red one, several
ordinary spans nesting and closing across a break, and an ordinary span
alone that must colour nothing. The earlier suite missed this because it
only ever tested one span at a time.
Also makes the absent-module dirname in the logo test random per run
rather than a fixed literal, so the case cannot quietly stop testing what
it claims the day something on disk takes that name.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/class/AdminBulkPageJsEscapingTest.php (1)
55-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the JavaScript argument preserves
$dirname.Lines 55-66 verify that the handler is syntactically safe. They do not verify that the first argument still equals the original
$dirname. A handler such asselectModule("", this)would pass these assertions and break module selection for escaped input.Decode the first JavaScript string argument and assert that it equals
$dirnamefor every data-provider case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/class/AdminBulkPageJsEscapingTest.php` around lines 55 - 66, The test around the handler assertions must also verify payload preservation, not only JavaScript syntax safety. Decode the first string argument from $decoded and assert that it equals the original $dirname for every data-provider case, while retaining the existing malformed-call and unsafe-character assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/class/AdminBulkPageJsEscapingTest.php`:
- Around line 55-66: The test around the handler assertions must also verify
payload preservation, not only JavaScript syntax safety. Decode the first string
argument from $decoded and assert that it equals the original $dirname for every
data-provider case, while retaining the existing malformed-call and
unsafe-character assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 458035e5-b4e9-4ff6-ad60-95cc649948c5
📒 Files selected for processing (7)
.scrutinizer.ymlclass/AdminBulkPage.phpclass/Report/LegacyModuleReportAdapter.phpclass/Set/ModuleSet.phptests/Unit/class/AdminBulkPageJsEscapingTest.phptests/Unit/class/AdminBulkPageLogoUrlTest.phptests/Unit/class/Report/LegacyModuleReportAdapterTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Unit/class/AdminBulkPageLogoUrlTest.php
- class/Report/LegacyModuleReportAdapter.php
- tests/Unit/class/Report/LegacyModuleReportAdapterTest.php
- class/AdminBulkPage.php
Greptile SummaryThe PR introduces structured, safely rendered module-operation reports and a resilient translation layer while hardening module-logo handling and improving the admin interface.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect established in the reviewed paths. The new reporting, translation, logo-validation, and admin-rendering paths preserve legacy APIs while escaping untrusted output and degrading safely for malformed language and manifest data.
|
| Filename | Overview |
|---|---|
| class/Report/LegacyModuleReportAdapter.php | Introduces the boundary parser that converts legacy installation-log HTML into structured, escaped-on-render events. |
| class/Report/ModuleOperationResult.php | Adds an immutable structured result API with outcome, severity, reason, transcript, plain-text, and array representations. |
| class/ModuleActionResult.php | Preserves the legacy result contract while routing rendering and conversion through the new report model. |
| class/ModuleActionService.php | Localizes operation messages and exposes additive structured-result methods without changing existing runOne/runMany return types. |
| class/Lang.php | Adds lazy module/core language loading with validation of constant values and reliable English fallbacks. |
| class/AdminBulkPage.php | Adopts safe report rendering, validated logo URLs, context-aware JavaScript escaping, localized labels, and RTL-aware counters. |
| class/Set/ModuleSetApplier.php | Localizes module-set planning, snapshot, and execution messages while preserving existing operation behavior. |
| admin/sets.php | Uses validated logo rendering and CSS classes while improving translated controls and RTL plan summaries. |
| assets/css/admin.css | Expands scoped report, accessibility, responsive, RTL, and utility styling for the revised admin UI. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Module action] --> B[Legacy XOOPS HTML log]
B --> C[LegacyModuleReportAdapter]
C --> D[LogEvent values]
D --> E[LogEventHtmlRenderer]
D --> F[Plain text]
D --> G[ModuleOperationResult array / JSON]
E --> H[Admin operation report]
Reviews (1): Last reviewed commit: "fix(installer): pair span closes so a ne..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New features:
Summary by Sourcery
Introduce a structured, translatable, and RTL-aware reporting and language layer for module operations, while improving admin UI accessibility, safety, and release metadata for the 1.7.0 Alpha 1 release.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation