Improve installation #1598
Improve installation #1598
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the installation process by updating minimum PHP and XOOPS version requirements and standardizing database operations to use the exec() method for write operations (INSERT, UPDATE, DELETE, DDL) instead of the query() method.
Changes:
- Updated minimum PHP version requirement from 5.6.0 to 7.4 and minimum XOOPS version from 2.5.11 to 2.5.12
- Replaced
query()withexec()for all mutating database operations across installation, uninstallation, and core model files - Removed conditional logic that previously chose between
query()andexec()based on force flags, now consistently usingexec()for write operations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/xoops_lib/modules/protector/xoops_version.php | Updated minimum PHP version to 7.4 and minimum XOOPS version to 2.5.12 |
| htdocs/xoops_lib/modules/protector/onuninstall.php | Changed DROP TABLE query from query() to exec() |
| htdocs/xoops_lib/modules/protector/oninstall.php | Changed CREATE TABLE query from query() to exec() |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Changed table creation query from query() to exec() |
| htdocs/modules/profile/include/install.php | Changed INSERT queries from query() to exec() |
| htdocs/class/model/write.php | Standardized all write operations (INSERT, UPDATE, DELETE) to use exec() method with commented-out old conditional logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
htdocs/class/model/write.php
Outdated
| // $queryFunc = empty($force) ? 'query' : 'exec'; | ||
| $queryFunc = 'exec'; |
There was a problem hiding this comment.
These commented-out lines should be removed rather than kept as comments. The old logic that conditionally chose between 'query' and 'exec' based on the $force parameter has been replaced with always using 'exec', which is the correct approach for DELETE operations. Leaving commented code reduces maintainability.
htdocs/class/model/write.php
Outdated
| // $queryFunc = empty($force) ? 'query' : 'exec'; | ||
| $queryFunc = 'exec'; |
There was a problem hiding this comment.
These commented-out lines should be removed rather than kept as comments. The old logic that conditionally chose between 'query' and 'exec' based on the $force parameter has been replaced with always using 'exec', which is the correct approach for UPDATE operations. Leaving commented code reduces maintainability.
| $modversion['module_website_name'] = 'XOOPS'; | ||
| $modversion['min_php'] = '5.6.0'; | ||
| $modversion['min_xoops'] = '2.5.11'; | ||
| $modversion['min_php'] = '7.4'; |
There was a problem hiding this comment.
The version string '7.4' is inconsistent with the previous format '5.6.0' and other modules in the codebase that use three-component version strings (e.g., '5.6.0' in pm/xoops_version.php and profile/xoops_version.php). While functionally equivalent in version_compare(), it's better to maintain consistency with the established format. Consider using '7.4.0' instead.
| $modversion['min_php'] = '7.4'; | |
| $modversion['min_php'] = '7.4.0'; |
htdocs/class/model/write.php
Outdated
| // $queryFunc = empty($force) ? 'query' : 'exec'; | ||
| $queryFunc = 'exec'; |
There was a problem hiding this comment.
These commented-out lines should be removed rather than kept as comments. The old logic that conditionally chose between 'query' and 'exec' based on the $force parameter has been replaced with always using 'exec', which is the correct approach for DELETE operations. Leaving commented code reduces maintainability.
- Remove commented-out $force conditional lines (Copilot review feedback) - Eliminate $queryFunc variable; call ->exec() directly for all writes - Remove redundant $queryFunc reassignments in insert() method - Use '7.4.0' for min_php to match codebase three-component format Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaced multiple database write calls from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
![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
🤖 Fix all issues with AI agents
In `@htdocs/install/sql/mysql.structure.sql`:
- Line 160: configoption.conf_id is defined as smallint(5) unsigned while
config.conf_id is int(10) unsigned, risking mismatch when config.conf_id >
65535; update the configoption table definition to change the column conf_id
from smallint(5) unsigned to int(10) unsigned (and adjust any index definitions
referencing configoption.conf_id if necessary) so both tables use the same type
and width for conf_id.
🧹 Nitpick comments (2)
htdocs/class/model/write.php (2)
264-266: Unused variable assignment.The static analysis correctly flags that
$resultis assigned but not used beyond the condition check. You can simplify this:♻️ Proposed simplification
- if (!$result = $this->handler->db->exec($sql)) { + if (!$this->handler->db->exec($sql)) { return false; }
276-278: Same pattern: unused variable assignment.Similar to line 264, the
$resultvariable is assigned but only used in the condition check.♻️ Proposed simplification
- if (!$result = $this->handler->db->exec($sql)) { + if (!$this->handler->db->exec($sql)) { return false; }
- configoption.conf_id: smallint(5) → int(10) unsigned to match config.conf_id, preventing data integrity issues past 65535 rows - write.php insert(): remove unused $result assignment from exec() calls (INSERT and UPDATE branches) — value was only checked in the if-condition, never used afterward Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
1 participant
Summary by CodeRabbit
Chores
Infrastructure
Improvements