Setting utf8mb4 as a default#30
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTwo distribution config files set Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #30 +/- ##
============================================
- Coverage 19.23% 17.96% -1.28%
- Complexity 7584 7803 +219
============================================
Files 621 665 +44
Lines 40085 43181 +3096
============================================
+ Hits 7709 7756 +47
- Misses 32376 35425 +3049 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates configuration template defaults to prefer utf8mb4 as the database connection charset, aligning generated config files with modern MySQL/MariaDB Unicode support expectations.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in the 2.0.18→2.3.0 upgrademainfile.dist.phptemplate. - Set
XOOPS_DB_CHARSETdefault toutf8mb4inxoops_data/data/secure.dist.php.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Changes upgrade template default charset to utf8mb4 (but may not take effect due to rewrite logic). |
| htdocs/xoops_data/data/secure.dist.php | Changes secure.php template default charset to utf8mb4. |
| die('Restricted Access'); | ||
| } | ||
| define('XOOPS_DB_CHARSET', ''); | ||
| define('XOOPS_DB_CHARSET', 'utf8mb4'); |
There was a problem hiding this comment.
Good catch. The issue was in Upgrade_230::write_mainfile(), not in the template itself.
For string defines, the rewrite logic fell back to '' when there was no submitted value and no existing constant. That meant XOOPS_DB_CHARSET from mainfile.dist.php could be rewritten
back to empty during upgrades unless a collation was explicitly selected.
I patched the upgrader so it now preserves the template default as the final fallback. In the same method I also fixed the numeric branch to use submitted values correctly instead of
reading constants.
This keeps the new utf8mb4 default effective for upgraded installs as well, not just fresh/generated configs.
![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)
upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php (1)
34-36: 🧹 Nitpick | 🔵 TrivialDirect superglobal access for HTTPS detection.
Lines 34-36 access
$_SERVER['HTTPS']directly for protocol detection. While this is acceptable for server environment variables (not user input), consider that:
- Modern setups behind reverse proxies may require checking
X-Forwarded-Protoheader- The check
$_SERVER['HTTPS'] === 'on'works for Apache but may not cover all server configurationsThis is acceptable for a distribution template but may need environment-specific adjustment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php` around lines 34 - 36, The current protocol detection uses direct superglobal access ($_SERVER['HTTPS']) and sets $http based only on === 'on'; update it to robustly handle reverse proxies and varied server values by creating or using a helper (e.g., detect_protocol or similar) that checks X-Forwarded-Proto (HTTP_X_FORWARDED_PROTO) first, then falls back to $_SERVER['HTTPS'] (accepting values like 'on' or '1'), and finally $_SERVER['REQUEST_SCHEME'] or default to 'http'; replace the inline $_SERVER['HTTPS'] check and assignment to $http with a call to that helper so $http reliably becomes either 'https://' or 'http://'.
🤖 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 `@upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php`:
- Around line 34-36: The current protocol detection uses direct superglobal
access ($_SERVER['HTTPS']) and sets $http based only on === 'on'; update it to
robustly handle reverse proxies and varied server values by creating or using a
helper (e.g., detect_protocol or similar) that checks X-Forwarded-Proto
(HTTP_X_FORWARDED_PROTO) first, then falls back to $_SERVER['HTTPS'] (accepting
values like 'on' or '1'), and finally $_SERVER['REQUEST_SCHEME'] or default to
'http'; replace the inline $_SERVER['HTTPS'] check and assignment to $http with
a call to that helper so $http reliably becomes either 'https://' or 'http://'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d6f0f73-caee-4ee8-be1b-069af796ac2e
📒 Files selected for processing (2)
htdocs/xoops_data/data/secure.dist.phpupgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php
Keep template defaults when Upgrade_230 rewrites mainfile.php and no existing constant or submitted value is available. This fixes the XOOPS_DB_CHARSET upgrade path so the new utf8mb4 default from mainfile.dist.php is not overwritten with an empty string on older installs. Also correct the numeric define branch to use submitted values instead of reading constants.
There was a problem hiding this comment.
Pull request overview
Updates XOOPS distribution/upgrade configuration templates to default the DB charset to utf8mb4, and also adjusts the xbootstrap5 theme’s JS loading/init behavior.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in dist/upgrade config templates. - Move core JS includes (jQuery + Bootstrap bundle) into the theme
<head>and adjust footer script blocks. - Refactor carousel initialization in
xbootstrap5/js/js.jsto support Bootstrap 5’s JS API with a fallback for jQuery plugins.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Defaults XOOPS_DB_CHARSET to utf8mb4 for that upgrade template. |
| htdocs/xoops_data/data/secure.dist.php | Defaults XOOPS_DB_CHARSET to utf8mb4 for generated secure.php. |
| htdocs/themes/xbootstrap5/theme.tpl | Changes script placement/order and removes duplicate module header injection in footer area. |
| htdocs/themes/xbootstrap5/js/js.js | Adds a shared carousel init helper and reuses it for carousel/slider setup. |
| <!-- 5. XOOPS module header (injects module-specific JS if needed) --> | ||
| <{$xoops_module_header}> | ||
|
|
||
| <!-- 6. Inline scripts (AFTER all libraries) --> |
This reverts commit c76e8d7.
Keep template defaults when Upgrade_230 rewrites mainfile.php and no existing constant or submitted value is available. This preserves the utf8mb4 default from the upgrade template instead of rewriting XOOPS_DB_CHARSET back to an empty string on older installs. It also fixes the numeric define branch to use submitted values correctly.
There was a problem hiding this comment.
Pull request overview
This PR updates the default database character set used in configuration templates to utf8mb4, aiming to standardize UTF-8 handling (including 4-byte characters) across installs and upgrades.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in the upgrademainfile.dist.phptemplate. - Adjust
Upgrade_230::write_mainfile()fallback logic to preserve template defaults when no submitted/existing value is available. - Set
XOOPS_DB_CHARSETdefault toutf8mb4insecure.dist.php.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Changes the upgrade template default DB charset to utf8mb4. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Updates upgrade mainfile rewrite logic to preserve template defaults / use submitted values. |
| htdocs/xoops_data/data/secure.dist.php | Changes the secure config template default DB charset to utf8mb4. |
| $val = isset($vars[$matches[3]]) ? (string) $vars[$matches[3]] : (defined($matches[3]) ? (string) constant($matches[3]) : $matches[4]); | ||
| $lines[$ln] = preg_replace("/(define\()([\"'])(XOOPS_[^\"']+)\\2,\s*([0-9]+)\s*\)/", "define('" . $matches[3] . "', " . $val . ' )', $lines[$ln]); | ||
| } elseif (preg_match("/(define\()([\"'])(XOOPS_[^\"']+)\\2,\s*([\"'])([^\"']*?)\\4\s*\)/", $lines[$ln], $matches)) { | ||
| $val = isset($vars[$matches[3]]) ? (string) $vars[$matches[3]] : (defined($matches[3]) ? (string) constant($matches[3]) : ''); | ||
| $val = isset($vars[$matches[3]]) ? (string) $vars[$matches[3]] : (defined($matches[3]) ? (string) constant($matches[3]) : $matches[5]); | ||
| $lines[$ln] = preg_replace("/(define\()([\"'])(XOOPS_[^\"']+)\\2,\s*([\"'])(.*?)\\4\s*\)/", "define('" . $matches[3] . "', '" . $val . "' )", $lines[$ln]); |
There was a problem hiding this comment.
Good point. I updated write_mainfile() to align with the installer/upgrader config writers:
- numeric replacements are now normalized before being written into numeric defines
- string replacements are now escaped before the regex replacement is applied
Escape string replacements and normalize numeric replacements before writing mainfile.php during the 2.0.18 to 2.3.0 upgrade. This aligns the upgrader with the installer configuration writer and avoids emitting invalid or unsafe define values from submitted settings.
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 `@upgrade/upd-2.0.18-to-2.3.0/index.php`:
- Around line 426-432: The upgrade script currently escapes configuration values
using addslashes() and then injects them via preg_replace replacement, causing
invalid PHP string literals and accidental backslashes or $ interpolation;
update the logic that builds $val and writes to $lines[$ln] (the block using
addslashes($val) and preg_replace(... define(...))) to instead use
var_export($val, true) to produce a correct PHP literal and switch to
preg_replace_callback() (or a callback-based preg_replace) to compute the
replacement string so no $-interpolation occurs; specifically replace the
addslashes + str_replace + preg_replace pattern with computing $export =
var_export($val, true) and using preg_replace_callback on the define regex to
return "define('...'," . $export . " )" to safely encode the string value.
🪄 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: 9d6c2d0b-8be3-42f8-947b-c49229ff8987
📒 Files selected for processing (1)
upgrade/upd-2.0.18-to-2.3.0/index.php
There was a problem hiding this comment.
Pull request overview
This PR updates XOOPS configuration templates and the 2.0.18→2.3.0 upgrader so upgraded sites inherit utf8mb4 as the default DB charset and config values are rewritten more reliably during the upgrade.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in upgrade and secure config templates. - Update
Upgrade_230::write_mainfile()to preserve template defaults when no value is submitted/defined, normalize numeric define values, and escape string values before regex replacement.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Sets upgrade template default charset to utf8mb4. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Improves upgrade-time mainfile rewriting (fallbacks + escaping/normalization). |
| htdocs/xoops_data/data/secure.dist.php | Sets secure config template default charset to utf8mb4. |
| $val = isset($vars[$matches[3]]) ? (string) $vars[$matches[3]] : (defined($matches[3]) ? (string) constant($matches[3]) : $matches[5]); | ||
| $val = str_replace( | ||
| ['$', "\r", "\n"], | ||
| ['\\$', '\r', '\n'], |
Use var_export() and preg_replace_callback() when rewriting string define values in Upgrade_230::write_mainfile(). This avoids replacement-string escaping issues and preserves valid PHP literals for configuration values written during the 2.0.18 to 2.3.0 upgrade.
There was a problem hiding this comment.
Pull request overview
This PR updates configuration templates and the 2.0.18→2.3.0 upgrade writer so upgraded installs reliably keep the intended default database charset (utf8mb4) and config values are written more safely.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in upgrade and secure dist templates. - Update
Upgrade_230::write_mainfile()to preserve template defaults when no submitted/existing value is present. - Emit string define values via
var_export()during rewrite to avoid malformed PHP from unescaped values.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Sets XOOPS_DB_CHARSET template default to utf8mb4 for this upgrade step. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Improves config rewrite logic to preserve template defaults and safely serialize string values. |
| htdocs/xoops_data/data/secure.dist.php | Sets XOOPS_DB_CHARSET default to utf8mb4 for generated secure.php. |
| $valSource = isset($vars[$matches[3]]) ? $vars[$matches[3]] : (defined($matches[3]) ? constant($matches[3]) : $matches[4]); | ||
| $val = is_numeric((string) $valSource) ? (string) ((int) $valSource) : $matches[4]; | ||
| $lines[$ln] = preg_replace("/(define\()([\"'])(XOOPS_[^\"']+)\\2,\s*([0-9]+)\s*\)/", "define('" . $matches[3] . "', " . $val . ' )', $lines[$ln]); |
Refine Upgrade_230::write_mainfile() so define value rewriting is easier to follow and safer during the 2.0.18 to 2.3.0 upgrade. Replace nested ternaries with explicit branches, emit string literals with var_export() via preg_replace_callback(), and only accept integer-shaped numeric values before rewriting numeric defines.
There was a problem hiding this comment.
Pull request overview
Updates XOOPS configuration templates and the 2.0.18→2.3.0 upgrader so that utf8mb4 becomes the effective default DB charset (including during upgrades), and upgrade-time config rewrites better preserve template defaults while emitting safer PHP literals.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in upgrade and secure config dist templates. - Adjust
Upgrade_230::write_mainfile()to preserve template fallback values and emit string defines viavar_export().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Sets the upgrade template default DB charset to utf8mb4. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Improves upgrade-time rewriting of define() values (fallback handling + safer string literal emission). |
| htdocs/xoops_data/data/secure.dist.php | Sets secure.php template default DB charset to utf8mb4. |
| @@ -419,11 +419,32 @@ public function write_mainfile($vars) | |||
| $lines = file($file); | |||
Handle unreadable mainfile.dist.php explicitly in Upgrade_230::write_mainfile() so the upgrader fails cleanly instead of falling through into noisier warnings.
There was a problem hiding this comment.
Pull request overview
This PR updates XOOPS upgrade/install configuration defaults to use utf8mb4 and improves the 2.0.18→2.3.0 upgrade config writer so template defaults are preserved and emitted safely.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in upgrade and secure dist templates. - Update
Upgrade_230::write_mainfile()to preserve template fallback values, normalize numeric defines, and emit string defines safely usingvar_export()+preg_replace_callback(). - Add handling for failure to read
mainfile.dist.php.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Changes upgrade template default DB charset to utf8mb4. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Makes write_mainfile() preserve defaults and safely write numeric/string define values. |
| htdocs/xoops_data/data/secure.dist.php | Changes secure dist default DB charset to utf8mb4. |
|
|
||
| $lines = file($file); | ||
| if (false === $lines) { | ||
| echo ERR_COULD_NOT_WRITE_MAINFILE; |
Use the upgrade file-access error when mainfile.dist.php cannot be read in Upgrade_230::write_mainfile() instead of reporting a mainfile write failure. This makes upgrader diagnostics clearer when the template file is missing or unreadable.
|
There was a problem hiding this comment.
Pull request overview
This PR updates XOOPS’ configuration templates and upgrade rewrite logic so upgraded and freshly generated configurations default to utf8mb4, ensuring consistent UTF-8 support across installations.
Changes:
- Set
XOOPS_DB_CHARSETdefault toutf8mb4in the upgrademainfile.dist.phptemplate. - Set
XOOPS_DB_CHARSETdefault toutf8mb4insecure.dist.php. - Improve
Upgrade_230::write_mainfile()to preserve template defaults when no submitted/existing value exists and to normalize/escape rewritten define values more safely.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| upgrade/upd-2.0.18-to-2.3.0/mainfile.dist.php | Changes the upgrade template default DB charset to utf8mb4. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Makes upgrader mainfile rewriting preserve template defaults and write safer normalized values. |
| htdocs/xoops_data/data/secure.dist.php | Changes the secure template default DB charset to utf8mb4. |
Summary by CodeRabbit
Chores
Bug Fixes