feat: section-scoped custom field resolution - #198
Merged
Conversation
…n down(), document republish step
FormBuilder::build() and InfolistBuilder::build() return a Grid/Container whose generateSchema() re-enters a fresh builder, silently dropping onlySections(). Thread the scope through both containers so ->onlySections([...])->build() matches ->onlySections([...])->values().
…migration - defaultUniqueIndexName() now folds in the connection's prefix_indexes/ getTablePrefix() exactly as Blueprint::createIndexName() does, so the computed drop target matches the real index name on prefixed installs instead of silently no-opping. - down() now aborts with a clear RuntimeException before touching the schema when rows share a code across sections, instead of dropping the wide key and then failing to add the narrow one back (which would leave MySQL with no unique key at all, since each ALTER TABLE auto-commits). - Documented the NULL-is-distinct-in-a-unique-index consequence of the nullable custom_field_section_id column in the migration's why comment. Covering tests in RelaxCustomFieldsUniqueKeyMigrationTest.php: down() restoring the narrow key, up()/down() idempotency in both directions, the new duplicate-detection guard (and its release once resolved), and the prefix-honoring fix cross-checked against Laravel's own Blueprint::createIndexName().
- codeExists() now reassigns $query = $scope($query) instead of discarding the return value, matching the docblock's Closure(Builder): Builder type and the sibling FieldForm::resolveUniqueRuleModifierUsing() convention. A resolver that returns a cloned/narrowed builder (rather than mutating in place) was previously a silent no-op. - Removed the unused $sectionId parameter from generateUniqueSectionCode() — it has no caller (CustomFieldsManagementPage.php passes two args) and no dedicated 'section' type test. Public API surface on a package that hasn't shipped this parameter yet; cheaper to remove now than to break later. $type stays in the resolver callback signature.
… code Before the unique key was relaxed to include custom_field_section_id, two fields could never share a code anywhere in the entity type, so a drag target could never already hold a colliding code. Now it can: updateFieldsOrder() receives the sortable's complete post-drop field-id list for the target section, and if two of them share a code the second update() throws an unhandled QueryException (a 500 in the package's own management UI). Pre-check for a duplicate code among the fields being moved and, if found, send a danger notification and return without writing anything, instead of partially reordering and then crashing.
- Added InfolistBuilder::build() coverage mirroring the existing FormBuilder::build() test — InfolistContainer's onlySections() threading had zero coverage. - 'composes section scope with only() field codes' asserted ->toHaveCount(1) on values(), which returns one component per section with SYSTEM_SECTIONS enabled — it passed whether only() dropped one field or none. Now asserts on the section's own field count via a new sectionFieldComponents() reflection helper. - Renamed the CodeGenerator sectionless test to describe what it actually exercises (a direct generateUniqueFieldCode() call, not a ManageFieldsTable call site). - Added a scope-closure discrimination test for the return-value fix in the previous CodeGenerator commit, using a clone()-returning closure so a mutation-only implementation can't accidentally pass it. Documented the persistence-contract caveat (onlySections() narrows resolution but does not scope UsesCustomFields::saveCustomFields(), which still writes by code) on BaseBuilder::onlySections()'s docblock.
- installation.md no longer names a specific version ('v3.7.0') for the
migration example — that tag doesn't exist; links to the upgrade guide
instead.
- data-model.md corrected 'code' from 'Unique identifier' to the actual
scope for both sections (unique per entity type/tenant, unchanged) and
fields (unique per entity type/tenant AND section as of this branch —
no longer globally unique per entity type).
- upgrade-guide.md gained a 'Picking Up New Migrations' section: the
republish-and-migrate instructions, the relax-unique-key example, the
down() duplicate-detection guard, and the NULL-is-not-constrained
caveat for sectionless fields.
- New docs/content/2.essentials/7.builder-scoping.md documents
onlySections() and CodeGenerator::resolveUniquenessScopeUsing(),
including the saveCustomFields() persistence-contract warning.
The code TextInput's unique rule was hardcoded to a global
(code, entity_type, tenant_id) scope with no modifier hook, unlike the
name field which already threaded resolveUniqueRuleModifierUsing(). A
consumer versioning forms and reusing field codes across versions
(the whole point of that feature) had no way to scope code uniqueness
the same way, so an edit on a cloned field with a legitimately reused
code failed validation against its own earlier version.
Adds a second, code-specific resolver (resolveUniqueCodeRuleModifierUsing)
mirroring the existing name hook rather than repurposing it: name and
code commonly need different scopes -- name is cosmetic and can be
scoped to 'this one form', but code is often a stable cross-form
identity that needs the opposite shape ('everything except a defined
set of related forms'). Backward compatible: the new hook defaults to
null and the code field's uniqueness behavior is unchanged unless a
consumer registers it.
PHPStan 2.2 (pulled in by larastan 3.10) narrows the match(true) subject across arms. By the is_numeric() arm $value can only be array|object| resource, for which is_numeric() is always false, so the arm is dead and analysis fails with function.impossibleType. Every numeric shape is already caught earlier: numeric strings by is_string(), ints by is_int(), floats by is_float(). Verified by probing '42', '3.14', '0x1A', ' 9 ', 42 and 3.14 through the chain - none reach the arm. Removing it is behavior-preserving. Pre-existing on 3.x; CI surfaced it here because the workflow runs composer update --prefer-stable rather than installing from the lock.
Two sites added by this branch drift from rector 2.6: - CodeGenerator::codeExists() - FlipTypeControlToUseExclusiveTypeRector prefers `instanceof Closure` over `!== null` for the resolver guard. - RelaxCustomFieldsUniqueKeyMigrationTest - NewlineBeforeNewAssignSetRector wants a blank line before the assignment following setAccessible(). Both were latent: CI runs PHPStan before Rector and never reached the Rector step while the analysis error was failing the job.
…ibleJs
formatJsValue() routes numeric strings through is_string(), so '42' is
emitted as the JS string literal '42' rather than the number 42. That is
correct, not an oversight: VisibilityOperator::evaluateEquals() compares
two strings as strings, so '42.5' and '42.50' are unequal on the server.
Emitting the condition as a number would make parseFloat('42.5') === 42.5
true on the client and desync the two engines - the exact invariant this
service exists to hold.
Nothing covered this, so the safe-looking reorder (is_numeric above
is_string) passed the suite. These tests fail against that reorder and
pass as shipped.
VisibilityOperator::evaluateEquals() folds two strings through strtolower(), but the emitted expression fell straight into `fieldVal === compareVal`. A condition of "Active" against a field holding "active" therefore evaluated true on the server and false in the browser: the field stayed hidden until the user matched the case exactly, and any server-rendered state disagreed with the live form. Reproduced by evaluating the generated expression in node against the backend operator - "active" and "ACTIVE" both returned false client-side where the server returned true. All four probed values now agree. not_equals negates this expression, so it inherits the fix. Option-backed choice fields are untouched: they compare resolved option ids, which must stay case-sensitive.
A differential harness (every condition value x every field value, backend result vs the generated expression evaluated in node) found 8 mismatches across 4 classes. The two engines are supposed to be interchangeable, so each one is a field that appears in the live form but not on a re-render, or the reverse. Numeric: a NUMERIC field reads back from integer_value as an int, while its condition is stored as the string the text input produced. Strict identity meant `42 === '42'` was false, so a numeric condition never matched server-side while the client matched it. Mixed number and numeric-string now compare numerically. Two strings still compare as strings, so '42.5' and '42.50' stay distinct on both sides. Boolean: formatJsValue() emits real booleans and the literals 'true' and 'false' alike as JS booleans, so the client cannot distinguish a bool from its spelling. Both sides now compare the lowercased spelling. Collections: in_array($expected, $fieldValue, true) looked for the whole condition array as a single element, so two identical arrays compared false on the server and true on the client. Two arrays now compare as sets. A single condition value against a multi-value field stays membership, on the string form so [42] matches '42' as the client does. The client kept its own gaps: an array field value fell through to String(fieldVal) === String(compareVal), comparing 'a,b' against 'a'. Harness now reports 0 mismatches over 220 pairs. The arm order in evaluateEquals and in the emitted expression are deliberately identical - reordering either desyncs them again.
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
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.
Problem
This package resolves custom fields by
code, globally perentity_type. That works until a consumer versions its form definitions.A consumer that clones a form into a new version gives each version its own
custom_field_sections. Because resolution is code-keyed and global, the consumer is forced to hand every cloned field a fresh code —hmis_id,hmis_id_1,hmis_id_2— purely so the builders can tell versions apart. Butcodeis also the reporting identity, so one logical field fragments into N report columns with identical labels, and a report built on one version returns blanks for records saved against another.The field code is doing two contradictory jobs: render-time discriminator (must differ per version) and stable identity (must not).
What this does
Splits them. Resolution becomes scopeable structurally, by section id, so
codecan stay stable across versions.BaseBuilder::onlySections(array $sectionIds): static— constrains resolution to the given sections. Inherited by all five builders (Form,Infolist,Table,Exporter,Importer). Also threaded throughFormContainer/InfolistContainerso->build()honours it, not just->values()— those containers re-enter a fresh builder, so without threading the scope was silently dropped.custom_fieldsunique key relaxed to(code, entity_type[, tenant], custom_field_section_id)via a new migration, so two sections can hold the same code. Without this the new API is unusable: the schema forbids the exact data shape it exists to enable.CodeGenerator::resolveUniquenessScopeUsing()— closes the write side. Without it, adding a new field named "HMIS ID" to version 2 still yieldshmis_id_1. The section id is passed as an explicit argument rather than read from ambient state. Mirrors the existingFieldForm::resolveUniqueRuleModifierUsing()andVisibilityComponent::resolveAvailableFieldsScopeUsing()hooks.Everything defaults to off. Existing consumers that never call the new APIs are unaffected — except the migration, see below.
Please read before merging
0. This PR now carries an unrelated second concern — consider splitting it. Commits
472cd556,91a3be4f,fb7080ffand5fa87dabfix conditional-visibility defects found while getting CI green. They are independently justified and tested, but they touchVisibilityOperatorandFrontendVisibilityService, which have nothing to do with section scoping.5fa87dabin particular changes the semantics of a core operator used by server-side visibility, infolists, tables and exports. Reviewing it on its own merits is strongly preferable to waving it through with the feature. See "Conditional visibility fixes" below.1.
codeis no longer globally unique perentity_type. This is unconditional for every sections-enabled consumer once they republish and migrate — there is no feature flag on the schema change. Any code resolving a field bycodealone (where('code', …)->first(),keyBy('code')) becomes ambiguous. In-package examples:UsesCustomFields::saveCustomFields(),BackendVisibilityService::getCachedFieldsForEntity(),CustomFieldsMigrator::find().2. The persistence contract.
saveCustomFields()iterates$this->customFields()and writes by code. With two sections sharing a code it writes the same value to both rows. This is documented as anIMPORTANT:note ononlySections()and in the new docs page: a consumer using section-scoped codes must scope its model'scustomFields()relation to match. We deliberately did not redesignsaveCustomFields()— that deserves its own decision.3. Null-section rows lose uniqueness.
custom_field_section_idis nullable and NULL is distinct in a unique index, so after the migration two rows with a NULL section may share(code, entity_type[, tenant]). Documented in the migration and upgrade guide; no schema-level fix.4. Upgrading requires an explicit republish.
runsMigrationsis not enabled on this package, sogetMigrations()only registers apublishes()mapping. A plaincomposer update+migrateis a silent no-op and the feature will appear broken. Runphp artisan vendor:publish --tag="custom-fields-migrations"thenphp artisan migrate. Now in the upgrade guide.5.
down()refuses to run once duplicate codes exist. It aborts with a named offending code before touching any DDL — otherwise MySQL's auto-committing DDL would drop the wide key, fail to add the narrow one, and leave the table with neither.6. Section-scoped codes need all three hooks registered together:
onlySections()for resolution,FieldForm::resolveUniqueRuleModifierUsing()for the name rule, and the newCodeGeneratorhook for generation. Two paths remain uncovered by any hook and are noted as known gaps: the "Duplicate field" action (ManageCustomField::generateUniqueCode()reimplements generation inline) and the manualcodeinput'suniquerule whenFIELD_CODE_AUTO_GENERATEis off.7. The suite runs on SQLite; this migration has never executed against MySQL or Postgres. Given that relaxing a shipped unique key is the riskiest change here, a manual MySQL run before tagging would be worth it. Two real MySQL-specific bugs were already caught and fixed during review that SQLite could not have surfaced — see below.
Notable catches during review
defaultUniqueIndexName()originally omitted the table prefix. Laravel'sBlueprint::createIndexName()prepends it whenprefix_indexesis set — true by default for mysql and pgsql. On a prefixed install the drop target never matched, so the narrow key survived andonlySections()would have appeared to work while the DB still rejected duplicate codes. Silent, with no error anywhere.toHaveCount(0)assertion passed against buggy code. That test now asserts on the query log instead.Conditional visibility fixes (unrelated to section scoping)
CI was red on arrival, for a reason that had nothing to do with this branch: PHPStan 2.2 (via larastan 3.10) now narrows the
match(true)subject across arms and flagged a deadis_numeric()arm inFrontendVisibilityService::formatJsValue(). The same failure reproduces on3.x— CI installs withcomposer update --prefer-stableand ignores the lock, so it drifts ahead of local vendor. Pulling that thread surfaced four defects.Dead arm removed (
472cd556). Every numeric shape is caught earlier byis_string/is_int/is_float, so the arm was unreachable. Verified by probing'42','3.14','0x1A',' 9 ',42,3.14through the chain. Behaviour-preserving.Rector drift (
86d03e44). Two sites added by this branch. Latent, not new: CI runs PHPStan before Rector and never reached the Rector step.Case-sensitivity desync (
fb7080ff).VisibilityOperator::evaluateEquals()folds two strings throughstrtolower(), but the emitted expression went straight tofieldVal === compareVal. A condition of"Active"against a field holding"active"evaluated true on the server and false in the browser — the field stayed hidden until the user matched the case exactly.Equals-operator alignment (
5fa87dab). A differential harness — every condition value × every field value, backend result vs the generated expression evaluated in node — found 8 mismatches across 4 classes:NUMERICfield reads back frominteger_valueas anintwhile its condition is the string a text input stored, so42 === '42'was false and a numeric condition never matched server-side. Mixed number/numeric-string now compares numerically; two strings still compare as strings, so'42.5'≠'42.50'.'true'spellingformatJsValue()emits real booleans and the literals'true'/'false'alike as JS booleans, so the client cannot distinguish a bool from its spelling. Both sides now compare the lowercased spelling.in_array($expected, $fieldValue, true)looked for the whole condition array as a single element, so two identical arrays compared false on the server and true on the client. Now set equality.String(fieldVal) === String(compareVal), comparing'a,b'against'a'. Now membership, on the string form so[42]matches'42'.The harness reports 0 mismatches over 220 pairs after the fix. The arm order in
evaluateEquals()and in the emitted expression are deliberately identical — reordering either desyncs them again.Two limits are inherent rather than overlooked, and are left as-is:
42and42.0are indistinguishable client-side. PHP's42 === 42.0is false; the backend now coerces, which resolves the disagreement by adopting the client's semantics.is_numeric()andNumber()disagree on hex. PHP rejects'0x1A'; JS parses it as26.Test plan
820 passed / 3 todos. Pint, PHPStan and Rector clean under the dependency set CI actually installs (
composer update --prefer-stable). Type coverage sits at 99.4%, unchanged from3.xand not a CI gate.Section scoping:
onlySections()scoping including two sections sharing a code; empty scope means no scope; composition withonly(); scope honoured throughbuild()for both form and infolist; sections-disabled guard; theCodeGeneratorhook scoped/unscoped/sectionless; and a dedicated migration test file coveringup(),down(), both idempotency branches, the pre-applied-index state, and the duplicate guard.Conditional visibility: numeric-string condition values pinned to string comparison; case-insensitive parity for
equalsandnot_equals; and operator-level coverage for the numeric, boolean, set-equality, membership and scalar-vs-collection classes. Five of these fail against the pre-fix operator.Every test asserting a fix was proven to fail against the unfixed code.
Intended release
v3.7.0— additive and backward compatible for section scoping.5fa87dabis not backward compatible: it changes when anequalsvisibility condition matches. Conditions that silently never matched server-side (numeric fields) will start matching, and consumers relying on the previous strict-identity behaviour will see visibility change. That alone may argue for shipping it separately.