Skip to content

[2.x] [Tags] Drop legacy unused background columns from tags table#4529

Open
Abdooo2235 wants to merge 2 commits intoflarum:2.xfrom
Abdooo2235:fix/4527-tags-remove-legacy-background-columns
Open

[2.x] [Tags] Drop legacy unused background columns from tags table#4529
Abdooo2235 wants to merge 2 commits intoflarum:2.xfrom
Abdooo2235:fix/4527-tags-remove-legacy-background-columns

Conversation

@Abdooo2235
Copy link
Copy Markdown

@Abdooo2235 Abdooo2235 commented Apr 7, 2026

Summary

This PR removes two legacy columns from the tags table that are no longer used by current tags flows:

  • background_path
  • background_mode

Fixes #4527.

What changed

  • Added migration using Migration::dropColumns() to remove both legacy columns.
  • Removed backgroundUrl and backgroundMode from the tags API resource.
  • Removed stale background accessors from the frontend Tag model.
  • Removed stale references from the tag factory and model docblocks.
  • Added an integration schema test asserting those columns are absent after migrations.

Backward compatibility

  • This is an intentional clean break for 2.x pre-stable, aligned with review feedback.
  • Existing data in removed columns is dropped as part of schema cleanup.

Scope note

An unrelated local change in extensions/tags/js/tsconfig.json was intentionally excluded from this PR to keep the fix focused.

Testing

  • Added: extensions/tags/tests/integration/api/tags/SchemaTest.php
  • Local test execution in this environment is blocked because phpunit is not available (sh: 1: phpunit: not found).

@Abdooo2235 Abdooo2235 requested a review from a team as a code owner April 7, 2026 23:20
Copy link
Copy Markdown
Contributor

@DavideIadeluca DavideIadeluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up the issue I opened. Have you considered using Flarum’s migration helper? The migration in the PR feels a bit more complex than necessary and could likely be simplified along these lines:

use Flarum\Database\Migration;

return Migration::dropColumns('tags', [
    'background_path' => [..],
    'background_mode' => [..],
]);

Note that with Migration::dropColumns() you still need to specify the column constraints and metadata, similar to what you would define in the up migration.

Another thing that stood out in the PR is that the attributes are still present. If background_mode and background_path are no longer required, wouldn’t this effectively become dead code? It might be better to remove them entirely and go for a clean break.

Since Flarum 2 hasn’t been tagged as stable yet, a breaking change like this should still be acceptable without creating backwards compatibility concerns.

Comment thread extensions/tags/src/Api/Resource/TagResource.php Outdated
@Abdooo2235
Copy link
Copy Markdown
Author

@DavideIadeluca thanks a lot for the thoughtful review and for opening this issue in the first place.

Great call on both points. I updated the PR accordingly:

  • Switched the migration to use Migration::dropColumns() with full column definitions.
  • Removed the leftover backgroundUrl / backgroundMode attributes from the API resource.
  • Removed the corresponding stale accessors from the frontend Tag model so there’s no dead code path left.

I agree with your reasoning about 2.x still being pre-stable, so I went with the clean break approach here.

I also reran the tags integration suite after the update:
composer test:integrationOK (127 tests, 213 assertions).

Really appreciate the guidance.

@Abdooo2235 Abdooo2235 requested a review from luceos April 19, 2026 11:34
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.x & 2.x] [Tags] Unused Columns in tags table

3 participants