normalize profile and session indexes for 2.7.0#23
Merged
mambax7 merged 15 commits intoXOOPS:masterfrom Apr 17, 2026
Merged
Conversation
Add three idempotent upgrade tasks to the 2.5.11 → 2.7.0 patch that
standardise index DDL across fresh and upgraded installations:
- normalizeprofilefieldname — ensures profile_field.field_name is
varchar(64) and its UNIQUE index uses explicit prefix (64) USING BTREE.
- normalizeprofileregstepsort — recreates profile_regstep.sort as
KEY (step_order, step_name(100)) USING BTREE.
- normalizesessionpk — recreates session PRIMARY KEY as
(sess_id(200)) USING BTREE.
Each check queries information_schema.STATISTICS.SUB_PART and only
applies when the prefix differs from the target, so the tasks are safe
to re-run against installations at any intermediate state. Two private
helpers (indexExists, indexPrefixLength) back the checks.
Fresh-install SQL updated to converge on the same final state:
- install/sql/mysql.structure.sql
session PRIMARY KEY → (sess_id(200)) USING BTREE.
- modules/profile/sql/mysql.sql
profile_field UNIQUE KEY field_name → (field_name(64)) USING BTREE.
profile_regstep KEY sort → (step_order, step_name(100)) USING BTREE.
All composite index sizes stay under the 767-byte InnoDB 5.7 default
limit when evaluated against utf8mb4 (4 bytes/char), keeping the schema
portable to InnoDB without innodb_large_prefix:
profile_field.field_name : 64*4 = 256 bytes
profile_regstep.sort : 2 + 100*4 = 402 bytes
session PRIMARY KEY : sess_id is CHARACTER SET ascii, 200 bytes
Follows the conventional-commits + XOOPS extension format (feat type, upgrade scope, single-sentence title under 70 chars). No RFC: footer — let me know if this work ties to a
specific RFC and I'll add one.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdd schema-normalization upgrade tasks and adjust MySQL index definitions: index prefix lengths and explicit USING BTREE specified in SQL; upgrade scripts now validate information_schema metadata and conditionally apply ALTER TABLE to normalize Changes
Sequence Diagram(s)sequenceDiagram
participant Upgrade as Upgrade System
participant InfoSchema as information_schema
participant DB as MySQL DB
Upgrade->>InfoSchema: Query table and index metadata
InfoSchema-->>Upgrade: Return metadata (columns, seq_in_index, sub_part, non_unique)
alt Metadata matches canonical definition
Upgrade->>Upgrade: Mark task PASSED / no-op
else Metadata differs or missing
Upgrade->>DB: Execute ALTER TABLE (MODIFY / DROP INDEX / ADD INDEX)
DB-->>Upgrade: Return success / warnings
alt Column wider than expected
Upgrade->>Upgrade: Log warning, skip index normalization
end
end
Upgrade->>Upgrade: Continue next task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #23 +/- ##
============================================
- Coverage 19.23% 18.00% -1.23%
- Complexity 7584 7803 +219
============================================
Files 621 665 +44
Lines 40085 42806 +2721
============================================
Hits 7709 7709
- Misses 32376 35097 +2721 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds schema-normalization steps to the 2.5.11 → 2.7.0 upgrade patch and aligns fresh-install SQL so that upgraded and newly installed databases converge on identical index definitions (with explicit index prefixes).
Changes:
- Add three idempotent upgrade tasks to normalize index DDL for
profile_field.field_name,profile_regstep.sort, and thesessionprimary key. - Add helper methods to detect index existence and prefix lengths via
information_schema. - Update module/core SQL dumps to match the target index definitions for fresh installs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds new upgrade tasks + helpers to normalize profile/session index definitions during upgrade. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module schema so fresh installs create the normalized indexes. |
| htdocs/install/sql/mysql.structure.sql | Updates fresh-install session table PK definition to the new (prefixed) form. |
Comment on lines
+764
to
+769
| public function apply_normalizesessionpk(): bool | ||
| { | ||
| $table = $this->db->prefix('session'); | ||
| $sql = "ALTER TABLE `{$table}` DROP PRIMARY KEY, ADD PRIMARY KEY (`sess_id`(200)) USING BTREE"; | ||
|
|
||
| return $this->execOrFail($sql); |
| sess_ip varchar(45) NOT NULL default '', | ||
| sess_data text, | ||
| PRIMARY KEY (sess_id), | ||
| PRIMARY KEY (sess_id(200)) USING BTREE, |
Comment on lines
+632
to
+642
| public function check_normalizeprofilefieldname(): bool | ||
| { | ||
| $table = $this->db->prefix('profile_field'); | ||
|
|
||
| // Column width must be 64 | ||
| $sql = "SELECT CHARACTER_MAXIMUM_LENGTH FROM `information_schema`.`COLUMNS`" | ||
| . " WHERE `TABLE_SCHEMA` = DATABASE()" | ||
| . " AND `TABLE_NAME` = " . $this->db->quote($table) | ||
| . " AND `COLUMN_NAME` = 'field_name' LIMIT 1"; | ||
| $result = $this->db->query($sql); | ||
| if (!$this->db->isResultSet($result) || !($result instanceof \mysqli_result)) { |
Comment on lines
+711
to
+733
| public function check_normalizeprofileregstepsort(): bool | ||
| { | ||
| $table = $this->db->prefix('profile_regstep'); | ||
|
|
||
| return 100 === $this->indexPrefixLength($table, 'sort', 'step_name'); | ||
| } | ||
|
|
||
| /** | ||
| * Recreate the `sort` index with an explicit (100) prefix on step_name. | ||
| * | ||
| * @return bool true on success | ||
| */ | ||
| public function apply_normalizeprofileregstepsort(): bool | ||
| { | ||
| $table = $this->db->prefix('profile_regstep'); | ||
|
|
||
| if ($this->indexExists($table, 'sort')) { | ||
| if (!$this->execOrFail("ALTER TABLE `{$table}` DROP INDEX `sort`")) { | ||
| return false; | ||
| } | ||
| } | ||
| $sql = "ALTER TABLE `{$table}` ADD KEY `sort` (`step_order`, `step_name`(100)) USING BTREE"; | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@upgrade/upd_2.5.11-to-2.7.0/index.php`:
- Around line 764-770: The DDL in apply_normalizesessionpk() performs DROP
PRIMARY KEY, ADD PRIMARY KEY on the session table which rebuilds the table and
can block live traffic; update release/upgrade notes and the upgrade runner
instructions to require maintenance/cleanmode before running
apply_normalizesessionpk(), and mention as optional pre-step that admins with
very large session rows may TRUNCATE the session table to make the DDL
near-instant; ensure the guidance references the apply_normalizesessionpk() step
so operators know to schedule downtime around this operation.
- Around line 711-716: The current check_normalizeprofileregstepsort only checks
SUB_PART for step_name via indexPrefixLength and thus misses cases where the
'sort' index lacks step_order or has wrong column order; replace or augment that
check to query information_schema.STATISTICS for the given $table and INDEX_NAME
'sort', ordering by SEQ_IN_INDEX, and verify there are exactly two rows with
COLUMN_NAME/SEQ_IN_INDEX pairs (step_order, 1) and (step_name, 2) and that
SUB_PART is NULL for step_order and 100 for step_name before returning true; use
the existing $table and 'sort' index identifiers and fail the check if the rows,
seq numbers, or sub_part values differ.
- Around line 660-695: The ALTER that narrows profile_field.field_name to
varchar(64) in apply_normalizeprofilefieldname can silently truncate existing
values when the current CHARACTER_MAXIMUM_LENGTH > 64; after fetching $row from
the information_schema query, explicitly check if (int)$row[0] > 64 and in that
case do not attempt the ALTER: instead log a clear warning including the current
length and the table name ($table) and return false (or alternatively return a
distinct failure code) so the migration either refuses to shrink or requires
manual intervention; keep the existing path for lengths <= 64 so the subsequent
index recreation still runs only when safe.
- Around line 752-757: The current check_normalizesessionpk uses
indexPrefixLength($table,'PRIMARY','sess_id') which can return null if the
session table has no primary key, causing apply_ to run a combined "DROP PRIMARY
KEY, ADD PRIMARY KEY" that will fail; update the migration to mirror
apply_normalizeprofilefieldname/apply_normalizeprofileregstepsort by modifying
apply_normalizesessionpk to split the ALTER into two statements and only run
"DROP PRIMARY KEY" if indexExists($table,'PRIMARY') returns true, then run a
separate "ADD PRIMARY KEY (sess_id(200))" statement, and keep
check_normalizesessionpk as-is or adjust it only if you want to treat a missing
PK as already normalized—use the symbols check_normalizesessionpk,
indexPrefixLength, indexExists, and apply_normalizesessionpk to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 619121d4-027d-4fe4-b84b-0f7703100247
📒 Files selected for processing (3)
htdocs/install/sql/mysql.structure.sqlhtdocs/modules/profile/sql/mysql.sqlupgrade/upd_2.5.11-to-2.7.0/index.php
Address review feedback on the profile/session index normalisation tasks added in 030463f: - Gate profile_field and profile_regstep tasks on table existence. If the Profile module isn't installed, both check_ and apply_ short-circuit to "already applied" instead of aborting the whole 2.7.0 patch on a missing information_schema row or a failing ALTER against a missing table. - Refuse to silently narrow profile_field.field_name. If a site has customised the column to a width greater than 64, apply_ now logs a descriptive error and returns false rather than relying on server sql_mode to raise an error — avoids silent data truncation on servers without STRICT_ALL_TABLES. - Tighten the profile_regstep.sort layout check. The previous check validated only the step_name SUB_PART; a partial hand-patch that dropped step_order from the index could pass. Now reads the full index via information_schema.STATISTICS ordered by SEQ_IN_INDEX and asserts the exact expected shape [(step_order, 1, NULL), (step_name, 2, 100)]. - Split the session PRIMARY KEY rebuild into separate DROP and ADD statements, guarded by indexExists('PRIMARY'). A session table that somehow lost its PK (manual intervention, prior failed migration) can now be normalised; the previous combined ALTER aborted on the DROP. Adds private tableExists() helper alongside indexExists() and indexPrefixLength().
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent schema-normalization tasks to the 2.5.11 → 2.7.0 upgrade patch and aligns fresh-install SQL so upgraded and new installs converge on the same index definitions for the Profile module and session table.
Changes:
- Add three upgrade tasks to normalize Profile module indexes and the session table PRIMARY KEY.
- Introduce helper methods to detect table/index presence and index prefix lengths via
information_schema. - Update fresh-install SQL schema files to use the normalized index definitions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds upgrade tasks + helpers to normalize profile/session index DDL using information_schema. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module table indexes to explicit prefix lengths and BTREE. |
| htdocs/install/sql/mysql.structure.sql | Updates session table PRIMARY KEY to use a prefix length and BTREE. |
Comment on lines
+1007
to
+1016
| private function tableExists(string $table): bool | ||
| { | ||
| $sql = "SELECT 1 FROM `information_schema`.`TABLES`" | ||
| . " WHERE `TABLE_SCHEMA` = DATABASE()" | ||
| . " AND `TABLE_NAME` = " . $this->db->quote($table) | ||
| . " LIMIT 1"; | ||
| $result = $this->db->query($sql); | ||
| if (!$this->db->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| return false; | ||
| } |
Comment on lines
+643
to
+655
| // Column width must be 64 | ||
| $sql = "SELECT CHARACTER_MAXIMUM_LENGTH FROM `information_schema`.`COLUMNS`" | ||
| . " WHERE `TABLE_SCHEMA` = DATABASE()" | ||
| . " AND `TABLE_NAME` = " . $this->db->quote($table) | ||
| . " AND `COLUMN_NAME` = 'field_name' LIMIT 1"; | ||
| $result = $this->db->query($sql); | ||
| if (!$this->db->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| return false; | ||
| } | ||
| $row = $this->db->fetchRow($result); | ||
| if (!$row || 64 !== (int) $row[0]) { | ||
| return false; | ||
| } |
Address second-round review feedback on PR XOOPS#23: - tableExists() now returns ?bool. null means the information_schema query failed (connection issue, permission denial), distinct from false which means "table is absent". Callers previously collapsed both into "skip the task", which could silently leave a present table unnormalised when the DB was in a partially-broken state. Callers now treat null as a hard failure (log + return false) and only skip when the table is genuinely absent. - check_/apply_normalizeprofilefieldname() now validate DATA_TYPE in addition to CHARACTER_MAXIMUM_LENGTH. A field_name defined as char(64) has CHARACTER_MAXIMUM_LENGTH=64, so the previous check incorrectly treated it as already normalised. The new check requires DATA_TYPE='varchar' before passing. apply_ additionally refuses to touch the column when DATA_TYPE is anything outside {varchar, char} (e.g. text, enum) — admin intervention is required for those cases rather than issuing a MODIFY that could truncate data. The sess_id(200) uniqueness concerns raised in r3102108842 and r3102108855 are intentional — XOOPS generates 32-char session IDs and the 200-byte prefix provides InnoDB/MyISAM portability under utf8mb4. Keeping as-is.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent schema-normalization tasks to the 2.5.11 → 2.7.0 upgrade patch and aligns fresh-install SQL so upgraded and new installations converge on identical index definitions (explicit prefix lengths + BTREE).
Changes:
- Add three upgrade tasks to normalize index/PK prefix definitions for
profile_field,profile_regstep, andsession. - Introduce helper methods to detect table/index existence and index prefix lengths via
information_schema. - Update fresh-install SQL for core + Profile module to match the normalized target DDL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds 3 idempotent upgrade tasks plus helpers to normalize index prefix definitions using information_schema. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module schema to use explicit index prefixes and USING BTREE to match the normalized target. |
| htdocs/install/sql/mysql.structure.sql | Updates fresh-install session table PK to use an explicit (sess_id(200)) USING BTREE definition. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@upgrade/upd_2.5.11-to-2.7.0/index.php`:
- Around line 874-887: The apply_normalizesessionpk() implementation should gate
the DROP PRIMARY KEY with indexExists($table, 'PRIMARY') and then call
execOrFail(...) to ADD PRIMARY KEY as written; keep the current split DROP/ADD
pattern in apply_normalizesessionpk(), using the same defensive pattern as
apply_normalizeprofilefieldname() and apply_normalizeprofileregstepsort(), and
add a brief operational note for release docs recommending maintenance mode (and
optional TRUNCATE session) because DROP PRIMARY KEY on `session` rebuilds the
table and may contend with session writes.
- Around line 707-727: The branch order should validate the column type before
checking length: in the code that reads $dataType and $currentLen for
profile_field.field_name, move the in_array($dataType, ['varchar','char'], true)
check above the $currentLen > 64 check so type-mismatch cases (e.g.
text/mediumtext) hit the "unexpected data type" log and return false first, and
only if $dataType is varchar/char evaluate $currentLen and log/refuse narrowing
to varchar(64) as currently implemented.
- Around line 1070-1083: indexExists currently returns false for both "index
missing" and "query failed"; change its signature to return ?bool (matching
tableExists) and return null when the information_schema query fails or the
result is not a valid mysqli_result, otherwise return true/false based on
fetchArray. Update the method named indexExists to use the same
failure-detection logic as tableExists (detect non-result or non-mysqli_result
-> return null) so callers like apply_normalizesessionpk can distinguish
permission/transient failures from a genuine missing index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04da8c91-08f9-41ba-8408-6dcbcee2042e
📒 Files selected for processing (1)
upgrade/upd_2.5.11-to-2.7.0/index.php
When profile_field.field_name is an unexpected wide type such as `text` (CHARACTER_MAXIMUM_LENGTH = 65535), the old order fired the "refusing to narrow to varchar(64)" branch first, which is misleading — the real blocker is the type mismatch, not the length. Swap the order: type mismatch is rejected first, so only legitimate `varchar`/`char` columns ever reach the narrowing guard. Behaviour is unchanged; only the logged diagnostic improves. Addresses CodeRabbit feedback on PR XOOPS#23 (r3102217357).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent schema-normalization steps to the 2.5.11 → 2.7.0 upgrade patch (and aligns fresh-install SQL) so key indexes/keys end up with consistent, portable prefix/index DDL across upgraded vs fresh installations.
Changes:
- Added 3 upgrade tasks to normalize index/prefix DDL for
profile_field.field_name,profile_regstep.sort, andsessionprimary key. - Introduced helper methods to inspect table/index presence and index prefix lengths via
information_schema. - Updated fresh-install SQL for
sessionand Profile module tables to match the normalized target DDL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds new upgrade tasks + information_schema-based helpers to detect/apply index normalization. |
| htdocs/modules/profile/sql/mysql.sql | Aligns Profile module table indexes with the normalized prefix + BTREE definitions. |
| htdocs/install/sql/mysql.structure.sql | Aligns fresh install session PK with the normalized (sess_id(200)) USING BTREE target. |
Comment on lines
+649
to
+653
| $sql = "SELECT `DATA_TYPE`, `CHARACTER_MAXIMUM_LENGTH` FROM `information_schema`.`COLUMNS`" | ||
| . " WHERE `TABLE_SCHEMA` = DATABASE()" | ||
| . " AND `TABLE_NAME` = " . $this->db->quote($table) | ||
| . " AND `COLUMN_NAME` = 'field_name' LIMIT 1"; | ||
| $result = $this->db->query($sql); |
Comment on lines
+734
to
+738
| if ('varchar' !== $dataType || $currentLen !== 64) { | ||
| $alter = "ALTER TABLE `{$table}` MODIFY `field_name` varchar(64) NOT NULL DEFAULT ''"; | ||
| if (!$this->execOrFail($alter)) { | ||
| return false; | ||
| } |
Comment on lines
+1083
to
+1086
| $result = $this->db->query($sql); | ||
| if (!$this->db->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| return false; | ||
| } |
The task header previously claimed the target was
"varchar(64) NOT NULL DEFAULT ''", but the check_ function only
validates DATA_TYPE + CHARACTER_MAXIMUM_LENGTH, so a column that is
already varchar(64) with a different nullability or default would be
reported as normalised without enforcement.
Update the header to state what is actually enforced:
- checked invariant: column is varchar(64) AND unique index uses
prefix (64)
- not checked: IS_NULLABLE, COLUMN_DEFAULT
The MODIFY ALTER still sets NOT NULL DEFAULT '' when it runs, matching
the module install SQL. Sites that have intentionally changed
nullability or default are left alone — forcing a rewrite there could
coerce NULL values to '' and surprise admins.
Addresses Copilot feedback on PR XOOPS#23 (r3102290919, r3102290942).
indexExists() previously returned false both when the index was genuinely absent and when the information_schema query failed. Callers in the three normalize tasks treated a false result as "index absent, skip the DROP and proceed to ADD" — if the query had actually failed on a table with the index present, the subsequent ADD would hit a "Duplicate key name" / "Multiple primary key" error that looked like a real DDL bug rather than an information_schema access problem. indexExists() now returns ?bool with the same semantics as tableExists(): true → index exists false → index is absent (safe to skip DROP) null → information_schema query failed (log + return false) Updated three call sites (apply_normalizeprofilefieldname, apply_normalizeprofileregstepsort, apply_normalizesessionpk) to log a clear diagnostic when the helper returns null and abort the task. Addresses Copilot feedback on PR XOOPS#23 (r3102290952) and CodeRabbit feedback (r3102217365).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent schema-normalization tasks to the 2.5.11 → 2.7.0 upgrade patch and aligns fresh-install SQL so upgraded and new installations converge on the same index definitions (explicit prefixes + BTREE).
Changes:
- Add three upgrade tasks to normalize Profile module indexes and the session table PRIMARY KEY using
information_schemachecks. - Introduce helper methods to detect table/index existence and index prefix length for idempotent upgrade behavior.
- Update fresh-install SQL for
session,profile_field, andprofile_regstepto match the normalized index DDL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds the three new upgrade tasks plus helpers that query information_schema to decide whether DDL changes are needed. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module install schema to use explicit index prefixes and USING BTREE for consistent dumps. |
| htdocs/install/sql/mysql.structure.sql | Updates fresh-install session table PRIMARY KEY to use sess_id(200) with USING BTREE. |
Comment on lines
+670
to
+671
| // Unique index `field_name` must have SUB_PART = 64 | ||
| return 64 === $this->indexPrefixLength($table, 'field_name', 'field_name'); |
Comment on lines
+879
to
+887
| * Check if the session PRIMARY KEY already uses a 200-char prefix on sess_id. | ||
| * | ||
| * @return bool true if already normalised (no action needed) | ||
| */ | ||
| public function check_normalizesessionpk(): bool | ||
| { | ||
| $table = $this->db->prefix('session'); | ||
|
|
||
| return 200 === $this->indexPrefixLength($table, 'PRIMARY', 'sess_id'); |
Bring the two remaining normalize checks up to the same strictness as check_normalizeprofileregstepsort: instead of validating a single cell (SUB_PART on one column), query the full index layout via information_schema.STATISTICS ordered by SEQ_IN_INDEX and assert the exact expected shape. - check_normalizeprofilefieldname now requires exactly one row with COLUMN_NAME='field_name', SEQ_IN_INDEX=1, SUB_PART=64, NON_UNIQUE=0. A non-UNIQUE or composite index named `field_name` no longer slips through as "already normalised". - check_normalizesessionpk now requires exactly one row with COLUMN_NAME='sess_id', SEQ_IN_INDEX=1, SUB_PART=200. A composite PRIMARY KEY containing sess_id(200) plus another column no longer slips through. All three normalize tasks are now consistent in how they verify index shape — the task is truly idempotent only when the live schema matches the canonical layout. Addresses Copilot feedback on PR XOOPS#23 (r3102364473, r3102364533).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds schema-normalization steps to the 2.5.11 → 2.7.0 upgrade patch and aligns fresh-install SQL so upgraded and new installs converge on the same index definitions (explicit prefix lengths + BTREE).
Changes:
- Add three idempotent upgrade tasks to normalize Profile module indexes and the session primary key.
- Add
USING BTREE+ explicit prefix lengths to Profile module install SQL indexes. - Update core install SQL to use a
(sess_id(200)) USING BTREEprimary key on the session table.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds upgrade tasks and helper methods to normalize index/prefix definitions via information_schema. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module schema to match the normalized index layouts (prefix lengths + BTREE). |
| htdocs/install/sql/mysql.structure.sql | Updates fresh-install session table PK to use a 200-char prefix and BTREE. |
Comment on lines
+761
to
+768
| // include STRICT_ALL_TABLES. | ||
| if ($currentLen > 64) { | ||
| $this->logs[] = sprintf( | ||
| 'profile_field.field_name is %s(%d); refusing to narrow to varchar(64) automatically. Reduce the column manually after verifying no values exceed 64 chars.', | ||
| $dataType, | ||
| $currentLen | ||
| ); | ||
| return false; |
| * @param string $indexName index name (use 'PRIMARY' for the primary key) | ||
| * @param string $columnName column name within the index | ||
| */ | ||
| private function indexPrefixLength(string $table, string $indexName, string $columnName): ?int |
Two cleanups in the normalize task area: - apply_normalizeprofilefieldname() no longer aborts the whole 2.7.0 patch when profile_field.field_name has been widened beyond 64 chars locally. It now logs a clear warning and returns true so the remaining 13 upgrade tasks can still run. The legacy UNIQUE index on the full wider column stays in place — still functional, still unique. The admin is told to narrow the column manually and re-run the upgrade to normalise the index shape. Rebuilding the UNIQUE index with prefix(64) on a wider column was considered and rejected: it would actively weaken uniqueness from "full column width" to "first 64 chars" and could allow previously blocked duplicates. - indexPrefixLength() helper is removed. After the strict-layout refactor of the three normalize checks (commit 2a806d7), no caller remains. Dead code drifts — delete it. Addresses Copilot feedback on PR XOOPS#23 (r3102436778, r3102436829).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent schema-normalization upgrade tasks and aligns fresh-install SQL so upgraded and new 2.7.0 installations converge on the same index definitions (especially prefix lengths for MySQL/InnoDB portability).
Changes:
- Add three upgrade tasks to normalize Profile module indexes and the
sessionprimary key during the 2.5.11 → 2.7.0 upgrade. - Add
information_schema-based helper methods to safely detect table/index presence before applying DDL. - Update fresh-install SQL for
session,profile_field, andprofile_regstepto match the normalized target index layouts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds upgrade tasks to normalize index prefix lengths / layouts and helper methods for table/index existence checks. |
| htdocs/modules/profile/sql/mysql.sql | Updates Profile module install DDL to use explicit index prefixes and USING BTREE. |
| htdocs/install/sql/mysql.structure.sql | Updates fresh-install session PK to use an explicit prefix and USING BTREE. |
Comment on lines
+832
to
+837
| $sql = "SELECT `COLUMN_NAME`, `SEQ_IN_INDEX`, `SUB_PART`" | ||
| . " FROM `information_schema`.`STATISTICS`" | ||
| . " WHERE `TABLE_SCHEMA` = DATABASE()" | ||
| . " AND `TABLE_NAME` = " . $this->db->quote($table) | ||
| . " AND `INDEX_NAME` = 'sort'" | ||
| . " ORDER BY `SEQ_IN_INDEX`"; |
Comment on lines
+1158
to
+1163
| /** | ||
| * Check whether an index exists on a table. | ||
| * | ||
| * Tri-state so callers can distinguish "index genuinely absent" from | ||
| * "we don't know" — mirrors tableExists(): | ||
| * true → index exists |
Codex review (HIGH severity) flagged that changing the session PK
from (sess_id) to (sess_id(200)) introduces a silent session-hijack
vector:
- sess_id remains varchar(256), so two distinct session IDs sharing
the first 200 characters are treated as the same row by the
PRIMARY KEY.
- htdocs/kernel/session.php:147 writes sessions with
ON DUPLICATE KEY UPDATE, so a collision silently overwrites the
existing session rather than erroring.
- No code in core enforces session_id <= 200 chars — the length is
governed by PHP's session.sid_length (tunable up to 256) plus the
XOOPS token generator. Admin config could push IDs past 200 bytes
and trigger collisions without any code signal.
The 2.5.11 schema had no correctness issue with PRIMARY KEY (sess_id);
the prefix change was a "while we're here" consistency pass that
introduced real risk to solve a non-problem.
Changes:
- Remove task normalizesessionpk from the task list and numbering
(cleancache renumbered 14 → 13).
- Delete check_normalizesessionpk() and apply_normalizesessionpk().
- Revert htdocs/install/sql/mysql.structure.sql so fresh installs
keep PRIMARY KEY (sess_id) on the full column.
Addresses Codex finding #1 and supersedes the earlier dismissals of
Copilot r3101820136, r3101820175, r3102108842, r3102108855.
Codex review (MEDIUM severity) flagged that the normalize task claimed to enforce "varchar(64) NOT NULL DEFAULT ''" but the check only looked at DATA_TYPE + CHARACTER_MAXIMUM_LENGTH, and the apply path only ran the MODIFY when type/length differed. A site with varchar(64) NULL or a different default would be reported as normalised without actually converging to the documented target. - check_normalizeprofilefieldname() now reads IS_NULLABLE and COLUMN_DEFAULT in addition to type/length, and only returns true when all four match. Uses strict comparison so a NULL default is correctly rejected (PHP (string) null == '' would collapse them). - apply_normalizeprofilefieldname() now triggers the MODIFY whenever any of the four attributes diverge, not just type/length. When the rewrite includes nullability or default changes, a warning is logged identifying the previous state so admins can audit — in particular, NULL values in a previously nullable column are coerced to '' by the MODIFY, which is a visible data change worth surfacing. - Task header comment restored to claim the full four-attribute invariant, now that it is actually enforced. Addresses Codex finding #2 and supersedes the docblock-honesty compromise in a9ab21f.
check_normalizeprofileregstepsort() previously validated columns and
prefix lengths but not NON_UNIQUE. A UNIQUE index with the same column
order and prefixes (e.g. UNIQUE KEY sort (step_order, step_name(100)))
would incorrectly pass as normalised — the canonical form is a plain
KEY, not UNIQUE.
Extend the STATISTICS query to SELECT NON_UNIQUE and assert it is 1
on both rows of the index. All three normalize checks now verify the
full expected shape of their index at identical strictness:
- field_name: (field_name, 1, 64, NON_UNIQUE=0) — UNIQUE
- sort: (step_order, 1, NULL, NON_UNIQUE=1),
(step_name, 2, 100, NON_UNIQUE=1) — plain KEY
Addresses Codex finding #3 and Copilot r3102515513.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@upgrade/upd_2.5.11-to-2.7.0/index.php`:
- Around line 670-678: The check fails on MariaDB 10.2.7+ because
information_schema.COLUMNS.COLUMN_DEFAULT returns quoted string literals (e.g.
"''"), so update the checks in check_normalizeprofilefieldname() to strip
surrounding single quotes before comparing defaults (use trim on the fetched
default value, e.g. trim($row[3], "'")); apply the same normalization in the
apply_/divergence-detection logic that compares $currentDefault around line ~795
so both the existence check and the MODIFY/default-detection compare unquoted
values consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22d29cda-257d-4932-9277-6d1c9b59f45c
📒 Files selected for processing (1)
upgrade/upd_2.5.11-to-2.7.0/index.php
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes Profile module schema/index definitions between fresh installs and upgrades in the 2.5.11 → 2.7.0 upgrade patch, adding idempotent tasks to enforce canonical column/index shapes.
Changes:
- Add two upgrade tasks to normalize
profile_field.field_name(column attributes + UNIQUE index prefix) andprofile_regstep.sort(composite non-unique index layout). - Add helper methods to detect table/index existence via
information_schemawith tri-state results. - Update Profile module fresh-install MySQL DDL to match the normalized index definitions (
USING BTREE, explicit prefix lengths).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds the new normalization tasks and supporting helpers to the 2.7.0 upgrade patch. |
| htdocs/modules/profile/sql/mysql.sql | Aligns install-time index DDL with the canonical normalized definitions used by the upgrade tasks. |
| $dataType, | ||
| $currentLen, | ||
| $isNullable ? 'NULL' : 'NOT NULL', | ||
| null === $currentDefault ? 'NULL' : "'{$currentDefault}'" |
Comment on lines
+796
to
+813
| if ($columnDiverges) { | ||
| // Log the specific divergence so admins can audit what changed — | ||
| // especially important when nullability or default is being | ||
| // rewritten (NULL values are coerced to '' by the MODIFY). | ||
| if ($isNullable || '' !== $currentDefault) { | ||
| $this->logs[] = sprintf( | ||
| 'profile_field.field_name is %s(%d) %s DEFAULT %s — rewriting to varchar(64) NOT NULL DEFAULT \'\'. Any existing NULL values will be coerced to empty strings.', | ||
| $dataType, | ||
| $currentLen, | ||
| $isNullable ? 'NULL' : 'NOT NULL', | ||
| null === $currentDefault ? 'NULL' : "'{$currentDefault}'" | ||
| ); | ||
| } | ||
| $alter = "ALTER TABLE `{$table}` MODIFY `field_name` varchar(64) NOT NULL DEFAULT ''"; | ||
| if (!$this->execOrFail($alter)) { | ||
| return false; | ||
| } | ||
| } |
Address three correctness/safety gaps in the profile_field normalize task flagged by CodeRabbit and Copilot reviews of 0045e0f, and confirmed by Codex: - MariaDB 10.2.7+ wraps string-literal COLUMN_DEFAULT values in single quotes — a column DEFAULT '' comes back as the 2-char string "''" instead of an empty string. The direct `'' !== $row[3]` check would therefore permanently mark any MariaDB 10.2.7+ install as "not normalised" and attempt a no-op MODIFY on every upgrade run. A new normaliseColumnDefault() helper strips surrounding quotes before comparing, so the check works uniformly across MySQL and MariaDB. - $currentDefault is interpolated into upgrade log messages that the upgrader renders as HTML (joined with <br>). A crafted column default containing HTML/JS markup would render in the upgrade UI. Wrap the dynamic values in htmlspecialchars(ENT_QUOTES, 'UTF-8') so the log displays them verbatim. - When field_name is nullable and the UNIQUE index still exists, the column can legally hold multiple NULL rows (SQL's NULL != NULL rule). MODIFY ... NOT NULL DEFAULT '' coerces those NULLs into '', which then violates the UNIQUE constraint and aborts the ALTER with a low-signal duplicate-key error. A pre-flight COUNT(*) now detects this case before attempting the MODIFY and refuses with an actionable message naming the exact row count so the admin can resolve the conflict and re-run. Addresses CodeRabbit r3102684017 and Copilot r3102721783, r3102721820.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds schema-normalization steps to the 2.5.11 → 2.7.0 upgrader so the Profile module’s profile_field.field_name definition and the profile_regstep.sort index converge to a consistent, canonical DDL across upgraded vs fresh installs.
Changes:
- Added two idempotent upgrade tasks to normalize the
profile_field.field_namecolumn + unique index, and theprofile_regstep.sortindex layout. - Added helper utilities to query
information_schema(tri-statetableExists()/indexExists()plus default normalization). - Updated the Profile module’s MySQL install schema to match the target index definitions (
USING BTREE, explicit prefix lengths).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds the upgrade tasks and helper methods to enforce canonical Profile schema/index definitions during upgrade. |
| htdocs/modules/profile/sql/mysql.sql | Aligns fresh-install schema with the normalized index definitions used by the upgrade tasks. |
Comment on lines
+671
to
+679
| if (!$row | ||
| || 'varchar' !== strtolower((string) $row[0]) | ||
| || 64 !== (int) $row[1] | ||
| || 'NO' !== strtoupper((string) $row[2]) | ||
| || '' !== self::normaliseColumnDefault($row[3]) // reject NULL default | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
apply_normalizeprofilefieldname() returns true when profile_field.field_name has been widened beyond 64 chars — it logs a "resolve manually" warning and defers the normalise, so the rest of the 2.7.0 upgrade can proceed. check_normalizeprofilefieldname() however still required CHARACTER_MAXIMUM_LENGTH = 64, so on widened-column installs it kept returning false. Since the XoopsUpgrade framework drives patch applicability solely from check_* results, the patch showed as permanently "not applied" even after apply_ ran and succeeded. Mirror the pattern already used by check_/apply_cleancache: apply_ sets a session flag on the skip-with-warning path; check_ reads the flag and returns true when it's set. The widened column is then reported as resolved-for-this-session and the upgrade queue can complete, while admins still see the warning in the upgrade log. A new session (fresh login) clears the flag so the advisory is re-surfaced on every subsequent upgrade run — consistent with how the framework surfaces operational state. Addresses Copilot r3102874899.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds idempotent upgrade tasks to the 2.5.11 → 2.7.0 upgrader to normalize Profile module schema details so fresh installs and upgraded sites converge on identical index/column DDL.
Changes:
- Add two new upgrade tasks to normalize
profile_field.field_name(column definition + unique index prefix) andprofile_regstep.sort(composite non-unique index layout). - Introduce upgrade helpers to query
information_schemaand normalize default-literal comparisons across MySQL/MariaDB. - Update Profile module fresh-install SQL to match the normalized index definitions (
USING BTREE, explicit prefix lengths).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds schema-normalization tasks and supporting information_schema helpers to enforce canonical Profile table/index layouts during upgrade. |
| htdocs/modules/profile/sql/mysql.sql | Aligns install-time index definitions with the canonical shapes enforced by the upgrade tasks. |
Comment on lines
+668
to
+674
| // Skip-with-warning path was already taken this session for a widened | ||
| // column. apply_ logged the advisory and deferred to manual action — | ||
| // treat the task as settled so the overall patch can complete rather | ||
| // than looping forever on check_ returning false. | ||
| if (!empty($_SESSION[$this->widenedFieldNameSkipKey])) { | ||
| return true; | ||
| } |
The session-flag short-circuit added in 69d75e0 was unconditional: once apply_ had taken the skip-with-warning path, check_ would return true for the rest of the session regardless of the actual column state. That meant an admin who followed the logged advice and manually narrowed profile_field.field_name back to 64 chars within the same upgrade session would see the task keep reporting "already applied" and never actually rebuild the unique index — they would have to log out or wait for a new session before the upgrade would converge. Move the column-metadata fetch above the flag check and honour the flag only while the original trigger condition (CHARACTER_MAXIMUM_LENGTH > 64) still holds. If the admin has narrowed the column, the flag is stale — clear it and fall through to the normal canonical-shape check so apply_ can rebuild the index immediately. No extra cost: the information_schema query was already performed further down; it has just been moved up. Addresses Copilot r3102996202.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@upgrade/upd_2.5.11-to-2.7.0/index.php`:
- Around line 1160-1173: Add a short docblock above the normaliseColumnDefault
method documenting its intended contract and limitation: mark it `@internal` and
state it only unwraps simple single-quoted literals (used here to detect the
empty string literal) and does not unescape embedded quotes (e.g. "'a''b'" will
return "a''b" not "a'b"); this makes the expected behaviour explicit for future
callers of normaliseColumnDefault.
- Around line 686-691: Add a one-line inline comment clarifying that when the
introspection query succeeds but fetchRow() returned false ($row === false) it
means the column was actually dropped between sessions, so clearing the
$_SESSION[$this->widenedFieldNameSkipKey] flag is intentional and harmless;
place this comment adjacent to the
unset($_SESSION[$this->widenedFieldNameSkipKey]) line in the same block that
checks $row and $this->widenedFieldNameSkipKey to make the intent clear to
future readers.
- Around line 830-845: The COUNT(*) probe currently ignores a failed query and
falls through to the ALTER which can produce misleading duplicate-key errors;
update the block around $countResult to treat a non-result (i.e. when
$this->db->query($countSql) returns null/false or
$this->db->isResultSet($countResult) is false or not an instance of
\mysqli_result) as a hard failure: append a descriptive message to $this->logs
explaining that the pre-flight COUNT probe failed (include $countSql or indicate
probe failure), and return false immediately so the upgrade aborts rather than
attempting the MODIFY ... NOT NULL DEFAULT '' step. Ensure you reference
$countResult, $this->db->query, $this->db->isResultSet and \mysqli_result when
implementing the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbb95948-da7e-4060-bf9f-5ea315442741
📒 Files selected for processing (1)
upgrade/upd_2.5.11-to-2.7.0/index.php
Comment on lines
+686
to
+691
| if (!empty($_SESSION[$this->widenedFieldNameSkipKey])) { | ||
| if ($row && (int) $row[1] > 64) { | ||
| return true; // still widened — defer to manual action | ||
| } | ||
| unset($_SESSION[$this->widenedFieldNameSkipKey]); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stale-flag revalidation on $row === false is a harmless side effect — worth a one-line sanity note.
When the introspection query succeeds but fetchRow() returns false (row genuinely missing — the column was dropped between sessions), the flag is cleared and the subsequent !$row check at Line 693 returns false anyway. Behaviour is correct, but a brief inline comment that $row === false here means "column is gone" would save the next reader a trace-through. No code change required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd_2.5.11-to-2.7.0/index.php` around lines 686 - 691, Add a one-line
inline comment clarifying that when the introspection query succeeds but
fetchRow() returned false ($row === false) it means the column was actually
dropped between sessions, so clearing the
$_SESSION[$this->widenedFieldNameSkipKey] flag is intentional and harmless;
place this comment adjacent to the
unset($_SESSION[$this->widenedFieldNameSkipKey]) line in the same block that
checks $row and $this->widenedFieldNameSkipKey to make the intent clear to
future readers.
Comment on lines
+1160
to
+1173
| private static function normaliseColumnDefault(mixed $default): ?string | ||
| { | ||
| if (null === $default) { | ||
| return null; | ||
| } | ||
| $value = (string) $default; | ||
| if (strlen($value) >= 2 | ||
| && str_starts_with($value, "'") | ||
| && str_ends_with($value, "'") | ||
| ) { | ||
| return substr($value, 1, -1); | ||
| } | ||
| return $value; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
normaliseColumnDefault() handles the target case correctly; edge-case caveat worth a line in the docblock.
For the narrow purpose here (comparing against ''), this is exactly right — MariaDB 10.2.7+ returns '' as the 2-char literal '', which unwraps cleanly to the empty string. One edge case that's fine to leave unhandled but worth documenting: if a DEFAULT contains embedded single quotes (e.g. MariaDB renders DEFAULT "a'b" as 'a''b'), the helper returns a''b rather than a'b — unimportant for the current comparison against '', but a future caller comparing more exotic defaults would be mildly surprised. A docblock @internal note limiting the helper to empty/simple literal comparisons would make the contract explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd_2.5.11-to-2.7.0/index.php` around lines 1160 - 1173, Add a short
docblock above the normaliseColumnDefault method documenting its intended
contract and limitation: mark it `@internal` and state it only unwraps simple
single-quoted literals (used here to detect the empty string literal) and does
not unescape embedded quotes (e.g. "'a''b'" will return "a''b" not "a'b"); this
makes the expected behaviour explicit for future callers of
normaliseColumnDefault.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the 2.5.11 → 2.7.0 upgrade patch and the Profile module’s fresh-install schema so that profile_field.field_name and profile_regstep.sort index definitions converge to the same canonical DDL across new installs and upgraded sites.
Changes:
- Add two new idempotent upgrade tasks to normalize Profile module column/index definitions via
information_schemachecks and targetedALTER TABLEoperations. - Add helper utilities in the 2.7.0 upgrade patch (
normaliseColumnDefault(),tableExists(),indexExists()) to support strict, re-runnable schema verification. - Update Profile module
mysql.sqlto match the canonical index prefix + BTREE definitions used by the upgrader.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| upgrade/upd_2.5.11-to-2.7.0/index.php | Adds two upgrade tasks to normalize Profile schema (column attributes + index prefix/layout) with strict information_schema validation and idempotent apply logic. |
| htdocs/modules/profile/sql/mysql.sql | Aligns fresh-install schema with the normalized index definitions (explicit prefixes + BTREE). |
The nullable-to-NOT-NULL pre-flight added in 4f593a2 silently skipped the collision guard when the COUNT(*) probe itself failed — if $this->db->query() returned anything other than a valid mysqli_result, the whole block was bypassed and control fell through to the MODIFY ... NOT NULL DEFAULT '' statement. The MODIFY would then fail with a duplicate-key error in exactly the case the pre-flight was added to catch, producing the low-signal SQL error the guard was supposed to replace. Treat query failure as a hard error: log the DB error via the standard _DB_QUERY_ERROR template and return false. Mirrors the pattern used in every other information_schema probe in this file. Addresses CodeRabbit r3103056876.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add two idempotent upgrade tasks to the 2.5.11 → 2.7.0 patch that
standardise index DDL across fresh and upgraded installations:
varchar(64) NOT NULL DEFAULT '' and its
UNIQUE index uses explicit prefix (64)
USING BTREE.
KEY (step_order, step_name(100)) USING BTREE,
non-UNIQUE.
Each check queries information_schema.COLUMNS / STATISTICS and asserts
the full expected layout (data type, length, nullability, default,
column order, prefix lengths, and NON_UNIQUE), so the tasks are both
strict about what they accept as "already normalised" and safe to
re-run against installations at any intermediate state. Two private
helpers (tableExists, indexExists) back the checks — both return ?bool
so callers can distinguish "object absent" from "information_schema
query failed" and log/abort appropriately rather than silently skip.
Edge cases:
isn't installed or when field_name has been widened beyond 64 chars
locally, so the remaining 2.7.0 upgrade tasks still run.
documented target; a warning is logged explicitly because NULL
values are coerced to '' by the MODIFY — admin-auditable.
Fresh-install SQL updated to converge on the same final state:
profile_field UNIQUE KEY field_name → (field_name(64)) USING BTREE.
profile_regstep KEY sort → (step_order, step_name(100)) USING BTREE.
Index sizes under utf8mb4 (4 bytes/char) stay within the 767-byte
InnoDB 5.7 default limit, keeping the schema portable to InnoDB
without innodb_large_prefix:
profile_field.field_name : 644 = 256 bytes
profile_regstep.sort : 2 + 1004 = 402 bytes
The session PRIMARY KEY change initially included in this branch has
been reverted (commit 0498b0d). Reducing sess_id to a prefix(200)
index while the column stayed varchar(256) would have allowed two
distinct session IDs sharing the first 200 chars to collide under
ON DUPLICATE KEY UPDATE in kernel/session.php, silently overwriting
session data whenever PHP's session.sid_length was tuned beyond the
defaults. The 2.5.11 session PK had no correctness issue to fix.
Summary by CodeRabbit