fix(system): defensive fallback when theme_set config rows are missing#44
Merged
mambax7 merged 14 commits intoMay 5, 2026
Merged
Conversation
Add a graceful-degradation path to the System theme-switch block.
system/blocks/system_blocks.php (b_system_themes_show):
Two `xoops_config` rows are required for this block to render —
`theme_set` (the active theme) and `theme_set_allowed` (the list
of permitted themes). If either is missing or unreadable —
e.g. mid-upgrade state, corrupted config row, custom installer
glitch — the previous code emitted PHP "Undefined index"
warnings and produced an empty select element, which in turn
blanked the page wherever the block was placed.
Capture both values up-front with sensible defaults
(`'default'` for the active theme, `[]` for the allowed list)
and, if the allowed list ends up empty, fall back to a
single-entry list containing the active theme. The block now
renders a usable single-option select rather than nothing.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughDerives a validated ChangesTheme Configuration Defense
Client-side Image URL Encoding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44 +/- ##
============================================
- Coverage 18.08% 18.07% -0.02%
Complexity 7840 7840
============================================
Files 665 665
Lines 42982 43024 +42
============================================
Hits 7775 7775
- Misses 35207 35249 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/system/blocks/system_blocks.php (1)
714-715:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
count($xoopsConfig['theme_set_allowed'])still bypasses the defensive fallback — fix the PR's own blind spot.The entire rationale of this PR is to eliminate
E_WARNING: Undefined array key "theme_set_allowed"when the config row is missing. Lines 672-682 correctly normalize everything into$allowedThemes, yet line 715 re-reads the raw config key directly. As of PHP 8, accessing an undefined array key emits a Warning, so this line will still fire the exact warning the PR claims to fix — and silently render a broken count in the block caption.🐛 Proposed fix
- $block['theme_select'] = $themeSelect . '<br>(' . sprintf(_MB_SYSTEM_NUMTHEME, '<strong>' - . count($xoopsConfig['theme_set_allowed']) . '</strong>') . ')<br>'; + $block['theme_select'] = $themeSelect . '<br>(' . sprintf(_MB_SYSTEM_NUMTHEME, '<strong>' + . count($allowedThemes) . '</strong>') . ')<br>';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/blocks/system_blocks.php` around lines 714 - 715, The block caption still reads count($xoopsConfig['theme_set_allowed']) which can trigger PHP 8 warnings; update the $block['theme_select'] construction to use the normalized $allowedThemes variable (created in the earlier normalization block around lines 672-682) instead of directly accessing $xoopsConfig['theme_set_allowed'], so the count() call becomes count($allowedThemes) and the warning is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@htdocs/modules/system/blocks/system_blocks.php`:
- Around line 714-715: The block caption still reads
count($xoopsConfig['theme_set_allowed']) which can trigger PHP 8 warnings;
update the $block['theme_select'] construction to use the normalized
$allowedThemes variable (created in the earlier normalization block around lines
672-682) instead of directly accessing $xoopsConfig['theme_set_allowed'], so the
count() call becomes count($allowedThemes) and the warning is avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2e828e9-c58b-4835-aecc-1acb70e17d6c
📒 Files selected for processing (1)
htdocs/modules/system/blocks/system_blocks.php
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves resilience of the System “theme switch” block (b_system_themes_show) so it can still render when xoops_config entries for theme_set and/or theme_set_allowed are missing or unreadable (e.g., mid-upgrade or corrupted config state), avoiding warnings and blank output.
Changes:
- Reads
theme_setandtheme_set_alloweddefensively with defaults ('default'and[]). - Falls back to a single-entry allowed list containing the current theme when the allowed list is empty.
- Uses the computed
$currentThemewhen constructing the select and screenshot URL.
Comment on lines
+678
to
+682
| $currentTheme = (string) ($xoopsConfig['theme_set'] ?? 'default'); | ||
| $allowedThemes = (array) ($xoopsConfig['theme_set_allowed'] ?? []); | ||
| if ([] === $allowedThemes) { | ||
| $allowedThemes = [$currentTheme]; | ||
| } |
| foreach ($xoopsConfig['theme_set_allowed'] as $theme) { | ||
| $select = new XoopsFormSelect('', 'xoops_theme_select', $currentTheme, $selectSize); | ||
| foreach ($allowedThemes as $theme) { | ||
| $select->addOption($theme, $theme); |
Comment on lines
691
to
693
| $themeSelect = '<img vspace="2" id="xoops_theme_img" src="' | ||
| . XOOPS_THEME_URL . '/' . $xoopsConfig['theme_set'] . '/shot.gif" ' | ||
| . XOOPS_THEME_URL . '/' . $currentTheme . '/shot.gif" ' | ||
| . ' alt="screenshot" width="' . (int)$options[1] . '" />' |
…k list
system/blocks/system_blocks.php (b_system_themes_show):
- Normalise theme names from $xoopsConfig at the top of the
function via a small lambda that strips anything outside
[A-Za-z0-9_-] — the alphabet XOOPS actually uses for theme
directory names. After normalisation $currentTheme falls back
to 'default' if it ended up empty, and $allowedThemes drops
empty entries and falls back to [$currentTheme] if all were
dropped.
- Make every downstream use the validated locals: the theme-
count display now reads count($allowedThemes) instead of
count($xoopsConfig['theme_set_allowed']) (which would raise a
count(null) TypeError on PHP 8 if the config row was missing
— the very situation this PR addresses), and $currentTheme
flows into the option label and screenshot URL safe by
construction.
- Belt-and-suspenders on the screenshot URL: build it with
rawurlencode($currentTheme) and htmlspecialchars() before
interpolation. For valid (validator-passing) input both are
no-ops, but the explicit encode + escape makes the output
site self-evidently safe and stays correct if the validator
is ever loosened.
Comment on lines
+688
to
+691
| $allowedThemes = array_values(array_filter( | ||
| array_map( | ||
| static fn($name): string => $normalizeTheme((string) $name), | ||
| (array) ($xoopsConfig['theme_set_allowed'] ?? []) |
Comment on lines
+695
to
+701
| if ([] === $allowedThemes) { | ||
| $allowedThemes = [$currentTheme]; | ||
| } | ||
|
|
||
| $selectSize = ($options[0] == 1) ? 1 : (int) $options[2]; | ||
| $select = new XoopsFormSelect('', 'xoops_theme_select', $xoopsConfig['theme_set'], $selectSize); | ||
| foreach ($xoopsConfig['theme_set_allowed'] as $theme) { | ||
| $select = new XoopsFormSelect('', 'xoops_theme_select', $currentTheme, $selectSize); | ||
| foreach ($allowedThemes as $theme) { |
… option Address PR XOOPS#44 follow-up review : system/blocks/system_blocks.php (b_system_themes_show): - theme_set_allowed is canonically a PHP array (kernel/configitem.php decodes 'array'-type configs via unserialize()). A direct override in mainfile.php / xoopsconfig.php could land as a pipe-separated string, which the previous (array) cast would have collapsed to a single mangled entry (e.g. 'default|xbootstrap5' → ['default|xbootstrap5'] → 'defaultxbootstrap5' after the normaliser stripped the pipe). Detect string explicitly, split on '|' and trim; treat any other non-array as an empty list and let the existing fallback take over. - After normalisation, if $currentTheme is not in the allowed list (partial or corrupted config), prepend it before array_unique. This keeps the rendered <select>'s `selected` value matched to one of its <option> values, and keeps the screenshot URL aligned with what the dropdown actually points at.
Comment on lines
+676
to
+682
| // Normalise each theme name to the filesystem-safe character set used | ||
| // for actual theme directories ([A-Za-z0-9_-]); anything else is | ||
| // treated as corruption and dropped. After normalisation, fall back to | ||
| // 'default' / [$currentTheme] as needed so the block always renders | ||
| // something useful instead of producing a PHP warning + blank output. | ||
| $normalizeTheme = static fn(string $name): string => (string) preg_replace('/[^A-Za-z0-9_-]/', '', $name); | ||
|
|
Address PR XOOPS#44 follow-up review: system/blocks/system_blocks.php (b_system_themes_show): The previous normaliser used preg_replace to STRIP characters outside [A-Za-z0-9_-], which silently rewrote valid XOOPS theme directory names containing dots — `my.theme` would be mangled to `mytheme`, pointing the screenshot URL and dropdown at a directory that doesn't exist. Switch to validate-and-reject: trim the name, reject anything with path separators (/, \), null bytes, '..' segments, a leading dot (hidden file), or characters outside [A-Za-z0-9_.-]. Names that fail validation become '' and get filtered out by the existing array_filter; names that pass are returned unchanged, so dotted theme directories survive. Adjusted the inline comments accordingly so they describe the current rule rather than the old strip-chars one.
| // string; anything else falls through to an empty list. | ||
| $rawAllowedThemes = $xoopsConfig['theme_set_allowed'] ?? []; | ||
| if (is_string($rawAllowedThemes)) { | ||
| $rawAllowedThemes = array_filter(array_map('trim', explode('|', $rawAllowedThemes))); |
Comment on lines
+676
to
+682
| // Validate (do NOT rewrite) each theme name against the directory-safe | ||
| // character set XOOPS theme directories actually use — letters, digits, | ||
| // underscore, hyphen, and dot — and reject names with path separators, | ||
| // null bytes, '..' segments, or a leading dot (hidden files). Names | ||
| // that fail validation are returned as '' and dropped by the | ||
| // array_filter below; anything that survives is the original name | ||
| // unmodified, so a valid `my.theme` directory keeps its dot. |
Address PR XOOPS#44 follow-up review: system/blocks/system_blocks.php (b_system_themes_show): - The pipe-split path used array_filter() without a callback, which drops any falsy entry — including a theme directory literally named "0". Replace with an explicit `'' !== $theme` callback so only empty strings are filtered. - Reword the inline comment on the validator. It claimed surviving names were returned "unmodified", but the validator calls trim() on its input. Updated to "returned trimmed but otherwise not rewritten" so the description matches the code (and trimming config input remains a sensible pre-filter).
Comment on lines
+700
to
+703
| $currentTheme = $normalizeTheme((string) ($xoopsConfig['theme_set'] ?? 'default')); | ||
| if ('' === $currentTheme) { | ||
| $currentTheme = 'default'; | ||
| } |
| $allowedThemes = array_values(array_filter( | ||
| array_map( | ||
| static fn($name): string => $normalizeTheme((string) $name), | ||
| $rawAllowedThemes |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/blocks/system_blocks.php`:
- Around line 720-724: The code maps $rawAllowedThemes through a callback that
casts items to string via (string) $name which can warn or throw on non-scalars;
update the pipeline to guard non-scalar entries before casting—either filter
$rawAllowedThemes with is_scalar(...) before array_map or change the mapping
callback (used with $normalizeTheme) to first check is_scalar($name) and skip or
return '' for non-scalars, then cast to string and normalize; ensure
$allowedThemes still uses array_filter/array_values to remove empty results and
reference the $rawAllowedThemes, $normalizeTheme and $allowedThemes symbols when
making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61c14ee-3e26-436e-b053-c9e71d4d3371
📒 Files selected for processing (1)
htdocs/modules/system/blocks/system_blocks.php
Address PR XOOPS#44 review: system/blocks/system_blocks.php (b_system_themes_show): Both reviewers flagged that the `(string) $value` casts on $xoopsConfig['theme_set'] and on each $rawAllowedThemes entry would TypeError on PHP 8 if the value happened to be an object without __toString(), and warn on arrays / "Array"-render them. This can happen when a corrupted xoops_config row or a mainfile.php override leaves a non-scalar in place. - The array_map callback over $rawAllowedThemes now wraps the cast in `is_scalar($name) ? (string) $name : ''`, so any non-scalar entry becomes '' and is dropped by the existing array_filter without producing PHP errors. - $currentTheme is derived from a pre-extracted $rawCurrentTheme guarded with is_string() — a single-value setting that isn't a string is treated as missing and falls back to 'default' via the existing empty-string check.
Comment on lines
+702
to
+703
| // corrupted override) directly to string, which would TypeError on | ||
| // an object without __toString or render an array as "Array". |
Comment on lines
+727
to
+729
| // Skip non-scalar entries (objects without __toString, resources, | ||
| // arrays, null) without casting — `(string)` on those would | ||
| // raise a TypeError on PHP 8. |
Address PR XOOPS#44 review: system/blocks/system_blocks.php (b_system_themes_show): - Pull the theme-config normalisation and validation out of b_system_themes_show() into two helper functions: _b_system_themes_normalize_name() — validates one directory name against the safe character set ([A-Za-z0-9_.-], no path separators / null bytes / .. / leading dot). _b_system_themes_resolve_config() — derives the safe ($currentTheme, $allowedThemes) tuple from $xoopsConfig, with all the defensive paths (string override unwinding, non-scalar skip, fallback to 'default', ensure current is in allowed, dedupe). The body of b_system_themes_show is now a single helper call plus the existing render code, dropping its cognitive complexity back below Sonar's 15-line threshold (php:S3776). - Reword the two casting comments. Object-to-string without __toString throws Error (not TypeError) on PHP 8, and array-to-string emits a "string conversion" Warning rather than throwing. The defensive code itself is unchanged.
Address PR XOOPS#44 review (Copilot): system/blocks/system_blocks.php: The previous docblock for systemNormalizeThemeName() said names with `'..' segments` are rejected. The implementation actually rejects ANY `..` substring (str_contains($name, '..')), so a name like `my..theme` is also rejected — which is the desired behaviour, but the docblock made it sound narrower. Reworded to match the implementation: "any `..` sequence (anywhere in the name, not just as a path segment, so e.g. `my..theme` is also rejected)". No code change. Note on the open CodeRabbit "share normalisation with runtime path" comment (common.php:266 et al.): tracked as follow-up issue XOOPS#45. Cross-cutting refactor would touch six files across include/ and class/ and is out of scope for this block-level hardening PR; the consolidation belongs in its own design pass and review cycle.
| * | ||
| * @internal Used by b_system_themes_show. | ||
| */ | ||
| function systemNormalizeThemeName(string $name): string |
Address PR XOOPS#44 review (Copilot): system/blocks/system_blocks.php: The function trims whitespace, then either returns '' (failure case) or the trimmed input (success). It does not rewrite the input into a canonical form, so "Validate" describes its behaviour more accurately than "Normalize". Renamed: systemNormalizeThemeName -> systemValidateThemeName Three call sites updated (the function declaration, the cross- reference in systemThemesResolveConfig's docblock, and two invocations in that helper's body). The function's own docblock already used "Validate"/"validation" throughout, so no further wording changes were required. Cross-cutting "share normalisation with runtime path" concern remains tracked in issue XOOPS#45 (deferred per the original PR scope).
Comment on lines
+673
to
+681
| if ( | ||
| '' === $name | ||
| || str_starts_with($name, '.') | ||
| || str_contains($name, '/') | ||
| || str_contains($name, '\\') | ||
| || str_contains($name, "\0") | ||
| || str_contains($name, '..') | ||
| || !preg_match('/^[A-Za-z0-9_.-]+$/', $name) | ||
| ) { |
system/blocks/system_blocks.php:
The previous validator enforced /^[A-Za-z0-9_.-]+$/ on every
theme name, which silently dropped legitimate XOOPS theme
directories that work elsewhere — XoopsLists::getDirListAsArray
accepts arbitrary directory names (excluding dot-prefixed) and
the theme factory's allowlist check is a plain in_array() with
no character validation. So a real `My Theme` (space) or a
non-ASCII directory name would render in the rest of XOOPS but
be rejected by this block's <select>.
Loosened the validator to enforce *path safety only*: empty
name, leading dot (hidden file), path separators (/, \), null
bytes, and `..` as a path segment are still rejected; the
alphabet regex is dropped. The `..` check now uses a
segment-aware regex (matches `..` only when bracketed by
separators or the string ends), so `my..theme` and `theme..`
pass — they're filesystem-valid even if unusual — while
`a/../b` and `..` itself stay rejected.
Output safety is moved to the proper layer:
- The XoopsFormSelect option *label* is now wrapped in
htmlspecialchars (the renderer escapes values but not
labels — earlier Copilot finding).
- The screenshot URL was already wrapped in rawurlencode +
htmlspecialchars; the comment is updated to describe why
that is now essential rather than belt-and-suspenders.
CodeRabbit's separate "share normalisation with runtime path"
comment remains tracked in issue XOOPS#45 (deferred per the original
PR scope).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
htdocs/modules/system/blocks/system_blocks.php:803
- The screenshot URL is now correctly encoded with
rawurlencode($currentTheme)for the initial<img src>, but theonchangehandler still callsshowImgSelected(), which concatenates the selected option value into the URL without any encoding (seehtdocs/include/xoops.js). For themes containing spaces or non-ASCII characters (explicitly allowed bysystemValidateThemeName()), the preview image will still break when changing the selection. Consider updating the onchange logic (orshowImgSelected) to URL-encode the theme directory component before assigningimgDom.src.
if ($options[0] == 1) {
// Encode + escape the screenshot URL. $currentTheme has passed
// the path-safety validator above, but the validator deliberately
// allows any character XOOPS theme discovery would accept —
// including spaces and non-ASCII — so rawurlencode is needed to
// produce a valid URL component, and htmlspecialchars is needed
// to make the result safe inside the src="..." attribute.
$themeShotUrl = htmlspecialchars(
XOOPS_THEME_URL . '/' . rawurlencode($currentTheme) . '/shot.gif',
ENT_QUOTES | ENT_SUBSTITUTE,
'UTF-8'
);
$themeSelect = '<img vspace="2" id="xoops_theme_img" src="'
. $themeShotUrl . '" '
. ' alt="screenshot" width="' . (int)$options[1] . '" />'
. '<br>';
$select->setExtra(' onchange="showImgSelected(\'xoops_theme_img\', \'xoops_theme_select\', \'themes\', \'/shot.gif\', '
. '\'' . XOOPS_URL . '\');" ');
Comment on lines
+778
to
+783
| // XoopsFormSelect escapes option *values* but not option *labels*; | ||
| // since the validator only enforces path safety, a legitimate | ||
| // theme directory may still contain HTML-significant characters | ||
| // (e.g. an `&` in the directory name). Escape the label here so | ||
| // it's safe in HTML body context. | ||
| $select->addOption($theme, htmlspecialchars($theme, ENT_QUOTES | ENT_HTML5, 'UTF-8')); |
…bel raw
system/blocks/system_blocks.php:
Replace the per-call htmlspecialchars() on the option label
with HTML-metacharacter rejection at the validator.
Background: XoopsFormSelect renderers diverge on label
escaping. Bootstrap3/4/5 and Legacy render labels raw (caller
escapes); Tailwind calls $this->esc($optName) (caller passes
raw). Pre-escaping in the caller works for the first four but
causes a visible double-escape under Tailwind (`&` -> `&`
in the rendered UI).
Fix at the boundary: extend systemValidateThemeName() to also
reject the five HTML metacharacters (< > & " '). Real theme
directory names don't contain those, but the rejection lets
the option value and label both be embedded raw — which works
uniformly under all five renderers without any per-renderer
branching at the caller. Path safety (no .. segments, no
separators, no leading dot, no null bytes) is unchanged.
The validator's docblock is updated to describe the two safety
classes (path AND HTML) and the deliberate decision not to
enforce a [A-Za-z0-9_.-] alphabet (which would have silently
dropped legitimate `My Theme` / `テーマ` directories).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
htdocs/modules/system/blocks/system_blocks.php:816
showImgSelected()builds the preview URL by concatenating the selected value directly (seehtdocs/include/xoops.js), butsystemValidateThemeName()allows characters that need URL-encoding in a path segment (e.g.,#or?). This can break the theme screenshot preview when switching themes (fragment/query truncation) even though the initial$themeShotUrlis correctly encoded. Either tighten the validator to reject URL-reserved characters that cannot be safely concatenated, or update the onchange handler / JS helper to URL-encode the selected value (e.g., viaencodeURIComponent).
$select->setExtra(' onchange="showImgSelected(\'xoops_theme_img\', \'xoops_theme_select\', \'themes\', \'/shot.gif\', '
. '\'' . XOOPS_URL . '\');" ');
Comment on lines
+688
to
+692
| function systemValidateThemeName(string $name): string | ||
| { | ||
| $name = trim($name); | ||
| if ( | ||
| '' === $name |
…mespace)
system/blocks/system_blocks.php:
Move the two top-level helpers introduced earlier in this PR —
`systemValidateThemeName()` and `systemThemesResolveConfig()` —
back inside `b_system_themes_show()` as local closures
($validateThemeName, $resolveConfig). The previous top-level
forms exposed two generic system* identifiers in the global
namespace alongside several existing system_*/system* helper
names elsewhere in the codebase, which Copilot flagged as
collision-prone.
Closures avoid the namespace concern AND keep cognitive
complexity low (Sonar treats closures as separate units, so
their bodies don't bubble back into b_system_themes_show's
S3776 score — which was the reason for extracting them in
the first place).
No behaviour change. Validator logic, resolver logic, and
call site are identical to the previous commit; only the
declaration shape moved.
Cross-cutting "share with runtime path" concern remains
tracked in issue XOOPS#45 (deferred per the PR's original scope).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
htdocs/modules/system/blocks/system_blocks.php:802
- The initial screenshot URL is now built with rawurlencode($currentTheme), but the onchange handler still calls showImgSelected(), which concatenates the selected option value into the URL without URL-encoding (see htdocs/include/xoops.js:195-199). For theme directory names containing spaces, non-ASCII, or reserved URL characters (e.g. '#', '?'), the preview image can break or become inconsistent after changing the selection. Consider updating the onchange handler (or showImgSelected) to URL-encode the selected value when constructing the image src so it matches the initial rendering logic.
$select->setExtra(' onchange="showImgSelected(\'xoops_theme_img\', \'xoops_theme_select\', \'themes\', \'/shot.gif\', '
. '\'' . XOOPS_URL . '\');" ');
include/xoops.js:
Wrap the selected option value in encodeURIComponent() before
concatenating it into the preview image URL. The block-side
validator now allows theme directory names with spaces and
non-ASCII characters; without encoding, switching the theme
via the dropdown produced URLs like
`/themes/My Theme/shot.gif` (literal space) — accepted by
most browsers but technically invalid and inconsistent with
the initial-render path which already uses rawurlencode().
encodeURIComponent is a no-op for [A-Za-z0-9_.-~] (RFC 3986
unreserved), so the change is safe for the other callers of
showImgSelected (avatar / rank / smiley pickers) that use
ASCII-only filenames.
The cross-cutting refactor that would also align
htdocs/include/common.php's runtime theme check with the same
normalisation remains tracked in issue XOOPS#45.
|
Comment on lines
+196
to
+202
| // encodeURIComponent on the selected value: the theme-switch | ||
| // selector now allows spaces and non-ASCII directory names, and | ||
| // avatar / rank / smiley selectors can also legitimately contain | ||
| // characters that need URL-encoding. encodeURIComponent is a | ||
| // no-op for [A-Za-z0-9_.-~] (RFC 3986 unreserved), so it does | ||
| // not change the URL for callers using ASCII-only filenames. | ||
| imgDom.src = xoopsUrl + "/" + imgDir + "/" + encodeURIComponent(selectDom.options[selectDom.selectedIndex].value) + extra; |
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.
Add a graceful-degradation path to the System theme-switch block.
system/blocks/system_blocks.php (b_system_themes_show):
Two
xoops_configrows are required for this block to render —theme_set(the active theme) andtheme_set_allowed(the listof permitted themes). If either is missing or unreadable —
e.g. mid-upgrade state, corrupted config row, custom installer
glitch — the previous code emitted PHP "Undefined index"
warnings and produced an empty select element, which in turn
blanked the page wherever the block was placed.
Summary by CodeRabbit