fix: support union-typed entity columns via explicit Column type - #145
Conversation
… type EntityMetadataFactory rejected any entity property whose reflection type is not a single named type, throwing "must have a type declaration" for union types such as `int|string`. Because db:migrate/db:diff parse metadata for every discovered entity, marko/media's MediaAttachment (a polymorphic foreign key declared `int|string $attachableId`) broke both commands for any project with marko/media installed, regardless of whether the entity was used. Handle ReflectionUnionType: a union has no single reflection type to infer a column type from, so require an explicit #[Column(type: ...)] and use it as the database type. Declare the media_attachments.attachable_id column as varchar, which holds both int and string primary keys while preserving the polymorphic union on the PHP side.
The database docs page listed inference rules for every named type but said nothing about unions, so the new "explicit #[Column(type: ...)] required" behavior was undiscoverable. Add the inference-table row, a Union-Typed Columns section using the polymorphic-foreign-key case, and a note that a varchar-backed union always hydrates to string. Co-Authored-By: Tu Van <vandinhtuit@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
markshust
changed the title
github-actions
Bot
added
bug
|
Thanks @TuVanDev — confirmed and merging. Reproduced on Requiring an explicit Verified locally: full suite 6861 passed, I pushed one docs commit to the branch so the new rule is discoverable: the database docs |
Step 5 told the user a failed run "leaves nothing to clean up". That is wrong: bin/release.sh checks out main and merges develop at line 25, well before the test suite runs at line 61. A test failure leaves the repo on main with the merge already made — nothing pushed and no tag, but not a no-op, and a retry from there trips the skill's own on-develop precondition with a misleading error. State the real failure state and the `git checkout develop` recovery. Add a test that asserts the merge still precedes the test run in bin/release.sh, so the claim cannot silently rot if the script is reordered. Also narrow tier 2: internals added in service of a fix (a new exception factory, say) are thrown at consumers rather than built against, so they do not promote a bug fix to a feature. Without that, PR #145 in the calibration table reads as tier 3 on a strict pass and would wrongly escalate a patch to a minor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
EntityMetadataFactorythrewProperty '...' must have a type declarationfor any entity property with a PHP union type (ReflectionUnionType), with no escape hatch. This brokedb:migrate/db:diffoutright for any project withmarko/mediainstalled, sinceMarko\Media\Entity\MediaAttachment::$attachableIdis declaredint|string(a polymorphic foreign key — an attachment can belong to entity types whose primary keys are int or string).Fix
EntityMetadataFactorynow accepts aReflectionUnionTypeproperty when it carries an explicit#[Column(type: ...)]attribute (already a supported mechanism for named-type columns, just not consulted before the union-type guard threw).MediaAttachment::$attachableIdis now declared#[Column(type: 'varchar', length: 255)], preserving itsint|stringPHP type while giving the factory an unambiguous DB column type. Union properties without an explicit#[Column(type:...)]still throw, now with a clearerEntityException::unionTypeRequiresColumnType()message instead of the previous misleading "must have a type declaration".The
ReflectionNamedTypecode path is unchanged — this is a strictly additive branch.Notes for reviewers
attachableIdalways comes back as a PHP string regardless of the original id's type (varchar columns hydrate to string, andint|stringaccepts that with no TypeError) — code comparing it against a real int PK should use loose comparison or cast explicitly. No existing usage inpackages/mediadoes a strict comparison.stringin their type list (a varchar column round-trips fine). A hypothetical future union likeint|boolwould still need casting support at hydration time — out of scope here since no such entity currently exists.packages/mediaships no migration files — schema is derived entirely from entity metadata viaSchemaRegistry, so this fix directly unblocksdb:diff/db:migratefor any consumer.Test plan
./vendor/bin/pest packages/database/tests— 883 passed./vendor/bin/pest packages/media/tests— 38 passeddevelop(pre-fix) with the original misleading error, and pass on this branch./vendor/bin/phpcs --standard=phpcs.xmlclean on all changed files./vendor/bin/phpstan— zero new errors vsdevelopbaselineAdditional changes (maintainer)
packages/docs-markdown/docs/packages/database.md: a union-type row in the Type Inference Rules table, a Union-Typed Columns section using the polymorphic-foreign-key case, and a note that a varchar-backed union always hydrates tostring(so app code should not strict-compare it against an integer primary key).