Open
Conversation
…t to test thoroughly
…omposer, but that is not yet implemented for HTMLPurifier
Co-authored-by: Raimondas Rimkevičius <github@mekdrop.name>
…unused dev dependencies, and clean up scripts
no license info than the wrong info
the autoloader in the vendor folder.
Contributor
Review Summary by QodoComposer integration with vendor directory security relocation
WalkthroughsDescription• Implements Composer-based autoloading for ImpressCMS libraries, replacing bundled PHP libraries with Composer dependencies • Moves HTMLPurifier, SimplePie, and WideImage libraries to Composer management, removing 300+ bundled library files • Adds moveVendorToTrust() functionality to securely relocate Composer vendor directory from web root to trust path during installation • Integrates new vendor move installation wizard page with comprehensive error handling and user feedback • Updates autoloader chain in icms.php and installation bootstrap to support both trust path and root path vendor locations • Implements Composer-based module and theme discovery via InstalledVersions instead of filesystem scanning • Adds composer.json with PSR-4/PSR-0 autoloading configuration for Icms\* and icms_* namespaces • Standardizes codebase to PSR-12 formatting: array syntax [], double quotes, proper brace placement • Updates SimplePie and HTMLPurifier references to use namespaced class constants from Composer packages • Increments database version to 49 and system version to 2.1.0 beta • Fixes minor bugs: undefined array index in theme blocks, array indentation inconsistencies Diagramflowchart LR
A["Bundled Libraries<br/>htmlpurifier, simplepie,<br/>wideimage"] -->|"Migrate to Composer"| B["composer.json<br/>Dependencies"]
B -->|"Install via Composer"| C["vendor/ Directory<br/>in Web Root"]
C -->|"moveVendorToTrust()"| D["Trust Path<br/>vendor/"]
E["Autoloader Chain"] -->|"Load from"| D
F["Module/Theme Discovery"] -->|"Query via"| B
G["Installation Wizard"] -->|"Execute"| C
G -->|"Trigger"| D
File Changes1. htdocs/libraries/icms/core/Filesystem.php
|
Contributor
Code Review by Qodo
1. $_icms_autoload not verified
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+49
to
+56
| $_icms_autoload_from_trustpath = false; | ||
| if (file_exists(ICMS_TRUST_PATH . "/vendor/autoload.php")) { | ||
| $_icms_autoload = ICMS_TRUST_PATH . "/vendor/autoload.php"; | ||
| $_icms_autoload_from_trustpath = true; | ||
| } else { | ||
| $_icms_autoload = ICMS_ROOT_PATH . "/vendor/autoload.php"; | ||
| } | ||
| require_once $_icms_autoload; |
Contributor
There was a problem hiding this comment.
1. $_icms_autoload not verified 📘 Rule violation ⛯ Reliability
require_once $_icms_autoload is executed without verifying that the selected autoload path exists, which can trigger a fatal error when neither trust-path nor web-root vendor/autoload.php is present. This violates the requirement to handle dependency-loading failure points gracefully with actionable context.
Agent Prompt
## Issue description
`htdocs/include/common.php` loads Composer autoloading via `require_once $_icms_autoload` without verifying the resolved path exists, which can cause an unhandled fatal error when `vendor/autoload.php` is missing.
## Issue Context
This is core bootstrap code; failures here should be handled with clear, actionable context rather than an opaque fatal include error.
## Fix Focus Areas
- htdocs/include/common.php[49-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+137
to
+205
| if ( | ||
| empty($_SERVER["SERVER_NAME"]) || | ||
| substr(PHP_SAPI, 0, 3) == "cli" || | ||
| $GLOBALS["icmsConfigMultilang"] | ||
| ) { | ||
| $icmsConfig["gzip_compression"] = 0; | ||
| } | ||
|
|
||
| if ($icmsConfig['gzip_compression'] == 1 && extension_loaded('zlib') && !ini_get('zlib.output_compression')) { | ||
| ini_set('zlib.output_compression', TRUE); | ||
| if (ini_get('zlib.output_compression_level') < 0) { | ||
| ini_set('zlib.output_compression_level', 6); | ||
| if ( | ||
| $icmsConfig["gzip_compression"] == 1 && | ||
| extension_loaded("zlib") && | ||
| !ini_get("zlib.output_compression") | ||
| ) { | ||
| ini_set("zlib.output_compression", true); | ||
| if (ini_get("zlib.output_compression_level") < 0) { | ||
| ini_set("zlib.output_compression_level", 6); | ||
| } | ||
| if (!zlib_get_coding_type()) { | ||
| ini_set('zlib.output_compression', FALSE); | ||
| ob_start('ob_gzhandler'); | ||
| ini_set("zlib.output_compression", false); | ||
| ob_start("ob_gzhandler"); | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * This address the strict compliance for PHP 5.3/5.4, but the rest of our timezone handling | ||
| * can be improved beyond this. ~skenow | ||
| */ | ||
| date_default_timezone_set(timezone_name_from_abbr("", $icmsConfig['default_TZ'] * 3600, 0)); | ||
| date_default_timezone_set( | ||
| timezone_name_from_abbr("", $icmsConfig["default_TZ"] * 3600, 0), | ||
| ); | ||
|
|
||
| // -- Include site-wide lang file | ||
| icms_loadLanguageFile('core', 'global'); | ||
| icms_loadLanguageFile('core', 'core'); | ||
| icms_loadLanguageFile('system', 'common'); | ||
| @define('_GLOBAL_LEFT', @_ADM_USE_RTL == 1 ? 'right' : 'left'); | ||
| @define('_GLOBAL_RIGHT', @_ADM_USE_RTL == 1 ? 'left' : 'right'); | ||
| icms_loadLanguageFile("core", "global"); | ||
| icms_loadLanguageFile("core", "core"); | ||
| icms_loadLanguageFile("system", "common"); | ||
| @define("_GLOBAL_LEFT", @_ADM_USE_RTL == 1 ? "right" : "left"); | ||
| @define("_GLOBAL_RIGHT", @_ADM_USE_RTL == 1 ? "left" : "right"); | ||
|
|
||
| // -- Include page-specific lang file | ||
| if (isset($xoopsOption['pagetype']) && FALSE === strpos($xoopsOption['pagetype'], '.')) { | ||
| icms_loadLanguageFile('core', $xoopsOption['pagetype']); | ||
| if ( | ||
| isset($xoopsOption["pagetype"]) && | ||
| false === strpos($xoopsOption["pagetype"], ".") | ||
| ) { | ||
| icms_loadLanguageFile("core", $xoopsOption["pagetype"]); | ||
| } | ||
|
|
||
| defined("XOOPS_USE_MULTIBYTES") or define("XOOPS_USE_MULTIBYTES", 0); | ||
|
|
||
| if (!empty($_POST['xoops_theme_select']) && in_array($_POST['xoops_theme_select'], $icmsConfig['theme_set_allowed'])) { | ||
| $icmsConfig['theme_set'] = $_POST['xoops_theme_select']; | ||
| $_SESSION['xoopsUserTheme'] = $_POST['xoops_theme_select']; | ||
| } elseif (!empty($_POST['theme_select']) && in_array($_POST['theme_select'], $icmsConfig['theme_set_allowed'])) { | ||
| $icmsConfig['theme_set'] = $_POST['theme_select']; | ||
| $_SESSION['xoopsUserTheme'] = $_POST['theme_select']; | ||
| } elseif (!empty($_SESSION['xoopsUserTheme']) && in_array($_SESSION['xoopsUserTheme'], $icmsConfig['theme_set_allowed'])) { | ||
| $icmsConfig['theme_set'] = $_SESSION['xoopsUserTheme']; | ||
| if ( | ||
| !empty($_POST["xoops_theme_select"]) && | ||
| in_array($_POST["xoops_theme_select"], $icmsConfig["theme_set_allowed"]) | ||
| ) { | ||
| $icmsConfig["theme_set"] = $_POST["xoops_theme_select"]; | ||
| $_SESSION["xoopsUserTheme"] = $_POST["xoops_theme_select"]; | ||
| } elseif ( | ||
| !empty($_POST["theme_select"]) && | ||
| in_array($_POST["theme_select"], $icmsConfig["theme_set_allowed"]) | ||
| ) { | ||
| $icmsConfig["theme_set"] = $_POST["theme_select"]; | ||
| $_SESSION["xoopsUserTheme"] = $_POST["theme_select"]; | ||
| } elseif ( | ||
| !empty($_SESSION["xoopsUserTheme"]) && | ||
| in_array($_SESSION["xoopsUserTheme"], $icmsConfig["theme_set_allowed"]) | ||
| ) { | ||
| $icmsConfig["theme_set"] = $_SESSION["xoopsUserTheme"]; | ||
| } | ||
|
|
||
| if ($icmsConfig['closesite'] == 1) { | ||
| include ICMS_INCLUDE_PATH . '/site-closed.php'; | ||
| if ($icmsConfig["closesite"] == 1) { | ||
| include ICMS_INCLUDE_PATH . "/site-closed.php"; |
Contributor
There was a problem hiding this comment.
2. common.php uses == 📘 Rule violation ✓ Correctness
Several modified conditionals use loose comparisons (==) instead of strict comparisons (===), which can cause unexpected behavior due to PHP type juggling. This violates the strict-comparison requirement for condition checks.
Agent Prompt
## Issue description
`htdocs/include/common.php` uses loose comparisons (`==`) in modified conditionals, which violates the strict-comparison requirement.
## Issue Context
These conditions affect core boot behavior (gzip, CLI detection, RTL constants, site-closed), so type juggling can cause hard-to-debug misbehavior.
## Fix Focus Areas
- htdocs/include/common.php[137-205]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+210
to
+219
| if ($icmsConfigPersona["multi_login"]) { | ||
| if (is_object(icms::$user)) { | ||
| $online_handler = icms::handler('icms_core_Online'); | ||
| $online_handler->write(icms::$user->getVar('uid'), icms::$user->getVar('uname'), time(), 0, $_SERVER['REMOTE_ADDR']); | ||
| $online_handler = icms::handler("icms_core_Online"); | ||
| $online_handler->write( | ||
| icms::$user->getVar("uid"), | ||
| icms::$user->getVar("uname"), | ||
| time(), | ||
| 0, | ||
| $_SERVER["REMOTE_ADDR"], | ||
| ); |
Contributor
There was a problem hiding this comment.
3. multi_login key unguarded 📘 Rule violation ⛯ Reliability
$icmsConfigPersona["multi_login"] and $_SERVER["REMOTE_ADDR"] are accessed without isset()/fallbacks, which can trigger notices or errors in edge cases (e.g., missing config or CLI/non-HTTP contexts). This violates the requirement to check array indices before access and provide fallbacks.
Agent Prompt
## Issue description
The code reads `$icmsConfigPersona["multi_login"]` and `$_SERVER["REMOTE_ADDR"]` without checking they exist, which can cause notices or failures.
## Issue Context
This runs during core bootstrap and should be resilient across environments (HTTP vs CLI) and configuration states.
## Fix Focus Areas
- htdocs/include/common.php[210-219]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+166
to
+170
| while (false !== ($entry = $dir->read())) { | ||
| // Skip pointers | ||
| if ($entry == '.' || $entry == '..') {continue;} | ||
| if ($entry == "." || $entry == "..") { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
4. copyrecursive() uses == 📘 Rule violation ✓ Correctness
icms_core_Filesystem::copyRecursive() uses loose equality (==) when comparing directory entries
("."/".."), which violates the strict-comparison requirement and risks type juggling edge cases.
This should be strict (===).
Agent Prompt
## Issue description
`copyRecursive()` uses `==` to compare directory entries, violating the strict-comparison requirement.
## Issue Context
Filesystem routines are sensitive to edge cases; strict comparisons avoid PHP type juggling surprises.
## Fix Focus Areas
- htdocs/libraries/icms/core/Filesystem.php[166-170]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+162
to
165
| onclick="location.href='<?php echo $wizard->pageURI("+1"); ?>'" class="next"> | ||
| <?php } ?> | ||
| <?php if ($_POST['mod'] != 1) { ?> | ||
| <?php if ($_POST["mod"] != 1) { ?> | ||
| <img src="img/right-arr.png" alt="<?php echo BUTTON_NEXT; ?>" width="16"/> |
Contributor
There was a problem hiding this comment.
5. $_post[mod] unchecked 📘 Rule violation ⛯ Reliability
The installer template reads $_POST["mod"] directly without checking it exists, and uses a loose comparison (!= 1), which can trigger notices and type-juggling behavior. This violates the requirement to check array indices before access and to use strict comparisons.
Agent Prompt
## Issue description
The installer template reads `$_POST["mod"]` without an existence check and compares loosely.
## Issue Context
Installer pages should not emit notices/warnings due to missing POST fields.
## Fix Focus Areas
- htdocs/install/install_tpl.php[162-165]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+188
to
+194
| $proto = | ||
| isset($_SERVER["HTTPS"]) && $_SERVER["HTTPS"] === "on" | ||
| ? "https" | ||
| : "http"; | ||
| $host = htmlentities($_SERVER["HTTP_HOST"]); | ||
| $server_php_self = htmlentities($_SERVER["PHP_SELF"]); | ||
| $base = substr($server_php_self, 0, strrpos($server_php_self, "/")); |
Contributor
There was a problem hiding this comment.
6. baselocation() unguarded $_server 📘 Rule violation ⛯ Reliability
baseLocation() reads $_SERVER["HTTP_HOST"] and $_SERVER["PHP_SELF"] without checking they exist, which can trigger notices in nonstandard environments or proxied/test contexts. This violates the requirement to check array indices before access and provide fallbacks.
Agent Prompt
## Issue description
`baseLocation()` accesses `$_SERVER` keys without checking they exist.
## Issue Context
While typically present in web requests, these keys can be missing in edge cases; the installer should not emit notices.
## Fix Focus Areas
- htdocs/install/class/IcmsInstallWizard.php[188-194]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| $feed->set_feed_url($rssurl); | ||
| $feed->set_cache_duration(3600); | ||
| $feed->set_autodiscovery_level(SIMPLEPIE_LOCATOR_NONE); | ||
| $feed->set_autodiscovery_level(\Simplepie\Simplepie::LOCATOR_NONE); |
Contributor
There was a problem hiding this comment.
7. Bad simplepie namespace 🐞 Bug ✓ Correctness
Admin RSS now calls \Simplepie\Simplepie::LOCATOR_NONE (wrong casing/namespace), which will trigger a class-not-found fatal when that page renders.
Agent Prompt
### Issue description
`htdocs/admin.php` references `\Simplepie\Simplepie::LOCATOR_NONE` which does not match the actual Composer SimplePie class namespace. This will fail at runtime.
### Issue Context
SimplePie is now provided via Composer and used as `\SimplePie\SimplePie`.
### Fix Focus Areas
- htdocs/admin.php[90-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+405
to
+419
| static public function getAvailable(): array | ||
| { | ||
| $package_types = ['impresscms-theme', 'impresscms-module']; | ||
|
|
||
| $dirtyList = $cleanList = array(); | ||
| foreach($package_types as $package_type) { | ||
| $dirtyList = \Composer\InstalledVersions::getInstalledPackagesByType($package_type); | ||
| foreach ($dirtyList as $item) { | ||
|
|
||
| $cleanList[$item]['Name'] = $item; | ||
| $cleanList[$item]['Version'] = InstalledVersions::getVersion($item); | ||
| $cleanList[$item]['PrettyVersion'] = InstalledVersions::getPrettyVersion($item); | ||
| } | ||
| } | ||
| return $cleanList; |
Contributor
There was a problem hiding this comment.
8. Getavailable() api break 🐞 Bug ✓ Correctness
icms_module_Handler::getAvailable() now returns Composer package metadata arrays, but installer/modulesadmin treat it as a list of module dirnames; this can produce warnings and break module install/admin flows when such packages are present.
Agent Prompt
### Issue description
`icms_module_Handler::getAvailable()` changed its return type/shape, but existing consumers still assume the legacy string list of module directory names.
### Issue Context
Installer and modules admin build filesystem paths and compute diffs using the returned values.
### Fix Focus Areas
- htdocs/libraries/icms/module/Handler.php[388-420]
- htdocs/install/page_modulesinstall.php[93-99]
- htdocs/modules/system/admin/modulesadmin/modulesadmin.php[97-103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+697
to
+734
| // ── Copy source → destination ───────────────────────────────────────── | ||
| @set_time_limit(300); | ||
|
|
||
| if (!self::copyRecursive($src, $dest)) { | ||
| // Copy failed – clean up any partial destination. | ||
| if (is_dir($dest)) { | ||
| self::deleteRecursive($dest, true); | ||
| } | ||
| return $base + [ | ||
| "status" => "error_copy", | ||
| "message" => sprintf( | ||
| "Failed to copy the vendor directory from the web root (%s) to the " . | ||
| "trust path (%s). Please check that the trust path is writable and " . | ||
| "that there is sufficient disk space, then retry. " . | ||
| "Alternatively, copy the vendor directory manually via FTP/SFTP.", | ||
| $src, | ||
| $dest, | ||
| ), | ||
| ]; | ||
| } | ||
|
|
||
| // Verify the copy completed by checking that autoload.php arrived. | ||
| if (!file_exists($dest . "/autoload.php")) { | ||
| self::deleteRecursive($dest, true); | ||
| return $base + [ | ||
| "status" => "error_copy", | ||
| "message" => sprintf( | ||
| "The vendor copy to the trust path (%s) appears incomplete " . | ||
| "(autoload.php is missing). The partial copy has been removed. " . | ||
| "Please retry or copy the vendor directory manually via FTP/SFTP.", | ||
| $dest, | ||
| ), | ||
| ]; | ||
| } | ||
|
|
||
| // ── Delete the original from the web root ───────────────────────────── | ||
| self::deleteRecursive($src, true); | ||
|
|
Contributor
There was a problem hiding this comment.
9. Vendor move can corrupt 🐞 Bug ⛯ Reliability
moveVendorToTrust() can delete the source vendor even if some files fail to copy, because copyRecursive() ignores copy() failures and returns true after the loop; this can leave an incomplete vendor tree and break the site.
Agent Prompt
### Issue description
The vendor move uses `copyRecursive()` which does not detect per-file copy failures, yet `moveVendorToTrust()` deletes the source after only checking for `autoload.php`.
### Issue Context
A partial copy (permissions/disk full) could still include `autoload.php`, causing the original vendor to be deleted and leaving the system in a broken state.
### Fix Focus Areas
- htdocs/libraries/icms/core/Filesystem.php[152-181]
- htdocs/libraries/icms/core/Filesystem.php[697-734]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
This introduces Composer-based autoloading for libraries. No new functionality, just the base framework to start extricating bundled PHP libraries from the core.
To test : execute a 'composer install' to get the necessary files in the vendor folder. This will have been done already in the release archive.