fix(migration): tags name widening was on wrong table - #236
Open
DocArmoryTech wants to merge 4 commits into
Open
fix(migration): tags name widening was on wrong table#236DocArmoryTech wants to merge 4 commits into
DocArmoryTech wants to merge 4 commits into
Conversation
20250829000000_GinormousTags ("extend name field to 255 length, matching
MISP") operates on a table called `tags`:
$this->table('tags')->removeIndex('name')->save();
$this->table('tags')->changeColumn('name', 'string', ['limit' => 255])->update();
$this->table('tags')->addIndex('name', ['limit' => 191])->save();
The Tags plugin's table is `tags_tags` -- TagsTable::initialize() calls
setTable('tags_tags'), and plugins/Tags/config/Migrations/20210831121348_TagSystem.php
creates it. No migration anywhere creates a bare `tags` table.
So the widening never happened, and an empty `tags` table was created as a
side effect, Phinx's ->save() creating a table that does not exist.
Confirmed on an instance at main:
+------------+-------------+--------------------------+
| table_name | column_name | character_maximum_length |
+------------+-------------+--------------------------+
| tags | name | 255 |
| tags_tags | name | 191 |
+------------+-------------+--------------------------+
Adds a new migration rather than correcting the original, which has already
run everywhere and will not run again. It widens tags_tags.name and drops
the stray table, guarded on it being empty.
The index on tags_tags.name is UNIQUE, created that way by TagSystem, and
getExistingTag() together with every "create tag if absent" path depends on
it. The original re-added the index without `unique`, harmless only because
it was acting on an empty table nothing reads -- applying it verbatim to
tags_tags would silently have dropped the constraint. Preserved here.
The 191-byte prefix is kept, as intended originally: utf8mb4 at 255
characters is 1020 bytes, past the 767-byte index limit on older InnoDB row
formats. Uniqueness is therefore enforced on the first 191 characters.
down() narrows back only when no name exceeds 191 characters, so a rollback
cannot truncate tag names.
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
2 participants
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.
20250829000000_GinormousTags- "extend name field to 255 length, matching MISP" - never widened the column it was aiming at (I think), and created an empty table as a side effect.Why
It operates on a table called
tags:The Tags plugin's table is
tags_tags—TagsTable::initialize()callssetTable('tags_tags'), andplugins/Tags/config/Migrations/20210831121348_TagSystem.phpcreates it. Nomigration anywhere creates a bare
tagstable, so Phinx's->save()createdone.
Changes
A new/repeat migration, since the original has already run everywhere and will not run again. It widens
tags_tags.nameto 255 and drops the straytagstable, guarded on it being empty.A second bug in the original, worth flagging
The index on
tags_tags.nameis UNIQUE —TagSystemcreates it with['unique' => true], andgetExistingTag()plus every "create the tag if itdoes not exist" path depends on it.
The original migration re-adds the index without
unique; harmless because it was on an empty table nothing reads. Fixing by correcting the table name could silently drop the uniqueness constraint on the live table.