the /install folder is deleted after installation, so we can't call d…#25
the /install folder is deleted after installation, so we can't call d…#25mambax7 merged 9 commits intoXOOPS:masterfrom
Conversation
…bManager from there
WalkthroughReplace runtime Changes
Sequence Diagram(s)sequenceDiagram
participant Upgrade as Upgrade Script
participant DB as Database
participant Log as Logger
Upgrade->>DB: exec("INSERT INTO tplfile (cols...) VALUES (...)") rgba(64,128,192,0.5)
DB-->>Upgrade: success
Upgrade->>DB: getInsertId() rgba(64,128,192,0.5)
DB-->>Upgrade: tpl_id (n)
Upgrade->>DB: exec("INSERT INTO tplsource (tpl_id, tpl_source) VALUES (n, ?)") rgba(64,128,192,0.5)
alt tplsource insert success
DB-->>Upgrade: success
Upgrade->>Log: logEscaped("inserted tplfile tpl_id = n") rgba(128,192,64,0.5)
else tplsource insert fails
DB-->>Upgrade: error
Upgrade->>Log: logDbError(error, redacted SQL) rgba(192,64,64,0.5)
Upgrade->>DB: exec("DELETE FROM tplfile WHERE tpl_id = n") rgba(64,128,192,0.5)
DB-->>Upgrade: delete success/failure
alt delete failure
Upgrade->>Log: logEscaped("failed cleanup for tpl_id = n") rgba(192,64,64,0.5)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)Invalid entry in excludePaths: If the excluded path can sometimes exist, append (?) parameters: 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 #25 +/- ##
============================================
- Coverage 19.23% 18.00% -1.23%
- Complexity 7584 7803 +219
============================================
Files 621 665 +44
Lines 40085 42806 +2721
============================================
Hits 7709 7709
- Misses 32376 35097 +2721 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the 2.5.10 → 2.5.11 upgrade script to avoid relying on installer-only database utilities (since /install may not exist after installation), and instead performs the required template DB writes directly through the core DB layer.
Changes:
- Remove
Db_managerbootstrap that requiredXOOPS_ROOT_PATH . '/install/class/dbmanager.php'. - Replace
Db_manager->insert()calls with explicitINSERTstatements executed via$this->db->exec(). - Retrieve new
tplfileIDs via$this->db->getInsertId()for subsequenttplsourceinserts.
Comments suppressed due to low confidence (1)
upgrade/upd_2.5.10-to-2.5.11/index.php:1074
- On exec() failure here the log message omits the DB error details (and SQL), which makes upgrade failures hard to diagnose. Consider using the existing execOrFail() helper (or at least append
$this->db->error()/ errno) for consistent, actionable logging.
$sql = 'INSERT INTO ' . $this->db->prefix('tplsource')
. ' (tpl_id, tpl_source) VALUES (' . $tplId . ', ' . $this->db->quote($tplsource) . ')';
if (!$this->db->exec($sql)) {
$this->logs[] = sprintf('Failed to backfill tplsource row for %s', $fileName);
return false;
}
| . ')'; | ||
| if (!$this->db->exec($sql)) { | ||
| $this->logs[] = sprintf('Failed to insert tplfile row for %s', $fileName); | ||
|
|
||
| return false; | ||
| } | ||
| $newtplid = $this->db->getInsertId(); |
There was a problem hiding this comment.
These INSERTs use $this->db->exec($sql) directly; when they fail the logs don't include the DB error message, unlike other parts of this file that call execOrFail(). Please route these through execOrFail() (or include $this->db->error()/errno) so operators can see why the INSERT failed.
Use execOrFail() for tplfile/tplsource INSERTs in the 2.5.10-to-2.5.11 upgrader so failed template backfills include the SQL and database error in upgrade logs.
Keep DB error details for tplsource insert failures in the 2.5.10-to-2.5.11 upgrader, but avoid logging raw template source into the upgrade UI by letting execOrFail() accept a redacted SQL string
| if (!$this->execOrFail($sql)) { | ||
| $this->logs[] = sprintf('Failed to insert tplfile row for %s', $fileName); | ||
|
|
||
| return false; | ||
| } | ||
| $newtplid = $this->db->getInsertId(); |
There was a problem hiding this comment.
After inserting into tplfile, the code no longer validates that a new template id was actually generated. In uncommon MySQL configurations (e.g., NO_AUTO_VALUE_ON_ZERO), inserting 0 into an AUTO_INCREMENT column can succeed but yield an insert id of 0, causing the subsequent tplsource insert to use tpl_id = 0 (and creating an invalid/orphan template). Consider inserting NULL for the id (or explicitly listing columns) and/or checking that getInsertId() returns a positive integer before continuing, failing with a clear log entry otherwise.
Use an explicit tplfile column list with NULL for tpl_id in the 2.5.10-to-2.5.11 upgrader, and fail if getInsertId() does not return a positive template id before inserting tplsource
| $sql = 'INSERT INTO ' . $this->db->prefix('tplsource') | ||
| . ' (tpl_id, tpl_source) VALUES (' . $tplId . ', ' . $this->db->quote($tplsource) . ')'; | ||
| $logSql = 'INSERT INTO ' . $this->db->prefix('tplsource') |
There was a problem hiding this comment.
In this INSERT, $tplId is concatenated without an explicit integer cast. Even though it currently comes from a DB row, casting to (int) (as done for $newtplid below) avoids accidental injection/formatting issues and keeps the SQL-building consistent.
Cast tplId explicitly in tplsource backfill SQL and make execOrFail() write HTML-safe log messages so upgrade errors remain diagnosable without allowing raw SQL or DB error text to inject markup into the upgrade UI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
upgrade/upd_2.5.10-to-2.5.11/index.php (1)
1081-1114:⚠️ Potential issue | 🟠 MajorClean up orphan
tplfilerow if the follow-uptplsourceINSERT fails.
tplfile/tplsourceare MyISAM (seehtdocs/install/sql/mysql.structure.sql), so there is no transactional safety net. If the INSERT at Line 1094 succeeds but the companion INSERT at Line 1110 fails, an orphantplfilerow with no matchingtplsourceremains. Becausecheck_templates()/check_templatesadmin()useINNER JOIN tplsource, the orphan is invisible to subsequent check passes, yet theUNIQUE tpl_refid_module_set_file_typekey will then reject a retry — leaving the upgrade wedged.Consider deleting the just-created
tplfilerow on tplsource failure (best-effort rollback), so a re-run can succeed.🛡️ Suggested best-effort rollback
$sql = 'INSERT INTO ' . $this->db->prefix('tplsource') . ' (tpl_id, tpl_source) VALUES (' . (int) $newtplid . ', ' . $this->db->quote($tplsource) . ')'; $logSql = 'INSERT INTO ' . $this->db->prefix('tplsource') . ' (tpl_id, tpl_source) VALUES (' . (int) $newtplid . ', [tpl_source omitted])'; if (!$this->execOrFail($sql, $logSql)) { $this->logs[] = sprintf('Failed to insert tplsource row for %s', $fileName); + // Best-effort cleanup so re-running the upgrade isn't blocked by an orphan tplfile + // and by the UNIQUE (tpl_refid, tpl_module, tpl_tplset, tpl_file, tpl_type) key. + $cleanupSql = 'DELETE FROM ' . $this->db->prefix('tplfile') + . ' WHERE `tpl_id` = ' . (int) $newtplid . ' LIMIT 1'; + if (!$this->db->exec($cleanupSql)) { + $this->logs[] = sprintf( + 'Failed to remove orphan tplfile row %d for %s', + (int) $newtplid, + $fileName + ); + } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.10-to-2.5.11/index.php` around lines 1081 - 1114, When the second INSERT into tplsource fails, perform a best-effort rollback by deleting the just-created tplfile row (identified by $newtplid) so the UNIQUE constraint won't block a retry; after the execOrFail($sql, $logSql) failure branch where you currently push to $this->logs and return false, build and execute a DELETE FROM {$this->db->prefix('tplfile')} WHERE tpl_id = (int)$newtplid (use the db quote/prefix helpers as used elsewhere), log any error from that delete but do not fail the upgrade on the delete itself (best-effort), then return false as before.
🤖 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.5.10-to-2.5.11/index.php`:
- Around line 1120-1130: Add a PHPDoc block to the private method
execOrFail(string $sql, ?string $logSql = null): bool that documents the new
redaction parameter and escaping behavior: explain that $logSql is an optional
redacted SQL string (used to avoid logging large/sensitive payloads like
tpl_source), that the method appends an HTML-escaped message to $this->logs (so
callers must pass raw, unescaped text to avoid double-encoding), include `@param`
for both $sql and $logSql, `@return` bool, and note any exceptions or error
conditions (e.g., DB errors) with an `@throws` tag if the method can throw; keep
the description concise and place the docblock directly above execOrFail.
---
Outside diff comments:
In `@upgrade/upd_2.5.10-to-2.5.11/index.php`:
- Around line 1081-1114: When the second INSERT into tplsource fails, perform a
best-effort rollback by deleting the just-created tplfile row (identified by
$newtplid) so the UNIQUE constraint won't block a retry; after the
execOrFail($sql, $logSql) failure branch where you currently push to $this->logs
and return false, build and execute a DELETE FROM {$this->db->prefix('tplfile')}
WHERE tpl_id = (int)$newtplid (use the db quote/prefix helpers as used
elsewhere), log any error from that delete but do not fail the upgrade on the
delete itself (best-effort), then return false as before.
🪄 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: 483abf92-386c-4cef-8f36-7f77a974263d
📒 Files selected for processing (1)
upgrade/upd_2.5.10-to-2.5.11/index.php
Add best-effort cleanup for tplfile rows when the follow-up tplsource insert fails in the 2.5.10-to-2.5.11 upgrader, and document execOrFail() so the redacted logging and HTML- safe error output are explicit.
Route upgrade DB error logging through shared HTML-safe helpers, escape orphan-cleanup log messages, and use _UPGRADE_CHARSET for log output consistency.
There was a problem hiding this comment.
Actionable comments posted: 3
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.5.10-to-2.5.11/index.php (1)
1081-1130: 🧹 Nitpick | 🔵 TrivialConsider wrapping the
tplfile+tplsourcepair in a transaction.The current design relies on a best-effort
DELETEto undo the orphantplfilerow when the subsequenttplsourceinsert fails. That works for the happy-ish path, but it leaves two failure modes unhandled:
- If
tplsourceinsert fails and the cleanup DELETE also fails (connection drop, lock timeout, etc.), you're back to an orphan row that will break the next upgrade run'scheck_templates()count.- A process crash between the two inserts leaves the same orphan.
InnoDB (the XOOPS default) would let you begin/commit/rollback around this pair so the two rows either both land or neither does — strictly better than a compensating DELETE.
XoopsMySQLDatabaseexposestransactionStart()/transactionCommit()/transactionRollback()for this.Non-blocking — the best-effort cleanup is a reasonable interim — but worth considering as a follow-up for this upgrader.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.10-to-2.5.11/index.php` around lines 1081 - 1130, Wrap the two related inserts (the tplfile INSERT block that computes $newtplid and the subsequent tplsource INSERT using $newtplid) in a DB transaction using XoopsMySQLDatabase's transactionStart(), transactionCommit(), and transactionRollback() so both rows are atomic: call transactionStart() before executing the tplfile INSERT, if execOrFail(...) for tplfile succeeds continue and perform the tplsource execOrFail(...); on any failure call transactionRollback(), perform any logging via logEscaped()/logs[] as before, and return false; on success call transactionCommit() after both inserts. Ensure you still call getInsertId() only after the tplfile insert and handle failures by rolling back rather than relying on the best‑effort DELETE cleanup.
🤖 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.5.10-to-2.5.11/index.php`:
- Around line 1111-1127: Replace the inline htmlspecialchars call and direct
push to $this->logs[] for the tplsource insert failure with a call to the
existing helper $this->logEscaped(), i.e. instead of
htmlspecialchars(sprintf('Failed to insert tplsource row for %s', $fileName),
ENT_QUOTES, _UPGRADE_CHARSET) push the same sprintf message through
$this->logEscaped() so both the initial failure and the orphan tplfile cleanup
failure use the same escaping helper (refer to logEscaped(), $this->logs[],
$fileName and the tplsource/tplfile failure messages).
- Around line 1117-1127: The cleanup branch currently swallows DB error details
when $this->db->exec($cleanupSql) returns false and only calls
$this->logEscaped(...), so update the failure path to surface the database error
(e.g. call $this->db->execOrFail($cleanupSql) or, if execOrFail isn't
appropriate, call $this->logDbError() or include $this->db->error() in the log)
instead of the current silent logEscaped; change the code around
$this->db->exec($cleanupSql) and the logEscaped(...) call to route failures
through execOrFail() or to augment the existing log with $this->db->error()
(refer to the $this->db->exec($cleanupSql) call and the logEscaped(...)
invocation).
- Around line 1159-1167: Add concise PHPDoc blocks to the new private helpers
logDbError(string $sql, ?string $logSql = null): void and logEscaped(string
$message): void similar to the existing execOrFail docblock: for logDbError
document parameters ($sql and $logSql), that it formats the message using the
_DB_QUERY_ERROR format string and appends $this->db->error(), state the void
return and that it does not escape input (expects raw SQL text); for logEscaped
document the $message param is raw, unescaped text, that it HTML-escapes using
htmlspecialchars with ENT_QUOTES and _UPGRADE_CHARSET before storing, and
declare void return; include `@param` and `@return` tags in both blocks (no code
changes beyond comments).
---
Outside diff comments:
In `@upgrade/upd_2.5.10-to-2.5.11/index.php`:
- Around line 1081-1130: Wrap the two related inserts (the tplfile INSERT block
that computes $newtplid and the subsequent tplsource INSERT using $newtplid) in
a DB transaction using XoopsMySQLDatabase's transactionStart(),
transactionCommit(), and transactionRollback() so both rows are atomic: call
transactionStart() before executing the tplfile INSERT, if execOrFail(...) for
tplfile succeeds continue and perform the tplsource execOrFail(...); on any
failure call transactionRollback(), perform any logging via logEscaped()/logs[]
as before, and return false; on success call transactionCommit() after both
inserts. Ensure you still call getInsertId() only after the tplfile insert and
handle failures by rolling back rather than relying on the best‑effort DELETE
cleanup.
🪄 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: 1e827a78-ec60-42c5-81ee-04b401edebe0
📒 Files selected for processing (1)
upgrade/upd_2.5.10-to-2.5.11/index.php
| $cleanupSql = 'DELETE FROM ' . $this->db->prefix('tplfile') | ||
| . ' WHERE `tpl_id` = ' . (int) $newtplid . ' LIMIT 1'; | ||
| if (!$this->db->exec($cleanupSql)) { | ||
| $this->logEscaped( | ||
| sprintf( | ||
| 'Failed to remove orphan tplfile row %d for %s', | ||
| (int) $newtplid, |
There was a problem hiding this comment.
If the orphan tplfile cleanup DELETE fails, the current log message doesn't include the underlying DB error or the statement that failed, which makes troubleshooting hard. Consider executing the cleanup via execOrFail()/logDbError() (with optional redaction) so the upgrade log captures the database error details.
Funnel the $fileName-interpolated failure messages in the template backfill block through logEscaped() so every log entry gets consistent HTML escaping, and run the orphan-cleanup DELETE through execOrFail() so the underlying DB error is captured alongside the friendly breadcrumb. Adds PHPDoc on logDbError() and logEscaped() to document the raw-input / HTML-escape-on-append contract.
Apply the HTML-escaping helper to every raw $this->logs[] write in the 2.5.10-to-2.5.11 patch, not just the template-backfill branch. XoopsUpgrade::message() joins logs with <br> and renders them as raw HTML, so any message interpolating a filesystem path, table name, basename(), DB error string, or template filename was a latent HTML injection vector in the admin upgrade UI. Converts 15 raw writes across the patch (banner/modules migration errors, captcha and textsanitizer file-copy errors, Smarty cleanup delete errors, template-metadata errors) so the "all logs go through the helper" invariant now holds across the file.
|
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.5.10-to-2.5.11/index.php (1)
282-328:⚠️ Potential issue | 🟡 MinorAvoid logging absolute server paths.
Line 285 and Line 327 now HTML-escape the messages, but they still expose
$newPath/$sourcePathin the upgrade UI. Keep the diagnostic useful without leaking installation paths.🛡️ Proposed fix
- $this->logEscaped(sprintf('Captcha config directory %s was not created', $newPath)); + $this->logEscaped(sprintf( + 'Captcha config directory %s was not created', + basename(rtrim($newPath, '/\\')) + )); @@ - $this->logEscaped(sprintf('Failed to read source %s', $sourcePath)); + $this->logEscaped(sprintf('Failed to read source %s', basename(rtrim($sourcePath, '/\\'))));As per coding guidelines: "Use
basename()in error messages to avoid exposing server paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.10-to-2.5.11/index.php` around lines 282 - 328, The log messages in makeDirectory() and apply_captchadata() expose full filesystem paths ($newPath and $sourcePath); change those logEscaped calls to log only the basename (use basename($newPath) and basename($sourcePath) or basename($sourcePath) for the file) so diagnostics remain useful without leaking installation paths—update the logEscaped in makeDirectory(), the failed copy message in copyFile() already uses basename(), and the logEscaped call that reports "Failed to read source" inside apply_captchadata() to use basename($sourcePath) instead of the full $sourcePath.
🤖 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.5.10-to-2.5.11/index.php`:
- Around line 282-328: The log messages in makeDirectory() and
apply_captchadata() expose full filesystem paths ($newPath and $sourcePath);
change those logEscaped calls to log only the basename (use basename($newPath)
and basename($sourcePath) or basename($sourcePath) for the file) so diagnostics
remain useful without leaking installation paths—update the logEscaped in
makeDirectory(), the failed copy message in copyFile() already uses basename(),
and the logEscaped call that reports "Failed to read source" inside
apply_captchadata() to use basename($sourcePath) instead of the full
$sourcePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 350a6df5-ac71-4dba-8c3a-c6e2d393fedd
📒 Files selected for processing (1)
upgrade/upd_2.5.10-to-2.5.11/index.php
…bManager from there
Summary by CodeRabbit
Refactor
Bug Fixes