Skip to content

Add error handling and validation improvements to utility methods - #274

Merged
jwaisner merged 2 commits into
mainfrom
bugs
Oct 5, 2026
Merged

jwaisner merged 2 commits into
mainfrom
bugs

Conversation

@N6REJ

@N6REJ N6REJ commented Oct 5, 2026

Copy link
Copy Markdown
Contributor
  • Add early return with error logging when file operations fail in replaceInFile()
  • Preserve line endings consistently by detecting and reapplying original CRLF/LF format
  • Return empty array instead of false from getVersionList() when directory cannot be opened
  • Add validation for version string length in getVersionList() to prevent empty suffixes
  • Check for target file at start path before recursing in findReposFromRoot()
  • Add null coalescing to prevent iteration over false values in getFolderList() calls
  • Replace is_numeric() with filter_var() for stricter port validation
  • Add error handling for JSON decode failures in getLatestVersion()
  • Use null coalescing operator for safer array access in GitHub API response
  • Add caption sanitization and file write validation in openFileContent()
  • Add error logging and empty array fallback when module folder scanning fails
  • Fix clearFolder() to propagate file count and close directory handle on error
  • Replace bit shift operators with exponentiation for size unit calculations

- Add early return with error logging when file operations fail in replaceInFile()
- Preserve line endings consistently by detecting and reapplying original CRLF/LF format
- Return empty array instead of false from getVersionList() when directory cannot be opened
- Add validation for version string length in getVersionList() to prevent empty suffixes
- Check for target file at start path before recursing in findReposFromRoot()
- Add null coalescing to prevent iteration over false values in getFolderList() calls
- Replace is_numeric() with filter_var() for stricter port validation
- Add error handling for JSON decode failures in getLatestVersion()
- Use null coalescing operator for safer array access in GitHub API response
- Add caption sanitization and file write validation in openFileContent()
- Add error logging and empty array fallback when module folder scanning fails
- Fix clearFolder() to propagate file count and close directory handle on error
- Replace bit shift operators with exponentiation for size unit calculations
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Harden utility methods against invalid input and filesystem failures

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Handle failed file and directory operations without iterating invalid results or opening unwritten
 content.
• Preserve file line endings and tighten port, version, and API-response validation.
• Correct recursive deletion counts and discover repositories at the search start path.
Diagram

