refactor: replace legacy database result handling#765
refactor: replace legacy database result handling#765anonymoususer72041 wants to merge 4 commits into
Conversation
|
@RussH before merging, we should discuss a few things:
|
|
@Frankli9986's PR #771 seems to be a partial duplicate of this PR.
As mentioned above, this PR may resolve multiple existing issues. One example could be #572, since @Frankli9986's PR specifically targets that issue and overlaps with the
Even if this PR fixes #572, I am still not sure whether changing old migrations is the best long-term approach. For example, I am currently looking into a new standardized migration system, possibly based on something like Phinx. If we go in that direction, the current migration system and the migration definitions in After checking the demo content installation flow in the current master branch, I think the more fundamental issue is that the demo install is based on an old backup snapshot. It loads Maybe we could eventually solve this in a similar direction as #716: instead of keeping and patching another old database fixture, the demo data could be rebased onto the current canonical schema or regenerated in a way that no longer requires replaying old install migrations. That would make the changes in this PR and #771 useful as short-term compatibility fixes, but not necessarily the final solution for #572. |
|
@anonymoususer72041
I agree this is not ideal as a long-term pattern. In general, I’d rather avoid rewriting old migrations unless we have to. In this case, though, the changes look acceptable as a short-term compatibility repair because they don't appear to change the migration intent; they replace PHP-incompatible mysql_* usage with the existing DatabaseConnection escaping/query helpers. So I’m comfortable with this for PHP 7.x/8.x compatibility, but I agree it shouldn’t become the preferred future direction. Longer term, replacing the legacy migration/demo install flow with a cleaner migration system, or regenerated current demo data, sounds like the better fix.
I would keep these. They are not just ChatGPT noise from my point of view. The PR now relies on getAssoc() operating on the active result set after query(), plus getNumRows(), getAffectedRows(), and getLastInsertID(). Having tests like this is useful, especially while we are replacing direct mysqli usage.
I’m happy to review the affected issues after merge, but I would avoid claiming that this PR closes specific issues unless the relevant path has been tested. It may well help with #572 or overlap with #771, especially around demo install / legacy mysql_* usage, but because demo content installation and backup behaviour have historically been fragile, I’d prefer wording it as “may help with” rather than “fixes” until confirmed. So my view is:
General PR review: Overall, this looks like a good direction. The move away from direct mysqli/mysql result handling and towards DatabaseConnection API fits the project better, and the PHP 7.2 test run pass is a plus. The ControlPanel.php changes look broadly clean, with getNumRows(), getAssoc(), getAffectedRows(), and getLastInsertID() replacing direct mysqli calls. One thing I would like reviewed before merge: backupDB.php now uses getAllAssoc() when dumping table data. That is cleaner API-wise, but it changes the backup path from row-by-row result iteration to loading each table result set into memory first. That may be fine for smaller installs, but could be unsafe for larger OpenCATS databases. If practical, I’d prefer keeping the DatabaseConnection abstraction while preserving row-by-row processing for table dumps. Before I can merge this, please resolve the current conflict in lib/ControlPanel.php and check the new Codacy issue? Once those are cleared, I’m happy to re-review/merge. |
anonymoususer72041
force-pushed
the
refactor/database-api-usage
branch
from
83c18d2 to
d0d14c1
Compare
|
Regarding the Codacy StaticAccess warning in For this PR, I would prefer to keep the installer change minimal and explicit. Your main substantive review point has been addressed separately, @RussH: |
anonymoususer72041
force-pushed
the
refactor/database-api-usage
branch
from
d0d14c1 to
edf7ff5
Compare
|
I force-pushed an updated version of this branch. The installer settings write path is now folded into the install database result handling commit, so the installer changes are kept together and the history is more focused. In particular, the fromAddress update/insert handling now consistently uses DatabaseConnection query execution and escaping instead of mixing MySQLQuery() with DatabaseConnection state checks. |
|
Following up from #771 — happy to consolidate around this PR. The demo-content installation on PHP 7.4 / 8.x (which #572 reports) is the specific path I tested, so once the migration changes here settle into a final shape I can run that scenario end-to-end against this branch and report back. Just ping me when it's ready for that pass. |
|
@Frankli9986 you might want to check out #796, which addresses the real problem behind #572. I have not tested it, so a review of #796 would be appreciated. |
3 participants
This PR removes obsolete legacy MySQL calls and reduces direct mysqli result handling in selected database-related areas by routing them through the existing DatabaseConnection API.
The install schema no longer uses obsolete mysql_* calls, and install-module database result handling now relies more consistently on the existing database helpers. Installer settings writes were also adjusted to use DatabaseConnection query execution and escaping consistently.
Backup-related database access in settings and script code is routed through the existing connection handling while preserving row-by-row processing where appropriate.
Database tests were extended to cover the normalized query handling patterns used by these changes, especially calling getAssoc() after a prior query(), iterating over the active result set, reading active result row counts, checking affected rows and retrieving inserted IDs.