Skip to content

fix(upgrade): skip stale patch directories and fix System module update URL #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mambax7 merged 2 commits into from
Apr 14, 2026
+37 −12
Merged

fix(upgrade): skip stale patch directories and fix System module update URL #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5aa2b4a
fix(upgrade): skip stale patch directories and fix System module upda…
mambax7 Apr 14, 2026
642321f
fix(upgrade): emit visible warnings for skipped patches and guard Db_…
mambax7 Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion upgrade/class/Xoops/Upgrade/UpgradeControl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,24 @@ public function buildUpgradeQueue(): bool

foreach ($dirs as $dir) {
if (str_contains($dir, '-to-')) {
$className = include $upgradeRoot . DIRECTORY_SEPARATOR . $dir . DIRECTORY_SEPARATOR . 'index.php';
$patchFile = $upgradeRoot . DIRECTORY_SEPARATOR . $dir . DIRECTORY_SEPARATOR . 'index.php';
if (!file_exists($patchFile)) {
continue;
}
try {
$className = include $patchFile;
Comment on lines +232 to +236
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

file_exists($patchFile) doesn’t guarantee this is an includable file (it may be a directory or unreadable), and include failures often only raise warnings (not Throwable) and return false, so this can still emit warnings and/or silently skip without logging. Consider using is_file() + is_readable() (or similar) and explicitly handling an include return value of false so broken patch entries are consistently skipped without PHP warnings.

Copilot uses AI. Check for mistakes.
} catch (\Throwable $e) {
// Stale directories from a previous version (e.g.
// upd_2.5.11-to-2.5.12 from the pre-rename beta cycle) can
// fail to load because they reference classes that no longer
// exist. Emit a visible warning and skip — crashing the
// entire upgrade is worse than skipping one broken patch.
trigger_error(
sprintf('Upgrade patch %s could not be loaded: %s', $dir, $e->getMessage()),
E_USER_WARNING
);
continue;
Comment on lines +231 to +247
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 14, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Don't silently drop broken shipped patches.

Line 233 and Line 245 continue before $results[$dir] is populated. That means a real upgrade directory with a missing index.php or a throwing index.php simply disappears from $upgradeQueue, so countUpgradeQueue() can reach zero and the wizard can advance without running a required migration.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@upgrade/class/Xoops/Upgrade/UpgradeControl.php` around lines 231 - 245, The
code in UpgradeControl:: (around $patchFile/$className handling) currently
continues on missing or throwing index.php without populating $results[$dir],
which lets $upgradeQueue shrink incorrectly; modify the branches where
file_exists($patchFile) is false and where the try/catch catches \Throwable to
assign a meaningful entry into $results[$dir] (e.g. status => 'skipped' or
'error' and include a reason/message) before continuing, and also add to
$this->logs as currently done; ensure $results and the upgrade queue logic (used
by countUpgradeQueue()) see these skipped/errored patches so the wizard does not
advance prematurely.

}
Comment on lines +237 to +248
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

The PR description says stale patch directories should be “logged and skipped”, but this code path only appends a message to $this->logs (which isn’t surfaced anywhere in the upgrade UI or written to PHP error logs). After fixing the missing property, consider also exposing these messages (e.g. via the view model / template) or emitting a trigger_error(..., E_USER_WARNING) so admins can actually see why a patch directory was skipped.

Copilot uses AI. Check for mistakes.
if (is_string($className) && class_exists($className)) {
$upg = $this->createPatch($className);
$results[$dir] = $upg->isApplied();
Expand Down
2 changes: 1 addition & 1 deletion upgrade/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@
}
if (0 === $upgradeControl->countUpgradeQueue()) {
echo $upgradeControl->oneButtonContinueForm(
XOOPS_URL . '/modules/system/admin.php?fct=modulesadmin&op=update&module=system',
XOOPS_URL . '/modules/system/admin.php?fct=modulesadmin&op=update&module=system',
[],
);
} else {
Expand Down
24 changes: 15 additions & 9 deletions upgrade/upd-2.4.x-to-2.5.0/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
use Xoops\Upgrade\XoopsUpgrade;
use Xoops\Upgrade\UpgradeControl;

require_once __DIR__ . '/dbmanager.php';
if (!class_exists('Db_manager', false)) {
require_once __DIR__ . '/dbmanager.php';
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Upgrade from 2.4.x to 2.5.0
Expand Down Expand Up @@ -79,11 +81,13 @@ public function check_templates(): bool
*/
public function apply_config(): bool
{
if (!file_exists($this->dbmanagerFile)) {
$this->logs[] = 'Database manager file not found: ' . $this->dbmanagerFile;
return false;
if (!class_exists('Db_manager', false)) {
if (!file_exists($this->dbmanagerFile)) {
$this->logs[] = 'Database manager file not found: ' . $this->dbmanagerFile;
return false;
}
require_once $this->dbmanagerFile;
}
require_once $this->dbmanagerFile;
$dbm = new Db_manager();

$sql = 'SELECT conf_id FROM `' . $this->db->prefix('config') . "` WHERE `conf_name` IN ('cpanel')";
Expand Down Expand Up @@ -197,11 +201,13 @@ public function apply_templates(): bool
return false;
}

if (!file_exists($this->dbmanagerFile)) {
$this->logs[] = 'Database manager file not found: ' . $this->dbmanagerFile;
return false;
if (!class_exists('Db_manager', false)) {
if (!file_exists($this->dbmanagerFile)) {
$this->logs[] = 'Database manager file not found: ' . $this->dbmanagerFile;
return false;
}
require_once $this->dbmanagerFile;
}
require_once $this->dbmanagerFile;
$dbm = new Db_manager();
$time = time();
foreach ($modversion['templates'] as $tplfile) {
Expand Down
4 changes: 3 additions & 1 deletion upgrade/upd_2.5.10-to-2.5.11/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
*/

require_once XOOPS_ROOT_PATH . '/install/class/dbmanager.php';
if (!class_exists('Db_manager', false)) {
require_once XOOPS_ROOT_PATH . '/install/class/dbmanager.php';
}

use Xmf\Database\Tables;
use Xoops\Upgrade\XoopsUpgrade;
Expand Down
Loading