graph TD
  Switch["Version switch"] --> Scan["Scan modules"] --> Readable{"Scan succeeded?"}
  Readable -->|yes| Paths["Build scan paths"] --> Rewrite["Replace config lines"] --> Files["Module files"]
  Readable -->|no| Skip["Log and skip"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Normalize folder-list failures in getFolderList
  • ➕ Provides one array-return contract for every caller.
  • ➕ Avoids repeating false-to-empty conversions across module branches.
  • ➖ Changes behavior for callers that distinguish a failed scan from an empty folder.
  • ➖ Needs a way to retain module-specific error context.

Recommendation: Keep the localized fallbacks for this targeted fix: they preserve getFolderList's existing failure contract and identify the affected module in logs. Consider a centralized array-return contract separately if all callers can safely treat scan failure as an empty list.

Files changed (2) +142 / -72

Bug fix (2) +142 / -72
class.action.switchVersion.phpHandle failed module folder scans during version switching +32/-0

Handle failed module folder scans during version switching

• Checks folder-list failures for Apache, PHP, MySQL, MariaDB, PostgreSQL, Node.js, Mailpit, and Xlight. Each failure is logged and replaced with an empty list so path construction does not iterate false.

core/classes/actions/class.action.switchVersion.php

class.util.phpHarden filesystem, validation, and version utilities +110/-72

Harden filesystem, validation, and version utilities

• Adds file-operation guards and line-ending preservation, safe folder-list iteration, stricter port validation, and safer handling of version directories and GitHub responses. Also counts recursively deleted files, closes directory handles on deletion errors, checks temporary-file writes, changes forced size-unit calculations, and removes is32BitsOs().

core/classes/class.util.php

@qodo-code-review

qodo-code-review Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Startup hides failed module scans ✓ Resolved
Description
getPathsToScan() turns a failed getFolderList() call into an empty iteration without logging or
propagating the failure. If a module directory cannot be opened during startup, its configuration
files are omitted from path replacement, and getFilesToScan() can cache that incomplete file list.
Code

core/classes/class.util.php[758]

+        foreach (($folderList ?: array()) as $folder) {
Evidence
getFolderList() returns false when opening a directory fails. The new fallback omits that
module's scan paths; startup passes the resulting file list to Path::changePath(), while
getFilesToScan() caches the list as a successful scan.

core/classes/class.util.php[1024-1034]
core/classes/class.util.php[755-768]
core/classes/class.util.php[672-693]
core/classes/actions/class.action.startup.php[755-772]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed module-folder scan is treated as an empty folder, so startup proceeds with an incomplete file list that can be cached.
## Fix Focus Areas
- core/classes/class.util.php[755-758]
- core/classes/class.util.php[654-693]
## Recommended Fix
Distinguish `false` from an empty folder list, log the affected path, and propagate scan failure so incomplete results are not cached or treated as a successful startup scan. Apply the same handling to the other module and tool folder lists.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Version switches reject padded ports ✓ Resolved
Description
isValidPort() now rejects integer strings with leading zeros even though module configuration
accepts numeric port strings and changePort() converts accepted values to integers. If an existing
setting such as 080 is loaded, the version-switch flow calls changePort() with that value and
returns before updating the port configuration.
Code

core/classes/class.util.php[R208-210]

+        return filter_var($port, FILTER_VALIDATE_INT, array(
+            'options' => array('min_range' => 1, 'max_range' => 65535)
+        )) !== false;
Evidence
Apache loads its port from raw module configuration and accepts it with is_numeric(). The
version-switch action passes the loaded port to changePort(), whose new shared validation rejects
a leading-zero string before its existing intval() conversion.

core/classes/bins/class.bin.apache.php[179-181]
core/classes/bins/class.bin.apache.php[219-224]
core/classes/bins/class.bin.apache.php[283-292]
core/classes/actions/class.action.switchVersion.php[308-312]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The stricter port check rejects leading-zero numeric settings that module configuration already accepts, interrupting port updates during version switching.
## Fix Focus Areas
- core/classes/class.util.php[206-211]
- core/classes/bins/class.bin.apache.php[179-221]
## Recommended Fix
Validate that string inputs contain only decimal digits, normalize them to an integer, then enforce the 1–65535 range. Keep rejecting non-digit input while allowing valid padded port settings.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 3 rules
Review mode: Auto: 🧠 Deep: Many independent utility changes across file, filesystem, API, and validation paths.

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread core/classes/class.util.php Outdated
Comment thread core/classes/class.util.php Outdated
Enhance file scanning and path validation to prevent incomplete scans from corrupting configuration updates. Introduce explicit failure detection in getPathsToScan() when module folders cannot be read, propagating false instead of empty arrays to distinguish scan failures from legitimately empty directories. Add folderList() wrapper to track failed directory reads and prevent silent data loss. Update getFilesToScan() to abort and skip caching when path discovery fails, ensuring incomplete results never poison subsequent operations. Modify startup and reload actions to detect scan failures and skip path updates rather than rewriting files with partial data. Refactor port validation to accept leading zeros (e.g., '080') by validating as decimal digits before range check, aligning with Apache's is_numeric() behavior. Consolidate port validation logic in Util::isValidPort() and update sanitizePort() to use shared rule with improved logging.
@jwaisner
jwaisner merged commit e69d24a into main Oct 5, 2026
3 checks passed
@jwaisner
jwaisner deleted the bugs branch October 5, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants