Skip to content

Follow up to #110: set-only mutators with enum casts - #124

Merged
tcampbPPU merged 5 commits into
fumeapp:masterfrom
lorenzodalaqua:set-only-mutator-cast-fallback
Jun 12, 2026
Merged

Follow up to #110: set-only mutators with enum casts#124
tcampbPPU merged 5 commits into
fumeapp:masterfrom
lorenzodalaqua:set-only-mutator-cast-fallback

Conversation

@lorenzodalaqua

Copy link
Copy Markdown
Contributor

Hi, this is a follow up to my previous PR (#110) with two additional fixes I found while applying the change in production.

After merging #110, models with a set-only Attribute mutator and an enum cast on the same column still generated the database column type (string) rather than the enum type. The cast-based type resolution was being skipped entirely because ModelInspector::getCastType() returns the 'attribute' marker whenever an Attribute method exists, so the enum class name never reached WriteColumnAttribute, and the #110 fallback only looked at $attribute['type'] (the DB column type), not the model's $casts.

While testing I also discovered the fixture I added in #110 (stringWithMutatorAndNoAccessor) never actually exercised the new branch: Illuminate\Database\Eloquent\Casts\Attribute wasn't imported in Complex, so the return type resolved to the non-existent App\Models\Attribute, hasAttributeMutator() returned false, and the test silently passed while going through the wrong path. That's fixed here too.

Problems

  1. A set-only Attribute mutator on a column with an enum cast generates the raw column type (string) instead of the enum type, because the 'attribute' marker from ModelInspector causes the actual cast to be ignored.
  2. WriteEnumConst iterates ReflectionClass::getConstants(), which includes plain const declarations alongside enum cases. This caused a crash ("Attempt to read property 'name' on array") on any enum that also defines non-case constants (e.g. a const array of cases, common in one codebase I work on).
  3. The Complex test fixture was missing the Attribute import, making the Add a fallback type for model properties with "set" only mutators #110 test a no-op.

Proposal

When no get closure is found, resolve the model's $casts before falling back to the column type. Enum class casts map to their generated const type, and scalar casts go through the normal mappings. This in theory mirrors what Eloquent does at runtime: reads on a set-only attribute still go through the model's cast.

For the WriteEnumConst crash, I suggest we filter getConstants() to only enum case instances before iterating.

I look forward to any feedback about this proposed fix and whether it fits with your vision for the package.

Changes

  • src/Actions/WriteColumnAttribute.php: when the Attribute has no get closure, consult getCasts() on the model instance first, then resolve enum casts and scalar casts before falling back to the DB column type.
  • src/Actions/WriteEnumConst.php: filter constants to instanceof \UnitEnum to skip non-case class constants.
  • test/laravel-skeleton/app/Models/Complex.php: Add missing use Illuminate\Database\Eloquent\Casts\Attribute import, add enumWithMutatorAndNoAccessor (set-only mutator with a Roles::class cast) to cover the main fix.
  • test/laravel-skeleton/database/migrations/…_create_complex_model_table.php: add the corresponding column to the table
  • test/laravel-skeleton/app/Enums/Roles.php: add a NON_ADMIN_ROLES const array to exercise the WriteEnumConst fix through all existing enum tests.
  • test/input/expectations/complex-model*.ts: Update the enum_with_mutator_and_no_accessor: Roles property and the Roles const block on all four expectation variants.

Complex::stringWithMutatorAndNoAccessor() declared an Attribute return
type without importing Illuminate\Database\Eloquent\Casts\Attribute, so
the declaration resolved to the non-existent App\Models\Attribute and
hasAttributeMutator() never recognized the method. The test added in
fumeapp#110 passed while silently skipping the code path it was meant to
cover. With the import in place, the column actually goes through the
accessor/attribute branch and still maps to its column type.
ReflectionClass::getConstants() returns plain class constants alongside
enum cases, so an enum that also declares helper constants with
non-case values (e.g. a const array of cases) crashed the writer with
"Attempt to read property 'name' on array". Keep only the constants
that are enum case instances. The NON_ADMIN_ROLES constant on the Roles
fixture covers this through every test that renders the enum.
When an Attribute mutator defines no get callback, reads still go
through the cast defined on the model, but the generator fell back
straight to the database column type (fumeapp#110). An attribute with an enum
cast and a set-only mutator therefore emitted the column type (string)
instead of the enum. Resolve the model's cast first -- enum classes map
to their generated const, scalar casts go through the regular mappings
-- and only use the column type when no cast matches.
UnitEnum is the base interface for all enums but only BackedEnum
has a value property. PHPStan correctly flags the access to
$case->value as undefined on UnitEnum. Since the package can
only emit meaningful TypeScript values for backed enums, narrowing
the filter to BackedEnum also makes the intent explicit.
@lorenzodalaqua
lorenzodalaqua force-pushed the set-only-mutator-cast-fallback branch from 98ebfb2 to a0288de Compare June 10, 2026 14:51
@lorenzodalaqua

Copy link
Copy Markdown
Contributor Author

I see some PHPStan errors, but I believe they are pre-existing since it's not a file touched by this PR. If I am somehow mistaken please let me know.

@tcampbPPU

Copy link
Copy Markdown
Member

hey there, let me look into the phpstan errors and see i can get those resolved on the main branch and i will look into your 2 pr, ty!

@lorenzodalaqua

Copy link
Copy Markdown
Contributor Author

Sounds good, thanks!

@tcampbPPU
tcampbPPU merged commit fe803e4 into fumeapp:master Jun 12, 2026
26 checks passed
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.

2 participants