fix: seed demo content from the current schema#796
Conversation
Frankli9986
left a comment
There was a problem hiding this comment.
Review summary
This PR correctly addresses #572. Replacing the legacy db/cats_testdata.bak restore path with db/cats_schema.sql followed by db/cats_demo_data.sql moves demo installation off the old mysql extension path and onto the existing mysqli-based MySQLQueryMultiple() helper, which is the root cause of the reported bug.
What looks good
db/cats_demo_data.sqlcontains only demoINSERTstatements — no schema DDL, nomodule_schemastate, and no base lookup data that belongs incats_schema.sql.- I cross-checked every table referenced in the new seed file against
db/cats_schema.sqland confirmed all tables and inserted columns exist in the current schema. - Existing non-demo install and legacy upgrade paths are untouched.
- Removing the stale
db/cats_testdata.baksnapshot is appropriate now that it is unused.
Suggestions / edge cases
- Add a guard for missing SQL files in
modules/install/ajax/ui.phparound the newfile_get_contents('db/cats_schema.sql')andfile_get_contents('db/cats_demo_data.sql')calls. If either file is missing,file_get_contents()returnsfalse, and in PHP 8explode()inMySQLQueryMultiple()will throw aTypeErrorinstead of silently no-oping. A clear installer error message (e.g. "Demo data files are missing") would be more user-friendly. - Consider wrapping the schema + seed execution in a transaction or adding basic error reporting so a partial demo install does not leave the database in a half-populated state.
- For PHP 7/8 compatibility, the code is otherwise fine: it uses
file_get_contents(),mysqli_query(), and no removed PHP 8 constructs.
Does this fix #572?
Yes. The old demo path restored a zipped .bak snapshot through ZipFileExtractor, which relied on the legacy mysql_* functions. The new path uses the same current-schema base as an empty install and then applies demo-only seed data via the existing mysqli helpers.
I'm happy to run PHP 7/8 demo-install tests if the project has CI or a local test matrix for the installer — just point me at the relevant test commands.
Frankli9986
left a comment
There was a problem hiding this comment.
Thanks for the fix — this is a solid improvement over restoring from a legacy full-database backup.
What works well:
- Directly addresses #572: demo content no longer flows through the old
cats_testdata.bak/ZipFileExtractorpath that relied on legacymysqlfunctions, and instead reuses the currentdb/cats_schema.sql+MySQLQueryMultiplepath already used by the empty-database installer. - Removing the binary
.bakfile reduces repo weight and eliminates a stale snapshot that could drift from the real schema. - The seed-only SQL file is easier to maintain and review than a compressed database dump.
Suggestions / things to double-check:
- Error handling for missing SQL files (PHP 8 compatibility):
file_get_contents()returnsfalseon failure. In PHP 8, passingfalseintoMySQLQueryMultiple()->explode()will throw aTypeError. Consider guarding both reads inonLoadDemoData(and the same pattern indoInstallEmptyDatabasewhile you are here):$schema = file_get_contents('db/cats_schema.sql'); if ($schema === false) { echo 'Failed to read db/cats_schema.sql'; break; } MySQLQueryMultiple($schema, ";
");
2. **SQL delimiter robustness:** Splitting on `";
"` works for the current files but will break if any INSERT ever contains that sequence inside a string literal. The existing `MySQLQueryMultiple()` has the same limitation, so this is not a regression — just something to be aware of if demo data is extended later.
3. **Foreign-key / referential sanity:** The seed references `site_id = 1` and `entered_by = 1`, and `activity` references candidate/joborder IDs. The current schema has no foreign keys, so insert order is not a problem. If FKs are ever added, the order in `db/cats_demo_data.sql` may need to be adjusted (e.g., `company`/`contact` before `joborder`, `candidate` before `activity`).
4. **User/site default data:** The new seed does not insert any `user` or `site` rows. If the installer creates those in a prior step, this is fine; otherwise the first login after a demo install may fail. Worth confirming against a real install flow.
5. **Minor maintainability:** The schema-load + seed-load logic is now duplicated between `doInstallEmptyDatabase` and `onLoadDemoData`. A small helper such as `loadSqlFile($path)` would make both paths shorter and more consistent.
I’m happy to run a PHP 7 / PHP 8 demo-install test if you can point me at the preferred test harness (or I can spin up the installer in a container). Just let me know.
Frankli9986
left a comment
There was a problem hiding this comment.
Thanks for this fix — it directly addresses #572 by replacing the legacy .bak restore path with file_get_contents + MySQLQueryMultiple, so the demo installer no longer relies on the old mysql extension extraction logic. The change is focused and the removal of db/cats_testdata.bak is appropriate since it is no longer referenced.
Correctness / edge cases
- Loading
db/cats_schema.sqlfirst and thendb/cats_demo_data.sqlis the right order; it means demo installs start from the same current-schema baseline as fresh installs. - Keeping
module_schemarows at version0from the base schema prevents the installer from incorrectly running legacy upgrade steps on demo data. - One edge case worth double-checking: the demo seed uses
site_id = 1throughout (e.g.,activity,candidate,company,joborder), and the basecats_schema.sqlinserts the admin user withsite_id = 1, but the onlysiterow inserted by the schema issite_id = 180(CATS_ADMIN). Because there are no FK constraints the inserts will not fail, but any code that later doesSELECT * FROM site WHERE site_id = 1will come up empty. If the originalcats_testdata.bakrestored asite_id = 1row, the new seed may be missing that base lookup row. Consider either adding anINSERT INTO siterow forsite_id = 1to the seed file, or aligning the demo references to the existingsite_id = 180row.
PHP 7 / PHP 8 compatibility
- The new code uses
file_get_contentsand the existingMySQLQueryMultiple/MySQLQueryhelpers, which wrapmysqli_*. No deprecatedmysql_*calls or PHP-version-specific syntax is introduced. Looks safe for PHP 7.4 through PHP 8.x.
Clarity / maintainability
- The inline comment explaining the new demo-mode flow is helpful.
- Switching the next installer step from
upgradeCatstoresumeParsingis consistent with the non-demo fresh-install path and avoids an unnecessary upgrade pass.
Does it fix #572?
Yes — the root cause in #572 was that demo content attempted to load through old mysql functions inside the zip extractor. That path is gone, replaced by direct SQL file execution through the project's existing mysqli-based helpers.
I'm happy to run a PHP 7 and PHP 8 demo-install smoke test if you can point me at the preferred environment/CI script (or I can spin one up in Docker from the repo's compose/setup files). Just let me know.
Thank you for your catch, I will adjust that and maybe find some time to open a new PR which removes the legacy multi tenant code. |
Fixes #572
This PR changes the demo installation flow so demo content is no longer restored from a full legacy database backup. Instead, demo installations now load the current base schema from
db/cats_schema.sqlfirst and then apply demo-only seed data fromdb/cats_demo_data.sql.The new demo seed contains only data intended to be applied on top of the current schema. It does not include schema definitions, historical
module_schemastate or base lookup data that already belongs todb/cats_schema.sql. This avoids treating demo content as an old installation that has to pass through legacy upgrade behavior.The installer path for demo data now follows the same current-schema starting point as a fresh installation and then adds the demo records on top. Existing non-demo installation and legacy upgrade paths are left unchanged.
The old
db/cats_testdata.bakdemo database snapshot has been removed because it is no longer used by the active demo installation flow.