refactor: convert FQCN morph types to aliases#187
Conversation
… convert morph types to aliases
…ases across modules
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaced stored and queried polymorphic model type FQCNs with Eloquent morph aliases by using Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php (2)
62-77: Consider wrapping the migration in a transaction for atomicity.The migration updates multiple tables sequentially. If it fails midway, the database will be left in a partially migrated state where some tables have aliases and others still have FQCNs. This could cause runtime errors since the application code expects consistent morph types.
♻️ Proposed fix to add transaction
public function up(): void { + DB::transaction(function (): void { foreach (self::TABLES as $table => $column) { if (! Schema::hasTable($table)) { continue; } foreach (self::MORPH_MAP as $alias => $fqcns) { foreach ($fqcns as $fqcn) { DB::table($table) ->where($column, $fqcn) ->update([$column => $alias]); } } } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php` around lines 62 - 77, Wrap the entire up() migration work (the loops over self::TABLES and self::MORPH_MAP) in a DB transaction so the multi-table updates are atomic: either use DB::transaction(...) around the existing logic or explicitly call DB::beginTransaction(), run the DB::table(...)->where(...)->update(...) loops, then DB::commit(), and on exception DB::rollBack() and rethrow; keep identifiers TABLES, MORPH_MAP, up(), Schema::hasTable and DB::table(...) unchanged so the same update logic runs inside the transaction.
79-102: Thedown()method should also be wrapped in a transaction for consistency.Same reasoning as the
up()method—partial rollback would leave inconsistent morph types.♻️ Proposed fix
public function down(): void { + DB::transaction(function (): void { $reverseMap = [ 'user' => User::class, // ... rest of the map ]; foreach (self::TABLES as $table => $column) { // ... } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php` around lines 79 - 102, Wrap the down() migration in a DB transaction to avoid partial updates: open a DB::transaction closure around the existing loop that iterates self::TABLES and applies the $reverseMap updates so all updates either commit or rollback together. Ensure you keep the Schema::hasTable checks and the inner DB::table(...)->where(...)->update(...) calls unchanged inside the transaction; reference the down() method, self::TABLES, $reverseMap, Schema::hasTable and DB::table to locate where to wrap the transaction.database/migrations/2026_03_21_200000_cleanup_orphan_users.php (1)
10-115: Wrap the entire migration in a transaction for data integrity.This migration performs complex multi-phase operations across numerous tables. A failure in Phase 2 or 3 would leave the database in an inconsistent state where some data has been merged/reassigned but orphan records remain partially deleted.
♻️ Proposed fix
public function up(): void { + DB::transaction(function (): void { // Build orphan→kept user mapping from dedup pairs $mappings = DB::select(' ... '); // ... rest of migration logic ... DB::table('users') ->whereIn('id', $orphanUserIds) ->delete(); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php` around lines 10 - 115, The up() migration must run inside a DB transaction to ensure all phases are atomic; wrap the existing logic in up() in a transaction (either by returning DB::transaction(function () { ... }) around the current body or by calling DB::beginTransaction(), running the existing steps, DB::commit() and DB::rollBack() in a try/catch that rethrows on error). Ensure you reference the same function up() and keep all current DB::* calls (e.g., DB::select, DB::table(...)->update/delete) inside the transactional block so any exception rolls back all changes.database/migrations/2026_03_21_192332_deduplicate_external_identities.php (1)
14-29: Length-based duplicate detection works correctly but relies on implicit assumptions.The query correctly identifies old vs new identity rows for the User model type (double-backslash variant = 25 characters, single-backslash = 22 characters, mathematically verified). However, the approach is fragile because it:
- Uses magic numbers (25, 22) without validation that only these lengths exist in the data
- Lacks explicit documentation of the FQCN pattern assumption
- Would break if the User namespace depth changed
Since external_identities currently only stores User model types, the detection is accurate in practice. Consider adding a comment documenting the expected FQCN variants and their lengths, or refactoring to match explicit patterns like
'He4rt%User%Models%User'for better maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php` around lines 14 - 29, The SELECT in the migration uses magic lengths (25/22) to distinguish User FQCN variants—replace the fragile length checks with explicit pattern matching or document the assumption: update the query that populates $duplicates (in this migration) to use explicit model_type patterns (e.g. LIKE patterns matching the current FQCN variants for User) instead of LENGTH(...) and add a short inline comment explaining the expected FQCN variants and why those patterns are used so future namespace changes won’t break silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.php`:
- Around line 51-59: The tenant lookup using ExternalIdentity->first() can
return null and the subsequent dereference of $this->tenantProvider->tenant_id
in the memberProvider query will crash; update the tenant query to either call
firstOrFail() on ExternalIdentity (so it throws a clear exception) or explicitly
check for null after the first() call and handle it (e.g., throw a descriptive
exception or return early) before building $this->memberProvider; refer to the
ExternalIdentity query that assigns $this->tenantProvider and the subsequent
$this->memberProvider assignment when applying the fix.
In
`@app-modules/identity/database/migrations/2025_11_02_171945_add_polymorphic_fields_to_providers_table.php`:
- Line 18: The conversion migration that backfills class aliases must also
convert existing values in the providers.model_type column (not just handle
fresh installs) — update the alias conversion migration to include the providers
table and run a safe UPDATE on providers setting model_type = <alias> where
model_type = <FQCN> (handle NULLs and only touch known mappings), referencing
the migration that adds polymorphic fields
(add_polymorphic_fields_to_providers_table) and the providers.model_type column
so you update legacy FQCN entries to their alias equivalents as part of the
migration.
In `@app/Providers/AppServiceProvider.php`:
- Around line 44-48: The configureDatabase call enables
Relation::requireMorphMap but the Economy module never registers polymorphic
types, causing ClassMorphViolationException for models like Wallet and
Transaction; in the Economy service provider (or where models are bootstrapped)
add a Relation::morphMap registration that maps the morph keys to the concrete
classes (e.g., map 'wallet' => Wallet::class and 'transaction' =>
Transaction::class), and audit polymorphic usages (morphTo, morphMany(owner),
etc.) to ensure each morph name used by Wallet, Transaction or related models
has a corresponding entry in the morphMap so Relation::requireMorphMap is
satisfied.
In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php`:
- Around line 44-48: The increment on the characters table can silently no-op if
the kept user has no character row; instead, before calling
DB::table('characters')->where('user_id',
$dup->new_user_id)->increment('experience', $oldXp), check for an existing
character for $dup->new_user_id (e.g., query for a row or use an upsert), and if
none exists create a new character record with the appropriate initial fields
including 'experience' set to $oldXp, otherwise increment as before; also emit a
warning/log entry referencing $dup->new_user_id and $oldXp when creating the
missing character so reviewers can audit the migration.
In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php`:
- Around line 32-39: The PostgreSQL-only cast 'users.id::text' inside the
whereNotExists closure causes portability issues; update the comparison in the
closure used to build $orphanUserIds to remove the ::text cast and compare
columns directly (e.g., use whereColumn('external_identities.model_id',
'users.id') or DB::raw('users.id')) so the query is grammar-agnostic, and ensure
external_identities.model_id is treated as a string at the PHP/model level if
necessary.
---
Nitpick comments:
In `@database/migrations/2026_03_21_192332_deduplicate_external_identities.php`:
- Around line 14-29: The SELECT in the migration uses magic lengths (25/22) to
distinguish User FQCN variants—replace the fragile length checks with explicit
pattern matching or document the assumption: update the query that populates
$duplicates (in this migration) to use explicit model_type patterns (e.g. LIKE
patterns matching the current FQCN variants for User) instead of LENGTH(...) and
add a short inline comment explaining the expected FQCN variants and why those
patterns are used so future namespace changes won’t break silently.
In `@database/migrations/2026_03_21_192333_convert_morph_types_to_aliases.php`:
- Around line 62-77: Wrap the entire up() migration work (the loops over
self::TABLES and self::MORPH_MAP) in a DB transaction so the multi-table updates
are atomic: either use DB::transaction(...) around the existing logic or
explicitly call DB::beginTransaction(), run the
DB::table(...)->where(...)->update(...) loops, then DB::commit(), and on
exception DB::rollBack() and rethrow; keep identifiers TABLES, MORPH_MAP, up(),
Schema::hasTable and DB::table(...) unchanged so the same update logic runs
inside the transaction.
- Around line 79-102: Wrap the down() migration in a DB transaction to avoid
partial updates: open a DB::transaction closure around the existing loop that
iterates self::TABLES and applies the $reverseMap updates so all updates either
commit or rollback together. Ensure you keep the Schema::hasTable checks and the
inner DB::table(...)->where(...)->update(...) calls unchanged inside the
transaction; reference the down() method, self::TABLES, $reverseMap,
Schema::hasTable and DB::table to locate where to wrap the transaction.
In `@database/migrations/2026_03_21_200000_cleanup_orphan_users.php`:
- Around line 10-115: The up() migration must run inside a DB transaction to
ensure all phases are atomic; wrap the existing logic in up() in a transaction
(either by returning DB::transaction(function () { ... }) around the current
body or by calling DB::beginTransaction(), running the existing steps,
DB::commit() and DB::rollBack() in a try/catch that rethrows on error). Ensure
you reference the same function up() and keep all current DB::* calls (e.g.,
DB::select, DB::table(...)->update/delete) inside the transactional block so any
exception rolls back all changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9991e42a-4fdf-47b9-92e6-84d5931fb48f
📒 Files selected for processing (22)
app-modules/activity/src/Actions/NewMessage.phpapp-modules/bot-discord/src/Events/MessageReceivedEvent.phpapp-modules/bot-discord/src/Events/WelcomeMember.phpapp-modules/bot-discord/src/SlashCommands/AbstractSlashCommand.phpapp-modules/bot-discord/src/SlashCommands/IntroductionCommand.phpapp-modules/economy/database/factories/WalletFactory.phpapp-modules/economy/tests/Feature/EconomyFlowTest.phpapp-modules/events/database/factories/EventAgendaFactory.phpapp-modules/events/src/Enums/SchedulableTypeEnum.phpapp-modules/events/src/Providers/EventsServiceProvider.phpapp-modules/gamification/src/Providers/GamificationServiceProvider.phpapp-modules/identity/database/factories/ExternalIdentityFactory.phpapp-modules/identity/database/migrations/2025_11_02_171945_add_polymorphic_fields_to_providers_table.phpapp-modules/identity/src/ExternalIdentity/Actions/CreateAccountByExternalIdentity.phpapp-modules/identity/src/Providers/IdentityServiceProvider.phpapp-modules/identity/src/User/Actions/UpdateProfile.phpapp-modules/identity/tests/Feature/Auth/AuthenticateActionTest.phpapp/Providers/AppServiceProvider.phpdatabase/migrations/2026_03_21_192332_deduplicate_external_identities.phpdatabase/migrations/2026_03_21_192333_convert_morph_types_to_aliases.phpdatabase/migrations/2026_03_21_200000_cleanup_orphan_users.phpdatabase/seeders/ThreeDotsSeeder.php
...identity/database/migrations/2025_11_02_171945_add_polymorphic_fields_to_providers_table.php
Show resolved
Hide resolved
database/migrations/2026_03_21_192332_deduplicate_external_identities.php
Show resolved
Hide resolved
- Add PostgreSQL 18 service with dynamic port mapping - Create test_he4rtbot database automatically - Inject DB_PORT per-step to override phpunit.xml default - Add pdo_pgsql to PHP extensions for CI runner - Switch from SQLite :memory: to production-parity testing
- Update phpunit.xml to use pgsql connection - Configure PostgreSQL credentials: test_he4rtbot database - Update .env.testing.example to match docker-compose setup - Ensure production parity for test migrations and queries
- Remove --parallel flag from test/test-unit/test-feature in Makefile - Remove --parallel flag from test scripts in composer.json - Add setup-test-db target for manual database creation - Parallel testing reserved for CI only (--ci --parallel) - Local runs use serial mode for simpler debugging
- Bump Filament packages to v5.4.1 - Update Laravel framework to v12.55.1 and Nightwatch to v1.24.4 - Upgrade pestphp/pest and related testing tools - Update webmozart/assert to v2.1.6 - Bump various JS dependencies (tailwindcss, prettier-plugin-blade) - Refresh composer.lock with new dependency versions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
96-98: Consider validating thefileargument.If
make import-dbis invoked without thefilevariable,pg_restorewill fail with an unclear error. A guard would improve usability.🔧 Proposed validation
.PHONY: import-db import-db: ## Import a PostgreSQL dump file (usage: make import-db file=path/to/dump) +ifndef file + $(error Usage: make import-db file=path/to/dump) +endif `@PGHOST`=localhost PGUSER=postgres PGPASSWORD=postgres pg_restore -x -O -cC -j 8 -d postgres $(file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 96 - 98, The import-db Make target invokes pg_restore with $(file) but doesn't validate that the file variable was provided; add a guard at the start of the import-db recipe to check that the $(file) variable is non-empty and fail with a clear message if missing (e.g., echo "Usage: make import-db file=path/to/dump" && exit 1), so update the import-db target (which runs pg_restore) to validate $(file) before calling pg_restore and print a helpful error when it's not supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 96-98: The import-db Make target invokes pg_restore with $(file)
but doesn't validate that the file variable was provided; add a guard at the
start of the import-db recipe to check that the $(file) variable is non-empty
and fail with a clear message if missing (e.g., echo "Usage: make import-db
file=path/to/dump" && exit 1), so update the import-db target (which runs
pg_restore) to validate $(file) before calling pg_restore and print a helpful
error when it's not supplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d87b07f1-b3bd-4faa-adf7-528bab3d554e
📒 Files selected for processing (6)
.env.testing.example.github/workflows/_pest.yml.github/workflows/continuous-integration.ymlMakefilecomposer.jsonphpunit.xml
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 `@composer.json`:
- Around line 15-16: The composer.json contains package constraints that don't
exist on Packagist; update the version constraints for the specific packages
named in the diff: change "pestphp/pest" to an existing release (e.g. ^4.4.2),
change "filament/spatie-laravel-media-library-plugin" to the published v5.4.0
(e.g. ^5.4.0), and change "owenvoke/blade-fontawesome" to the latest available
on Packagist (e.g. ^3.2.0) or remove/hold that dependency until the upstream
release is available, then run composer update to verify installs succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cc67b73-99e6-4934-9b6c-a00efeec6188
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
composer.jsonpackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
- Set $keyType to 'string' in Badge and Sponsor models for UUID support - Update tests to use tenant model keys instead of hardcoded IDs - Allow badgeId to be string or int in PersistClaimedBadge handler - Adjust event attendee pivot updates to use attendee keys - Ensure unique usernames in UserFactory - Add migration to relax country/state column lengths in user_address
|
Lessons learned from 911a805: "Never use SQLite as a test database unless the production database is also SQLite." |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php (2)
120-122:⚠️ Potential issue | 🟡 MinorInconsistent use of
->idvs->getKey().Line 98 was updated to use
auth()->user()->getKey(), but line 122 still usesauth()->user()->id. For consistency and UUID support, update this to usegetKey().Proposed fix
expect($event->attendees()->count())->toBe(5) ->and($event->fresh()->waitlist_count)->toBe(1) - ->and($event->isParticipating(auth()->user()->id))->toBeTrue(); + ->and($event->isParticipating(auth()->user()->getKey()))->toBeTrue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php` around lines 120 - 122, The assertion uses auth()->user()->id which is inconsistent with the earlier change to auth()->user()->getKey(); update the call passed into Event::isParticipating to use auth()->user()->getKey() instead of ->id so UUIDs work and calls are consistent—locate the assertion chain containing expect($event->attendees()->count())->toBe(5)->and($event->fresh()->waitlist_count)->toBe(1)->and($event->isParticipating(...)) and replace the inner auth()->user()->id with auth()->user()->getKey().
140-141:⚠️ Potential issue | 🟡 MinorSame inconsistency — use
getKey()for consistency.Proposed fix
expect($event->attendees()->count())->toBe(4) - ->and($event->isParticipating(auth()->user()->id))->tobeFalse(); + ->and($event->isParticipating(auth()->user()->getKey()))->toBeFalse();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php` around lines 140 - 141, The assertion uses auth()->user()->id inconsistently; change the call to use getKey() for consistency by replacing auth()->user()->id with auth()->user()->getKey() in the assertion that calls isParticipating() (the line chaining expect($event->attendees()->count())->toBe(4)->and($event->isParticipating(...))->tobeFalse()). Ensure only the parameter passed to isParticipating() is updated to auth()->user()->getKey().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app-modules/identity/database/migrations/2026_03_21_000002_fix_user_address_column_lengths.php`:
- Around line 14-15: The migration widens the DB columns country/state but the
Filament form still restricts them to maxLength(4); update
app-modules/identity/src/Filament/Shared/Schemas/UserAddressForm.php to increase
or remove the maxLength constraint for the country and state fields (references:
UserAddressForm, the country and state field declarations that call
maxLength(4)) so UI validation matches the migration and users can enter longer
values; ensure any client/server validators using the same symbols are updated
consistently.
- Around line 22-23: The migration currently shrinks the country and state
columns to length 4 which makes rollback unsafe if existing values exceed 4
characters; before calling $table->string('country', 4)->nullable()->change()
and $table->string('state', 4)->nullable()->change() (in this migration file),
add a safe normalization step that truncates or cleans existing values to <=4
characters (e.g., update rows to substring(country,1,4) / substring(state,1,4)
or set overly-long values to NULL) inside a DB transaction, and/or add a guard
that aborts the schema change if any rows exceed length 4 so the schema change
cannot fail during rollback. Ensure the normalization runs in the same migration
(or in down()/up() as appropriate) and target the country and state column names
referenced above.
---
Outside diff comments:
In `@app-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.php`:
- Around line 120-122: The assertion uses auth()->user()->id which is
inconsistent with the earlier change to auth()->user()->getKey(); update the
call passed into Event::isParticipating to use auth()->user()->getKey() instead
of ->id so UUIDs work and calls are consistent—locate the assertion chain
containing
expect($event->attendees()->count())->toBe(5)->and($event->fresh()->waitlist_count)->toBe(1)->and($event->isParticipating(...))
and replace the inner auth()->user()->id with auth()->user()->getKey().
- Around line 140-141: The assertion uses auth()->user()->id inconsistently;
change the call to use getKey() for consistency by replacing auth()->user()->id
with auth()->user()->getKey() in the assertion that calls isParticipating() (the
line chaining
expect($event->attendees()->count())->toBe(4)->and($event->isParticipating(...))->tobeFalse()).
Ensure only the parameter passed to isParticipating() is updated to
auth()->user()->getKey().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0a6a458-8df8-4102-9225-bf61d79f9b6b
📒 Files selected for processing (9)
app-modules/events/src/Models/Sponsor.phpapp-modules/events/tests/Feature/Filament/Admin/Event/CreateEventTest.phpapp-modules/events/tests/Feature/Filament/Admin/Sponsors/CreateSponsorTest.phpapp-modules/events/tests/Feature/Filament/App/Events/ListEventsTest.phpapp-modules/gamification/src/Badge/Models/Badge.phpapp-modules/gamification/src/Character/Actions/PersistClaimedBadge.phpapp-modules/identity/database/factories/UserFactory.phpapp-modules/identity/database/migrations/2026_03_21_000002_fix_user_address_column_lengths.phpapp-modules/identity/tests/Feature/FindProfileTest.php
✅ Files skipped from review due to trivial changes (1)
- app-modules/identity/database/factories/UserFactory.php
Summary
Changes
Summary by CodeRabbit
Bug Fixes
Chores
Other