Skip to content

[2.x] fix(messages): shorten mention pivot table and column names - #4960

Closed
imorland wants to merge 2 commits into
2.xfrom
im/messages-shorter-identifiers
Closed

[2.x] fix(messages): shorten mention pivot table and column names#4960
imorland wants to merge 2 commits into
2.xfrom
im/messages-shorter-identifiers

Conversation

@imorland

Copy link
Copy Markdown
Member

Found while validating the reworked CI matrix in #4959, which runs prefixed jobs at the maximum prefix length the installer accepts. Two of those jobs failed:

SQLSTATE[42000]: 1059 Identifier name
'flarum_ci_dialog_message_mentions_group_dialog_message_id_foreign' is too long

Changes proposed in this pull request:

DatabaseConfig accepts table prefixes of up to 10 characters. This extension's mention pivot tables generate foreign key names that cannot survive one:

dialog_message_mentions_group_dialog_message_id_foreign   55 chars
+ a 10-character prefix                                   65 chars
MySQL / MariaDB limit                                      64
PostgreSQL limit                                           63 bytes

So on MySQL or MariaDB, a bundled extension cannot be enabled on any installation using a 10-character prefix, and the installer permits exactly that. On PostgreSQL it appears to succeed, because PostgreSQL truncates over-long identifiers silently rather than erroring — the constraint is created under a name nobody chose, and two such names colliding after truncation would be the next failure.

Auditing every migration in core and the bundled extensions — 181 files, driven through Laravel's own createIndexName rather than by pattern-matching the convention — put all seven of the longest identifiers in this extension:

55  dialog_message_mentions_group_dialog_message_id_foreign
55  dialog_message_mentions_group_mentions_group_id_foreign
54  dialog_message_mentions_user_dialog_message_id_foreign
54  dialog_message_mentions_post_dialog_message_id_foreign
53  dialog_message_mentions_user_mentions_user_id_foreign
53  dialog_message_mentions_post_mentions_post_id_foreign
53  dialog_message_mentions_tag_dialog_message_id_foreign
45  post_mentions_group_mentions_group_id_foreign            <- flarum/mentions

This renames the four pivot tables and their columns so the generated names fit:

before after
dialog_message_mentions_user message_mentions_user
dialog_message_mentions_post message_mentions_post
dialog_message_mentions_group message_mentions_group
dialog_message_mentions_tag message_mentions_tag
dialog_message_id message_id
mentions_user_id / mentions_post_id / mentions_group_id / mentions_tag_id user_id / post_id / group_id / tag_id

The longest identifier here becomes 41 characters, so this extension is no longer the binding constraint — flarum/mentions is, at 45. The maximum safe prefix across all supported drivers goes from 8 to 18, which finally clears the 10 the installer allows, with room to spare.

The tables and columns are private to the extension: they are referenced only by these migrations and the four belongsToMany calls in DialogMessage, all updated here. Nothing in the JS, the API resources, or the tests refers to them.

Why rename rather than name the keys explicitly

Passing an explicit name to foreign() looked like the smaller change, but it is wrong. prefix_indexes is consulted only inside Blueprint::createIndexName(), which runs only when no name is supplied — so an explicit name is never prefixed. Foreign key constraint names must be unique per database, not per table, which is exactly the shared-database case prefixes exist to serve. Two installations sharing one database would then collide:

ERROR 1826 (HY000): Duplicate foreign key constraint name 'dmm_group_msg_foreign'

Verified against MySQL 8.4. Shortening the generated names keeps prefixing intact and avoids trading a length bug for a collision bug.

Existing installations

A release candidate installation that already enabled this extension has the old tables. 2026_08_22_000000_rename_message_mentions_tables.php renames them and their columns where present, and no-ops where the new names already exist, so a fresh install and an upgraded one converge. An installation using a 10-character prefix cannot have created the old tables in the first place, since that is the failure this fixes.

Reviewers should focus on:

  • Whether message_mentions_* is the right name given the parent table is dialog_messages. Strictly the convention would give dialog_message_mentions_*, which is what caused this. The alternative is renaming dialog_messages to messages as well, which is a much larger change for no additional headroom.
  • The rename migration on each driver. It renames columns that are part of a composite primary key and carry a foreign key. MySQL, MariaDB and PostgreSQL all handle RENAME COLUMN natively; SQLite needs 3.25 or newer, which is below the declared SQLITE_MINIMUM of 3.35.
  • Dropping these keys in future migrations now needs the conventional name derived from the new table and column names. Nothing does today.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered? — explicit key names were tried and rejected for the collision above; lowering the documented prefix maximum to 8 was rejected as it constrains every installation because of one extension.
  • For core PRs, does this need to be in core, or could it be in an extension? — bundled extension only.
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation — no frontend changes.
  • Frontend changes: tests are green — no frontend changes.
  • Frontend changes: tests have been added — no frontend changes.
  • Backend changes: tests are green — CI on this branch; the prefixed jobs on [2.x] ci: rebuild the backend matrix around declared database support #4959 are the real check, once rebased.
  • Backend changes: tests have been added — the coverage this needs is the prefixed matrix jobs in [2.x] ci: rebuild the backend matrix around declared database support #4959 plus the flarum/testing check, both following separately.
  • Where applicable, changes are suitable for all supported database drivers — that is the substance of the change; see the note on RENAME COLUMN above.
  • Core developer confirmed locally this works as intended.
  • The description above is written by me and describes what this pull request actually does.

@imorland
imorland requested a review from a team as a code owner August 21, 2026 21:49
@imorland

Copy link
Copy Markdown
Member Author

Closing — the approach here cannot work, and #4961 replaces it.

This tried to raise the maximum usable table prefix by shortening the identifiers these migrations generate. Migrations are immutable and a new installation replays all of them, so the ceiling is set by migration history, not by the current schema. A later rename cannot stop the original create migration from generating a 55-character foreign key name on every new install, so the ceiling stays where it is regardless. Getting the benefit would have meant editing migrations that have already run on other installations, which is not something we do.

The defect is real: the installer accepts a 10-character prefix that no driver can actually accommodate. #4961 fixes it the only way that works without touching migration history — by deriving the limit per driver (9 on MySQL and MariaDB, 8 on PostgreSQL, unrestricted on SQLite) and enforcing it in the installer and at boot, so the failure is stated up front instead of surfacing as MySQL error 1059 from inside a migration.

The audit that came out of this is still worth recording: flarum/messages owns the seven longest identifiers in core and the bundled extensions, which is why the ecosystem-wide prefix limit is as low as it is. A check in flarum/testing will follow so a future migration introducing a longer name fails a test suite rather than someone's forum.

@imorland imorland closed this Aug 21, 2026
@imorland
imorland deleted the im/messages-shorter-identifiers branch August 21, 2026 22:03
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