Skip to content

refactor: replace legacy database result handling#765

Open
anonymoususer72041 wants to merge 6 commits into
opencats:masterfrom
anonymoususer72041:refactor/database-api-usage
Open

refactor: replace legacy database result handling#765
anonymoususer72041 wants to merge 6 commits into
opencats:masterfrom
anonymoususer72041:refactor/database-api-usage

Conversation

@anonymoususer72041
Copy link
Copy Markdown
Contributor

@anonymoususer72041 anonymoususer72041 commented May 2, 2026

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 result handling now relies more consistently on the existing database helpers. Backup-related database access in settings and script code is also routed through the existing connection handling while preserving row-by-row processing where appropriate.

ControlPanel database result handling has been updated to use DatabaseConnection methods for row fetching, row counts, affected rows and inserted IDs instead of direct mysqli_* calls. The count-row handling was also adjusted to explicitly read the named count column rather than relying on the previous mysqli_fetch_row() pattern.

Database tests were extended to cover the normalized query handling patterns used by these changes, especially calling getAssoc() after a prior query() and iterating over the active result set. Obsolete database comments and dead examples referencing legacy mysql_* APIs were also cleaned up.

@anonymoususer72041
Copy link
Copy Markdown
Contributor Author

anonymoususer72041 commented May 2, 2026

@RussH before merging, we should discuss a few things:

  1. I am not sure if changing previous migrations in f481abd is a good idea. Generally speaking, I have a problem with the current migration system and want to implement a new system (while removing the different legacy migration systems long term).

  2. Updating the tests in fb229c6 was an idea of ChatGPT. I am not sure it this is a beneficial commit, so I am okay to drop it if it's just noise.

  3. Generally, this PR might fix other previous opened issues, for example the backup functionality. I haven't tested them as the GitHub issues tab does not seem to be properly maintained. Could you take a look onto this and cleanup the affected issues after merging this PR, @RussH?

@anonymoususer72041
Copy link
Copy Markdown
Contributor Author

@Frankli9986's PR #771 seems to be a partial duplicate of this PR.

  1. Generally, this PR might fix other previous opened issues, for example the backup functionality. I haven't tested them as the GitHub issues tab does not seem to be properly maintained. Could you take a look onto this and cleanup the affected issues after merging this PR, @RussH?

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 modules/install/Schema.php changes in this PR. I have not tested the demo content installation myself, because it is not my focus.

  1. I am not sure if changing previous migrations in f481abd is a good idea. Generally speaking, I have a problem with the current migration system and want to implement a new system (while removing the different legacy migration systems long term).

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 modules/install/Schema.php may eventually be replaced. That is why I see both this PR's fix and #771's fix for #572 as temporary solutions rather than the final direction.

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 db/cats_testdata.bak, executes the contained db/catsbackup.sql.* files and then continues through the legacy upgrade and module schema handling. This means that every fresh demo installation has to replay historical upgrade logic again, including old migration code that can break on newer PHP versions.

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.

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.

1 participant