From f0068818ad658e5efdad435b18d2fd811a207ab5 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 14:47:13 +0400 Subject: [PATCH 01/16] feat: add onlySections() scope to custom field builders --- .../Integration/Builders/BaseBuilder.php | 24 ++++++ tests/Feature/ConsumerScopeHooksTest.php | 84 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/Filament/Integration/Builders/BaseBuilder.php b/src/Filament/Integration/Builders/BaseBuilder.php index dfa9d26f..4e73eafd 100644 --- a/src/Filament/Integration/Builders/BaseBuilder.php +++ b/src/Filament/Integration/Builders/BaseBuilder.php @@ -29,6 +29,9 @@ abstract class BaseBuilder protected array $only = []; + /** @var array */ + protected array $onlySections = []; + public function forSchema(Schema $schema): static { /** @var Model & HasCustomFields $model */ @@ -82,6 +85,22 @@ public function only(array $fieldCodes): static return $this; } + /** + * Constrain resolution to the given custom field sections. + * + * Field codes are unique per section, not globally, in consumers that version their + * sections. Scoping structurally lets two sections carry the same code without one + * bleeding into the other's schema. + * + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + /** * @return Collection */ @@ -94,6 +113,10 @@ protected function getFilteredSections(): Collection /** @var Collection $sections */ $sections = $this->sections + ->when($this->onlySections !== [], fn (Builder $query): Builder => $query->whereIn( + $this->sections->getModel()->getQualifiedKeyName(), + $this->onlySections + )) ->with(['fields' => function (mixed $query): mixed { return $query ->when($this instanceof TableBuilder, fn (CustomFieldQueryBuilder $q, bool $condition): CustomFieldQueryBuilder => $q->visibleInList()) @@ -127,6 +150,7 @@ protected function getFieldsDirectly(): Collection ->when($this instanceof InfolistBuilder, fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->visibleInView()) ->when($this->only !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereIn('code', $this->only)) ->when($this->except !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereNotIn('code', $this->except)) + ->when($this->onlySections !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereIn('custom_field_section_id', $this->onlySections)) ->with('options') ->orderBy('sort_order') ->get() diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index af0c3cff..7c493ab2 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -5,6 +5,7 @@ use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; use Relaticle\CustomFields\Enums\CustomFieldsFeature; +use Relaticle\CustomFields\Facades\CustomFields; use Relaticle\CustomFields\FeatureSystem\FeatureConfigurator; use Relaticle\CustomFields\Filament\Management\Forms\Components\VisibilityComponent; use Relaticle\CustomFields\Filament\Management\Schemas\FieldForm; @@ -110,3 +111,86 @@ function availableFields(VisibilityComponent $component): array expect(FieldForm::schema(section: $section))->toBeArray()->not->toBeEmpty(); }); }); + +describe('BaseBuilder onlySections() scope', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + }); + + it('scopes resolution to the given sections', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying A', 'code' => 'qualifying_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying B', 'code' => 'qualifying_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Alpha', + 'code' => 'alpha', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Beta', + 'code' => 'beta', + 'type' => 'text', + ]); + + $scoped = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->values(); + + $unscoped = CustomFields::form() + ->forModel(Post::class) + ->values(); + + expect($scoped)->toHaveCount(1) + ->and($unscoped)->toHaveCount(2); + }); + + it('treats an empty section scope as no scope', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Only Section', 'code' => 'only_section']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'A Field', + 'code' => 'a_field', + 'type' => 'text', + ]); + + expect( + CustomFields::form() + ->forModel(Post::class) + ->onlySections([]) + ->values() + )->toHaveCount(1); + }); + + it('composes section scope with only() field codes', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Combo', 'code' => 'combo']); + + foreach (['keep_me', 'drop_me'] as $index => $code) { + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => ucfirst($code), + 'code' => $code, + 'type' => 'text', + 'sort_order' => $index, + ]); + } + + expect( + CustomFields::form() + ->forModel(Post::class) + ->onlySections([$section->id]) + ->only(['keep_me']) + ->values() + )->toHaveCount(1); + }); +}); From 35e746ec8fd54b83e5e5c3ad39ce92e572670c06 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 14:58:25 +0400 Subject: [PATCH 02/16] fix: relax custom_fields unique key to allow code reuse across sections --- .../relax_custom_fields_unique_key.php | 69 +++++++++++++++++++ src/CustomFieldsServiceProvider.php | 1 + tests/Feature/ConsumerScopeHooksTest.php | 26 +++---- 3 files changed, 80 insertions(+), 16 deletions(-) create mode 100644 database/migrations/relax_custom_fields_unique_key.php diff --git a/database/migrations/relax_custom_fields_unique_key.php b/database/migrations/relax_custom_fields_unique_key.php new file mode 100644 index 00000000..18a9808a --- /dev/null +++ b/database/migrations/relax_custom_fields_unique_key.php @@ -0,0 +1,69 @@ +defaultUniqueIndexName($table, $oldColumns); + $existingIndexes = collect(Schema::getIndexes($table))->pluck('name'); + + Schema::table($table, function (Blueprint $blueprint) use ($existingIndexes, $oldColumns, $oldIndexName, $newColumns, $newIndexName): void { + if ($existingIndexes->contains($oldIndexName)) { + $blueprint->dropUnique($oldColumns); + } + + if (! $existingIndexes->contains($newIndexName)) { + $blueprint->unique($newColumns, $newIndexName); + } + }); + } + + /** + * Mirrors Laravel's own auto-generated unique-index name so the drop target matches + * exactly what create_custom_fields_table.php produced, without hardcoding a name that + * would drift if the tenant foreign key column is renamed via config. + * + * @param array $columns + */ + private function defaultUniqueIndexName(string $table, array $columns): string + { + return strtolower($table.'_'.implode('_', $columns).'_unique'); + } +}; diff --git a/src/CustomFieldsServiceProvider.php b/src/CustomFieldsServiceProvider.php index 538e58c2..df9c83d1 100644 --- a/src/CustomFieldsServiceProvider.php +++ b/src/CustomFieldsServiceProvider.php @@ -206,6 +206,7 @@ private function getMigrations(): array { return [ 'create_custom_fields_table', + 'relax_custom_fields_unique_key', ]; } } diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index 7c493ab2..cb2d21e7 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -119,25 +119,19 @@ function availableFields(VisibilityComponent $component): array ); }); - it('scopes resolution to the given sections', function (): void { + it('scopes resolution to the given sections when two sections share a field code', function (): void { $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying A', 'code' => 'qualifying_a']); $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Qualifying B', 'code' => 'qualifying_b']); - CustomField::factory()->create([ - 'custom_field_section_id' => $sectionA->id, - 'entity_type' => Post::class, - 'name' => 'Alpha', - 'code' => 'alpha', - 'type' => 'text', - ]); - - CustomField::factory()->create([ - 'custom_field_section_id' => $sectionB->id, - 'entity_type' => Post::class, - 'name' => 'Beta', - 'code' => 'beta', - 'type' => 'text', - ]); + foreach ([$sectionA, $sectionB] as $section) { + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'Shared', + 'code' => 'shared_code', + 'type' => 'text', + ]); + } $scoped = CustomFields::form() ->forModel(Post::class) From c9624893d3ebcde16a75552420a790253aefbb3b Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 15:23:40 +0400 Subject: [PATCH 03/16] fix: guard onlySections() on sections-disabled installs, add migration down(), document republish step --- .../relax_custom_fields_unique_key.php | 91 ++++++++++++++----- .../1.getting-started/1.installation.md | 16 ++++ .../Integration/Builders/BaseBuilder.php | 12 ++- tests/Feature/ConsumerScopeHooksTest.php | 66 ++++++++++++++ 4 files changed, 163 insertions(+), 22 deletions(-) diff --git a/database/migrations/relax_custom_fields_unique_key.php b/database/migrations/relax_custom_fields_unique_key.php index 18a9808a..daf27d7b 100644 --- a/database/migrations/relax_custom_fields_unique_key.php +++ b/database/migrations/relax_custom_fields_unique_key.php @@ -23,6 +23,34 @@ return new class extends Migration { public function up(): void + { + $this->swapUniqueKey( + from: $this->narrowColumns(), + fromIndexName: null, + to: $this->wideColumns(), + toIndexName: $this->wideIndexName(), + ); + } + + public function down(): void + { + $this->swapUniqueKey( + from: $this->wideColumns(), + fromIndexName: $this->wideIndexName(), + to: $this->narrowColumns(), + toIndexName: null, + ); + } + + /** + * Drops $from's unique key if present and adds $to's if absent. Shared by both + * directions: up() widens (code, entity_type[, tenant]) to also include + * custom_field_section_id; down() narrows it back to the original key. + * + * @param array $from + * @param array $to + */ + private function swapUniqueKey(array $from, ?string $fromIndexName, array $to, ?string $toIndexName): void { $table = config('custom-fields.database.table_names.custom_fields'); @@ -30,40 +58,61 @@ public function up(): void return; } - $oldColumns = ['code', 'entity_type']; - - if (FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY)) { - $oldColumns[] = config('custom-fields.database.column_names.tenant_foreign_key'); - } - - $newColumns = [...$oldColumns, 'custom_field_section_id']; - $newIndexName = FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY) - ? 'cf_code_entity_tenant_section_unique' - : 'cf_code_entity_section_unique'; - - $oldIndexName = $this->defaultUniqueIndexName($table, $oldColumns); + $fromIndexName ??= $this->defaultUniqueIndexName($table, $from); + $toIndexName ??= $this->defaultUniqueIndexName($table, $to); $existingIndexes = collect(Schema::getIndexes($table))->pluck('name'); - Schema::table($table, function (Blueprint $blueprint) use ($existingIndexes, $oldColumns, $oldIndexName, $newColumns, $newIndexName): void { - if ($existingIndexes->contains($oldIndexName)) { - $blueprint->dropUnique($oldColumns); + Schema::table($table, function (Blueprint $blueprint) use ($existingIndexes, $fromIndexName, $to, $toIndexName): void { + if ($existingIndexes->contains($fromIndexName)) { + $blueprint->dropUnique($fromIndexName); } - if (! $existingIndexes->contains($newIndexName)) { - $blueprint->unique($newColumns, $newIndexName); + if (! $existingIndexes->contains($toIndexName)) { + $blueprint->unique($to, $toIndexName); } }); } /** - * Mirrors Laravel's own auto-generated unique-index name so the drop target matches - * exactly what create_custom_fields_table.php produced, without hardcoding a name that - * would drift if the tenant foreign key column is renamed via config. + * @return array + */ + private function narrowColumns(): array + { + $columns = ['code', 'entity_type']; + + if (FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY)) { + $columns[] = config('custom-fields.database.column_names.tenant_foreign_key'); + } + + return $columns; + } + + /** + * @return array + */ + private function wideColumns(): array + { + return [...$this->narrowColumns(), 'custom_field_section_id']; + } + + private function wideIndexName(): string + { + return FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY) + ? 'cf_code_entity_tenant_section_unique' + : 'cf_code_entity_section_unique'; + } + + /** + * Mirrors Laravel's own auto-generated unique-index name (Blueprint::createIndexName()) + * so the drop target matches exactly what create_custom_fields_table.php produced, + * without hardcoding a name that would drift if a configured column name changes. * * @param array $columns */ private function defaultUniqueIndexName(string $table, array $columns): string { - return strtolower($table.'_'.implode('_', $columns).'_unique'); + $index = strtolower($table.'_'.implode('_', $columns).'_unique'); + + return str_replace(['-', '.'], '_', $index); } }; diff --git a/docs/content/1.getting-started/1.installation.md b/docs/content/1.getting-started/1.installation.md index 90cba90a..fb4f4908 100644 --- a/docs/content/1.getting-started/1.installation.md +++ b/docs/content/1.getting-started/1.installation.md @@ -84,3 +84,19 @@ php artisan vendor:publish --tag="custom-fields-translations" ```bash php artisan vendor:publish --tag="custom-fields-views" ``` + +## Picking Up New Migrations After an Upgrade + +Package migrations are not run automatically by `php artisan migrate` after a version +bump — they're only copied into your app the first time you install, or when you +explicitly republish them. If a release adds or changes a migration, republish and run +it: + +```bash +php artisan vendor:publish --tag="custom-fields-migrations" +php artisan migrate +``` + +v3.7.0, for example, relaxes the `custom_fields` unique key so a field code can be reused +across sections (needed for `onlySections()`). Existing installs need this step to pick +that change up. diff --git a/src/Filament/Integration/Builders/BaseBuilder.php b/src/Filament/Integration/Builders/BaseBuilder.php index 4e73eafd..99063dd5 100644 --- a/src/Filament/Integration/Builders/BaseBuilder.php +++ b/src/Filament/Integration/Builders/BaseBuilder.php @@ -145,12 +145,22 @@ protected function getFilteredSections(): Collection */ protected function getFieldsDirectly(): Collection { + /* + * custom_field_section_id only exists on the table when SYSTEM_SECTIONS was + * enabled at migration time, and this method is exclusively the sections-disabled + * path (see getAllFields()). A section scope can never match anything here — there + * is no section table to resolve it against — so return empty rather than filter + * by a column that may not exist. + */ + if ($this->onlySections !== [] && ! FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_SECTIONS)) { + return collect(); + } + return CustomFields::newCustomFieldModel()::forMorphEntity($this->model::class) ->when($this instanceof TableBuilder, fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->visibleInList()) ->when($this instanceof InfolistBuilder, fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->visibleInView()) ->when($this->only !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereIn('code', $this->only)) ->when($this->except !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereNotIn('code', $this->except)) - ->when($this->onlySections !== [], fn (CustomFieldQueryBuilder $q): CustomFieldQueryBuilder => $q->whereIn('custom_field_section_id', $this->onlySections)) ->with('options') ->orderBy('sort_order') ->get() diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index cb2d21e7..37b367f1 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -4,6 +4,10 @@ use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; +use Illuminate\Database\Events\QueryExecuted; +use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Schema; use Relaticle\CustomFields\Enums\CustomFieldsFeature; use Relaticle\CustomFields\Facades\CustomFields; use Relaticle\CustomFields\FeatureSystem\FeatureConfigurator; @@ -188,3 +192,65 @@ function availableFields(VisibilityComponent $component): array )->toHaveCount(1); }); }); + +describe('BaseBuilder onlySections() scope on a sections-disabled install', function (): void { + /* + * custom_field_section_id only exists on the table when SYSTEM_SECTIONS was enabled at + * migration time. Toggling the feature flag at runtime does not remove an already + * migrated column, so the column is dropped here to reproduce a genuine + * sections-were-never-enabled install rather than merely flipping the config flag. + */ + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY) + ); + + collect(Schema::getIndexes('custom_fields')) + ->filter(fn (array $index): bool => in_array('custom_field_section_id', $index['columns'], true)) + ->each(fn (array $index) => DB::statement("DROP INDEX \"{$index['name']}\"")); + + Schema::table('custom_fields', fn (Blueprint $table) => $table->dropColumn('custom_field_section_id')); + }); + + it('returns fields unchanged when the section scope is empty', function (): void { + CustomField::factory()->create([ + 'entity_type' => Post::class, + 'name' => 'Alpha', + 'code' => 'alpha_flat', + 'type' => 'text', + ]); + + expect(CustomFields::form()->forModel(Post::class)->onlySections([])->values()) + ->toHaveCount(1); + }); + + it('returns an empty collection instead of throwing when a section scope is requested', function (): void { + CustomField::factory()->create([ + 'entity_type' => Post::class, + 'name' => 'Alpha', + 'code' => 'alpha_flat', + 'type' => 'text', + ]); + + /* + * SQLite falls back to treating an unresolvable double-quoted identifier as a + * string literal instead of raising "no such column" (the error MySQL, the + * project's production driver, actually raises), so a dropped-column WHERE clause + * silently matches zero rows here either way. A query-log assertion is the + * driver-agnostic way to prove the guard short-circuits before any query runs, + * rather than merely happening to agree with the guarded result by accident. + */ + $customFieldsTableQueried = false; + + DB::listen(function (QueryExecuted $query) use (&$customFieldsTableQueried): void { + if (str_contains($query->sql, 'custom_fields')) { + $customFieldsTableQueried = true; + } + }); + + $result = CustomFields::form()->forModel(Post::class)->onlySections([1])->values(); + + expect($customFieldsTableQueried)->toBeFalse() + ->and($result)->toHaveCount(0); + }); +}); From b293479886ac3ebf83724f3cc50c8ed43f66bbaf Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 17:27:16 +0400 Subject: [PATCH 04/16] feat: thread section scope through form and infolist containers 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(). --- .../Integration/Builders/FormBuilder.php | 3 +- .../Integration/Builders/FormContainer.php | 14 +++++ .../Integration/Builders/InfolistBuilder.php | 3 +- .../Builders/InfolistContainer.php | 14 +++++ tests/Feature/ConsumerScopeHooksTest.php | 58 +++++++++++++++++++ 5 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/Filament/Integration/Builders/FormBuilder.php b/src/Filament/Integration/Builders/FormBuilder.php index 4e1c4a7e..62bf6e46 100644 --- a/src/Filament/Integration/Builders/FormBuilder.php +++ b/src/Filament/Integration/Builders/FormBuilder.php @@ -24,7 +24,8 @@ public function build(): Grid $container = FormContainer::make() ->forModel($this->explicitModel ?? null) ->only($this->only) - ->except($this->except); + ->except($this->except) + ->onlySections($this->onlySections); // Only set withoutSections if explicitly configured if ($this->withoutSections !== null) { diff --git a/src/Filament/Integration/Builders/FormContainer.php b/src/Filament/Integration/Builders/FormContainer.php index d628ad71..66c1c1f4 100644 --- a/src/Filament/Integration/Builders/FormContainer.php +++ b/src/Filament/Integration/Builders/FormContainer.php @@ -15,6 +15,9 @@ final class FormContainer extends Grid private array $only = []; + /** @var array */ + private array $onlySections = []; + private ?bool $withoutSections = null; public static function make(array|int|null $columns = 12): static @@ -52,6 +55,16 @@ public function only(array $fieldCodes): static return $this; } + /** + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + public function withoutSections(bool $withoutSections = true): static { $this->withoutSections = $withoutSections; @@ -79,6 +92,7 @@ private function generateSchema(): array ->withoutSections($withoutSections) ->only($this->only) ->except($this->except) + ->onlySections($this->onlySections) ->values() ->toArray(); } diff --git a/src/Filament/Integration/Builders/InfolistBuilder.php b/src/Filament/Integration/Builders/InfolistBuilder.php index 4d5973d8..972d8eb8 100644 --- a/src/Filament/Integration/Builders/InfolistBuilder.php +++ b/src/Filament/Integration/Builders/InfolistBuilder.php @@ -31,7 +31,8 @@ public function build(): InfolistContainer ->hiddenLabels($this->hiddenLabels) ->visibleWhenFilled($this->visibleWhenFilled) ->only($this->only) - ->except($this->except); + ->except($this->except) + ->onlySections($this->onlySections); // Only set withoutSections if explicitly configured if ($this->withoutSections !== null) { diff --git a/src/Filament/Integration/Builders/InfolistContainer.php b/src/Filament/Integration/Builders/InfolistContainer.php index 1e5942c8..7f7a37d8 100644 --- a/src/Filament/Integration/Builders/InfolistContainer.php +++ b/src/Filament/Integration/Builders/InfolistContainer.php @@ -16,6 +16,9 @@ final class InfolistContainer extends Grid private array $only = []; + /** @var array */ + private array $onlySections = []; + private bool $hiddenLabels = false; private bool $visibleWhenFilled = false; @@ -57,6 +60,16 @@ public function only(array $fieldCodes): static return $this; } + /** + * @param array $sectionIds + */ + public function onlySections(array $sectionIds): static + { + $this->onlySections = $sectionIds; + + return $this; + } + public function hiddenLabels(bool $hiddenLabels = true): static { $this->hiddenLabels = $hiddenLabels; @@ -98,6 +111,7 @@ private function generateSchema(): array ->forModel($model) ->only($this->only) ->except($this->except) + ->onlySections($this->onlySections) ->hiddenLabels($this->hiddenLabels) ->visibleWhenFilled($this->visibleWhenFilled) ->withoutSections($withoutSections); diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index 37b367f1..b89766d1 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -2,8 +2,10 @@ declare(strict_types=1); +use Filament\Forms\Components\Field; use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; +use Filament\Schemas\Schema as FilamentSchema; use Illuminate\Database\Events\QueryExecuted; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; @@ -16,6 +18,8 @@ use Relaticle\CustomFields\Models\CustomField; use Relaticle\CustomFields\Models\CustomFieldSection; use Relaticle\CustomFields\Tests\Fixtures\Models\Post; +use Relaticle\CustomFields\Tests\Fixtures\Models\User; +use Relaticle\CustomFields\Tests\Fixtures\Resources\Posts\Pages\CreatePost; /** * Consumer scoping hooks let an app (e.g. a versioned-form builder) constrain the @@ -254,3 +258,57 @@ function availableFields(VisibilityComponent $component): array ->and($result)->toHaveCount(0); }); }); + +describe('FormContainer/InfolistContainer onlySections() scope', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + + $this->actingAs(User::factory()->create()); + }); + + /* + * Filament's getFlatComponents() keys its result by component statePath, so two + * fields sharing a code would collapse into a single array entry regardless of + * onlySections() — a false negative unrelated to scoping. Distinct codes per + * section are what let the assertion actually discriminate scoped vs. unscoped. + */ + it('honors the section scope through build(), not just values()', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built A', 'code' => 'built_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built B', 'code' => 'built_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Built A Field', + 'code' => 'built_a_field', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Built B Field', + 'code' => 'built_b_field', + 'type' => 'text', + ]); + + $container = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->build(); + + $schema = FilamentSchema::make(livewire(CreatePost::class)->instance()) + ->model(Post::class) + ->components([$container]); + + $fieldNames = collect($schema->getFlatComponents()) + ->filter(fn (object $component): bool => $component instanceof Field) + ->map(fn (Field $component): string => $component->getName()); + + expect($fieldNames)->toHaveCount(1) + ->and($fieldNames)->toContain('custom_fields.built_a_field') + ->and($fieldNames)->not->toContain('custom_fields.built_b_field'); + }); +}); From 17dbdc15ebec8a7015ad0d8d263e16fca429723e Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 17:43:09 +0400 Subject: [PATCH 05/16] feat: add uniqueness scope hook to CodeGenerator --- src/Livewire/Concerns/CreatesCustomFields.php | 2 +- src/Support/CodeGenerator.php | 45 ++++++++-- tests/Feature/ConsumerScopeHooksTest.php | 89 +++++++++++++++++++ 3 files changed, 128 insertions(+), 8 deletions(-) diff --git a/src/Livewire/Concerns/CreatesCustomFields.php b/src/Livewire/Concerns/CreatesCustomFields.php index 8c64245b..e67e03ea 100644 --- a/src/Livewire/Concerns/CreatesCustomFields.php +++ b/src/Livewire/Concerns/CreatesCustomFields.php @@ -20,7 +20,7 @@ protected function mutateFieldData(array $data, string $entityType, int|string|n } if (FeatureManager::isEnabled(CustomFieldsFeature::FIELD_CODE_AUTO_GENERATE) && blank($data['code'] ?? null)) { - $data['code'] = CodeGenerator::generateUniqueFieldCode($data['name'], $entityType); + $data['code'] = CodeGenerator::generateUniqueFieldCode($data['name'], $entityType, sectionId: $sectionId); } $result = [ diff --git a/src/Support/CodeGenerator.php b/src/Support/CodeGenerator.php index 0cdeced4..25412b47 100644 --- a/src/Support/CodeGenerator.php +++ b/src/Support/CodeGenerator.php @@ -4,6 +4,8 @@ namespace Relaticle\CustomFields\Support; +use Closure; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Str; use Relaticle\CustomFields\CustomFields; use Relaticle\CustomFields\Enums\CustomFieldsFeature; @@ -15,6 +17,9 @@ */ final class CodeGenerator { + /** @var (Closure(string, string, int|string|null): (Closure(Builder): Builder)|null)|null */ + private static ?Closure $uniquenessScopeResolver = null; + /** * Generate a slug-style code from a name. */ @@ -23,10 +28,24 @@ public static function generateFromName(string $name): string return Str::of($name)->slug('_')->toString(); } + /** + * Narrow the uniqueness check to a subset of rows. + * + * The callback receives the entity type, either 'field' or 'section', and the section + * the code is being generated within (null when there is none). It returns a query + * scope closure, or null to leave the check global. + * + * @param (Closure(string, string, int|string|null): (Closure(Builder): Builder)|null)|null $callback + */ + public static function resolveUniquenessScopeUsing(?Closure $callback): void + { + self::$uniquenessScopeResolver = $callback; + } + /** * Generate a unique code for a custom field within an entity type. */ - public static function generateUniqueFieldCode(string $name, string $entityType, ?int $ignoreId = null): string + public static function generateUniqueFieldCode(string $name, string $entityType, ?int $ignoreId = null, int|string|null $sectionId = null): string { $baseCode = self::generateFromName($name); @@ -34,14 +53,15 @@ public static function generateUniqueFieldCode(string $name, string $entityType, $baseCode, $entityType, 'field', - $ignoreId + $ignoreId, + $sectionId ); } /** * Generate a unique code for a section within an entity type. */ - public static function generateUniqueSectionCode(string $name, string $entityType, ?int $ignoreId = null): string + public static function generateUniqueSectionCode(string $name, string $entityType, ?int $ignoreId = null, int|string|null $sectionId = null): string { $baseCode = self::generateFromName($name); @@ -49,7 +69,8 @@ public static function generateUniqueSectionCode(string $name, string $entityTyp $baseCode, $entityType, 'section', - $ignoreId + $ignoreId, + $sectionId ); } @@ -60,12 +81,13 @@ private static function ensureUniqueCode( string $baseCode, string $entityType, string $type, - ?int $ignoreId = null + ?int $ignoreId = null, + int|string|null $sectionId = null ): string { $code = $baseCode; $counter = 1; - while (self::codeExists($code, $entityType, $type, $ignoreId)) { + while (self::codeExists($code, $entityType, $type, $ignoreId, $sectionId)) { $code = sprintf('%s_%d', $baseCode, $counter); $counter++; } @@ -80,7 +102,8 @@ private static function codeExists( string $code, string $entityType, string $type, - ?int $ignoreId = null + ?int $ignoreId = null, + int|string|null $sectionId = null ): bool { $model = $type === 'field' ? CustomFields::newCustomFieldModel() @@ -95,6 +118,14 @@ private static function codeExists( $query->where($model->getKeyName(), '!=', $ignoreId); } + if (self::$uniquenessScopeResolver !== null) { + $scope = (self::$uniquenessScopeResolver)($entityType, $type, $sectionId); + + if ($scope !== null) { + $scope($query); + } + } + if (FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY)) { $query->where( config('custom-fields.database.column_names.tenant_foreign_key'), diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index b89766d1..079762f0 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -6,6 +6,7 @@ use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; use Filament\Schemas\Schema as FilamentSchema; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Events\QueryExecuted; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\DB; @@ -17,6 +18,7 @@ use Relaticle\CustomFields\Filament\Management\Schemas\FieldForm; use Relaticle\CustomFields\Models\CustomField; use Relaticle\CustomFields\Models\CustomFieldSection; +use Relaticle\CustomFields\Support\CodeGenerator; use Relaticle\CustomFields\Tests\Fixtures\Models\Post; use Relaticle\CustomFields\Tests\Fixtures\Models\User; use Relaticle\CustomFields\Tests\Fixtures\Resources\Posts\Pages\CreatePost; @@ -36,6 +38,7 @@ afterEach(function (): void { VisibilityComponent::resolveAvailableFieldsScopeUsing(null); FieldForm::resolveUniqueRuleModifierUsing(null); + CodeGenerator::resolveUniquenessScopeUsing(null); }); function nullGet(): Get @@ -120,6 +123,92 @@ function availableFields(VisibilityComponent $component): array }); }); +describe('CodeGenerator uniqueness scope resolver', function (): void { + it('suffixes a colliding code when no resolver is registered (backward compatible)', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Default', 'code' => 'scope_default']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class)) + ->toBe('hmis_id_1'); + }); + + it('returns the base code when the collision is outside the registered scope', function (): void { + $outside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Outside', 'code' => 'scope_outside']); + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Inside', 'code' => 'scope_inside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $outside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id'); + }); + + it('still suffixes when the collision is inside the registered scope', function (): void { + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Inside Only', 'code' => 'scope_inside_only']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $inside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id_1'); + }); + + it('passes null as the section id for the sectionless caller, matching current behavior', function (): void { + $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Sectionless', 'code' => 'sectionless']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + $receivedSectionId = 'not-called'; + + CodeGenerator::resolveUniquenessScopeUsing( + function (string $entityType, string $type, int|string|null $sectionId) use (&$receivedSectionId): ?Closure { + $receivedSectionId = $sectionId; + + return null; + } + ); + + CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class); + + expect($receivedSectionId)->toBeNull(); + }); +}); + describe('BaseBuilder onlySections() scope', function (): void { beforeEach(function (): void { config()->set('custom-fields.features', FeatureConfigurator::configure() From 4ea007e65ed44eaa662f0b0a4a3ebb8a87f417b2 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 18:27:40 +0400 Subject: [PATCH 06/16] fix: honor table prefix and guard duplicate rows in relax-unique-key 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(). --- .../relax_custom_fields_unique_key.php | 68 ++++++- ...elaxCustomFieldsUniqueKeyMigrationTest.php | 171 ++++++++++++++++++ 2 files changed, 238 insertions(+), 1 deletion(-) create mode 100644 tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php diff --git a/database/migrations/relax_custom_fields_unique_key.php b/database/migrations/relax_custom_fields_unique_key.php index daf27d7b..37c9b5a9 100644 --- a/database/migrations/relax_custom_fields_unique_key.php +++ b/database/migrations/relax_custom_fields_unique_key.php @@ -4,6 +4,7 @@ use Illuminate\Database\Migrations\Migration; use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Schema; use Relaticle\CustomFields\Enums\CustomFieldsFeature; use Relaticle\CustomFields\FeatureSystem\FeatureManager; @@ -19,6 +20,15 @@ * * custom_field_sections is deliberately left untouched: onlySections() scopes by section * id, never by section code, so nothing here requires sections to share a code. + * + * custom_field_section_id is nullable, and both MySQL and Postgres treat NULL as distinct + * in a unique index — including within a composite one. So after this migration, two rows + * that both have custom_field_section_id IS NULL can still share (code, entity_type[, + * tenant]): the wide key does not constrain them, because NULL never equals NULL for + * uniqueness purposes. That's a protection existing installs have today (every row is + * globally unique per entity type) and silently lose once this ships. There is no schema + * workaround for this — a consumer that needs collision protection for sectionless fields + * must keep them out of this data shape or enforce it at the application layer. */ return new class extends Migration { @@ -34,6 +44,8 @@ public function up(): void public function down(): void { + $this->assertNoDuplicatesUnderNarrowKey(); + $this->swapUniqueKey( from: $this->wideColumns(), fromIndexName: $this->wideIndexName(), @@ -42,6 +54,42 @@ public function down(): void ); } + /** + * MySQL runs each ALTER TABLE as its own auto-committing DDL statement, so dropping + * the wide key and adding the narrow one are not transactional together. If rows exist + * that share (code, entity_type[, tenant]) across different sections — exactly the + * shape the wide key exists to allow — the DROP succeeds and the subsequent ADD fails + * on the duplicate, leaving the table with neither unique key. Check first and abort + * before touching anything. + */ + private function assertNoDuplicatesUnderNarrowKey(): void + { + $table = config('custom-fields.database.table_names.custom_fields'); + + if (! Schema::hasColumn($table, 'custom_field_section_id')) { + return; + } + + $columns = $this->narrowColumns(); + + $duplicateCodes = DB::table($table) + ->select($columns) + ->groupBy($columns) + ->havingRaw('count(*) > 1') + ->pluck('code'); + + if ($duplicateCodes->isEmpty()) { + return; + } + + throw new RuntimeException(sprintf( + 'Cannot roll back the custom_fields unique key: %d code(s) — including "%s" — are shared by more than one row for the same (%s), only differing by custom_field_section_id. onlySections() allows this under the wide key, but the narrow key being restored cannot. Resolve or remove the duplicate rows before rolling back this migration.', + $duplicateCodes->count(), + $duplicateCodes->first(), + implode(', ', $columns) + )); + } + /** * Drops $from's unique key if present and adds $to's if absent. Shared by both * directions: up() widens (code, entity_type[, tenant]) to also include @@ -107,12 +155,30 @@ private function wideIndexName(): string * so the drop target matches exactly what create_custom_fields_table.php produced, * without hardcoding a name that would drift if a configured column name changes. * + * Laravel's shipped config/database.php enables `prefix_indexes` for mysql and pgsql, + * which makes Blueprint::createIndexName() fold the connection's table prefix into the + * name it generates. Skipping that step here would compute a drop target that never + * matches the real index name on a prefixed install, so the drop would silently no-op. + * * @param array $columns */ private function defaultUniqueIndexName(string $table, array $columns): string { - $index = strtolower($table.'_'.implode('_', $columns).'_unique'); + $connection = Schema::getConnection(); + + $prefixedTable = $connection->getConfig('prefix_indexes') + ? $this->applyTablePrefix($table, $connection->getTablePrefix()) + : $table; + + $index = strtolower($prefixedTable.'_'.implode('_', $columns).'_unique'); return str_replace(['-', '.'], '_', $index); } + + private function applyTablePrefix(string $table, string $prefix): string + { + return str_contains($table, '.') + ? substr_replace($table, '.'.$prefix, strrpos($table, '.'), 1) + : $prefix.$table; + } }; diff --git a/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php new file mode 100644 index 00000000..536dd5e1 --- /dev/null +++ b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php @@ -0,0 +1,171 @@ +migration = require __DIR__.'/../../database/migrations/relax_custom_fields_unique_key.php'; + $this->table = config('custom-fields.database.table_names.custom_fields'); +}); + +function customFieldsIndexNames(string $table): Collection +{ + return collect(Schema::getIndexes($table))->pluck('name'); +} + +it('down() restores the narrow key and drops the wide one', function (): void { + expect(customFieldsIndexNames($this->table)) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); + + $this->migration->down(); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('custom_fields_code_entity_type_unique') + ->not->toContain('cf_code_entity_section_unique'); +}); + +it('up() restores the wide key after a down() round trip', function (): void { + $this->migration->down(); + $this->migration->up(); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); +}); + +it('up() is idempotent when the wide index already exists', function (): void { + expect(fn () => $this->migration->up())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table)->filter( + fn (string $name): bool => $name === 'cf_code_entity_section_unique' + ))->toHaveCount(1); +}); + +it('up() is idempotent when the narrow index is already gone, the state a consumer who hand-applied an equivalent change is in', function (): void { + Schema::table($this->table, fn (Blueprint $table) => $table->dropUnique('cf_code_entity_section_unique')); + + expect(customFieldsIndexNames($this->table)) + ->not->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); + + expect(fn () => $this->migration->up())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table))->toContain('cf_code_entity_section_unique'); +}); + +it('down() aborts before dropping anything when rows share a code across sections', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + expect(fn () => $this->migration->down()) + ->toThrow(RuntimeException::class, 'duplicate_code'); + + $indexes = customFieldsIndexNames($this->table); + + expect($indexes) + ->toContain('cf_code_entity_section_unique') + ->not->toContain('custom_fields_code_entity_type_unique'); +}); + +it('down() succeeds once the duplicate rows are resolved', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'code' => 'section_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + $duplicate = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'code' => 'duplicate_code', + 'type' => 'text', + ]); + + $duplicate->update(['code' => 'no_longer_duplicate']); + + expect(fn () => $this->migration->down())->not->toThrow(Throwable::class); + + expect(customFieldsIndexNames($this->table)) + ->toContain('custom_fields_code_entity_type_unique') + ->not->toContain('cf_code_entity_section_unique'); +}); + +it('honors prefix_indexes and the connection table prefix when computing the drop-target index name', function (): void { + config()->set('database.connections.prefixed_for_test', [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => 'wp_', + 'prefix_indexes' => true, + ]); + + $originalDefault = config('database.default'); + config()->set('database.default', 'prefixed_for_test'); + + try { + $connection = DB::connection('prefixed_for_test'); + $connection->useDefaultSchemaGrammar(); + $columns = ['code', 'entity_type', 'custom_field_section_id']; + + $defaultUniqueIndexName = new ReflectionMethod($this->migration, 'defaultUniqueIndexName'); + $defaultUniqueIndexName->setAccessible(true); + $actual = $defaultUniqueIndexName->invoke($this->migration, 'custom_fields', $columns); + + $blueprint = new Blueprint($connection, 'custom_fields'); + $createIndexName = new ReflectionMethod($blueprint, 'createIndexName'); + $createIndexName->setAccessible(true); + $expected = $createIndexName->invoke($blueprint, 'unique', $columns); + + expect($actual)->toBe($expected) + ->and($actual)->toStartWith('wp_custom_fields_'); + } finally { + config()->set('database.default', $originalDefault); + DB::purge('prefixed_for_test'); + } +}); + +it('computes the same index name with no prefix configured, matching current behavior', function (): void { + $columns = ['code', 'entity_type']; + + $defaultUniqueIndexName = new ReflectionMethod($this->migration, 'defaultUniqueIndexName'); + $defaultUniqueIndexName->setAccessible(true); + $actual = $defaultUniqueIndexName->invoke($this->migration, 'custom_fields', $columns); + + expect($actual)->toBe('custom_fields_code_entity_type_unique'); +}); From b33d53549e5cedda531d16e3825df125b706fc94 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 18:27:46 +0400 Subject: [PATCH 07/16] fix: use scope closure return value in CodeGenerator, drop unused param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- src/Support/CodeGenerator.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Support/CodeGenerator.php b/src/Support/CodeGenerator.php index 25412b47..4c46fcee 100644 --- a/src/Support/CodeGenerator.php +++ b/src/Support/CodeGenerator.php @@ -61,7 +61,7 @@ public static function generateUniqueFieldCode(string $name, string $entityType, /** * Generate a unique code for a section within an entity type. */ - public static function generateUniqueSectionCode(string $name, string $entityType, ?int $ignoreId = null, int|string|null $sectionId = null): string + public static function generateUniqueSectionCode(string $name, string $entityType, ?int $ignoreId = null): string { $baseCode = self::generateFromName($name); @@ -69,8 +69,7 @@ public static function generateUniqueSectionCode(string $name, string $entityTyp $baseCode, $entityType, 'section', - $ignoreId, - $sectionId + $ignoreId ); } @@ -122,7 +121,7 @@ private static function codeExists( $scope = (self::$uniquenessScopeResolver)($entityType, $type, $sectionId); if ($scope !== null) { - $scope($query); + $query = $scope($query); } } From 01a1ce4588f6b9a73e6b933279fae989b1c58025 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 18:27:53 +0400 Subject: [PATCH 08/16] fix: notify instead of 500ing when a section drag creates a duplicate 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. --- resources/lang/en/custom-fields.php | 1 + src/Livewire/ManageCustomFieldSection.php | 32 +++++++++++++++++ .../Pages/CustomFieldsFieldManagementTest.php | 35 +++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/resources/lang/en/custom-fields.php b/resources/lang/en/custom-fields.php index 2fed43ef..1de8f52f 100644 --- a/resources/lang/en/custom-fields.php +++ b/resources/lang/en/custom-fields.php @@ -29,6 +29,7 @@ 'default_section_name' => 'Default', 'notifications' => [ 'created' => 'Section created', + 'duplicate_field_code' => 'This section already has a field with that code. Rename one of them before moving it here.', ], 'actions' => [ 'activate' => 'Activate', diff --git a/src/Livewire/ManageCustomFieldSection.php b/src/Livewire/ManageCustomFieldSection.php index 8d75c973..98cb829f 100644 --- a/src/Livewire/ManageCustomFieldSection.php +++ b/src/Livewire/ManageCustomFieldSection.php @@ -11,9 +11,11 @@ use Filament\Actions\Contracts\HasActions; use Filament\Forms\Concerns\InteractsWithForms; use Filament\Forms\Contracts\HasForms; +use Filament\Notifications\Notification; use Filament\Support\Enums\Size; use Filament\Support\Enums\Width; use Illuminate\Contracts\View\View; +use Illuminate\Database\Eloquent\Model; use Livewire\Component; use Relaticle\CustomFields\CustomFields; use Relaticle\CustomFields\CustomFieldsPlugin; @@ -45,6 +47,22 @@ public static function resolveUniqueRuleModifierUsing(?Closure $callback): void public function updateFieldsOrder(int|string $sectionId, array $fields): void { $model = CustomFields::newCustomFieldModel(); + + /* + * 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 drop target could never + * already hold a colliding code. Now it can — check before writing, or the second + * update() in the loop below throws an unhandled QueryException. + */ + if ($this->fieldsHaveDuplicateCode($model, $fields)) { + Notification::make() + ->danger() + ->title(__('custom-fields::custom-fields.section.notifications.duplicate_field_code')) + ->send(); + + return; + } + foreach ($fields as $index => $field) { $model->query() ->withDeactivated() @@ -59,6 +77,20 @@ public function updateFieldsOrder(int|string $sectionId, array $fields): void $this->dispatch('fields-reordered')->to('manage-custom-field-section'); } + /** + * @param array $fieldIds + */ + private function fieldsHaveDuplicateCode(Model $model, array $fieldIds): bool + { + return $model->query() + ->withDeactivated() + ->whereIn($model->getKeyName(), $fieldIds) + ->select('code') + ->groupBy('code') + ->havingRaw('count(*) > 1') + ->exists(); + } + public function actions(): ?ActionGroup { if ($this->section->hasSystemDefinedFields()) { diff --git a/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php b/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php index 3fdd2c73..1aa53c33 100644 --- a/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php +++ b/tests/Feature/Admin/Pages/CustomFieldsFieldManagementTest.php @@ -188,6 +188,41 @@ ]); }); + it('notifies instead of 500ing when a drag would create a duplicate code within the target section', function (): void { + $targetSection = $this->section; + $sourceSection = CustomFieldSection::factory() + ->forEntityType($this->userEntityType) + ->create(); + + $existingField = CustomField::factory() + ->ofType('text') + ->create([ + 'custom_field_section_id' => $targetSection->getKey(), + 'entity_type' => $this->userEntityType, + 'code' => 'shared_code', + 'sort_order' => 0, + ]); + + $draggedField = CustomField::factory() + ->ofType('text') + ->create([ + 'custom_field_section_id' => $sourceSection->getKey(), + 'entity_type' => $this->userEntityType, + 'code' => 'shared_code', + 'sort_order' => 0, + ]); + + livewire(ManageCustomFieldSection::class, [ + 'section' => $targetSection, + 'entityType' => $this->userEntityType, + ]) + ->call('updateFieldsOrder', $targetSection->getKey(), [$existingField->getKey(), $draggedField->getKey()]) + ->assertNotified(); + + expect($draggedField->fresh()) + ->custom_field_section_id->toBe($sourceSection->getKey()); + }); + }); describe('ManageCustomField - Field Actions', function (): void { From 4ebc7088f09c771e65bc449c0e8ea214691299bf Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 18:28:01 +0400 Subject: [PATCH 09/16] test: cover InfolistContainer scope threading, fix two weak assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../Integration/Builders/BaseBuilder.php | 8 ++ tests/Feature/ConsumerScopeHooksTest.php | 109 ++++++++++++++++-- 2 files changed, 108 insertions(+), 9 deletions(-) diff --git a/src/Filament/Integration/Builders/BaseBuilder.php b/src/Filament/Integration/Builders/BaseBuilder.php index 99063dd5..d8817249 100644 --- a/src/Filament/Integration/Builders/BaseBuilder.php +++ b/src/Filament/Integration/Builders/BaseBuilder.php @@ -92,6 +92,14 @@ public function only(array $fieldCodes): static * sections. Scoping structurally lets two sections carry the same code without one * bleeding into the other's schema. * + * IMPORTANT: this only narrows resolution (what gets loaded onto the form/infolist). + * It does not change how UsesCustomFields::saveCustomFields() saves — that method + * writes by code against the model's customFields() relationship. If two sections + * share a code and customFields() isn't scoped to match, saveCustomFields() will + * write the same submitted value to both field rows. Scoping resolution alone is not + * sufficient; the model's customFields() relation must be scoped to the same + * section(s) too. See the "Builder Scoping" docs page. + * * @param array $sectionIds */ public function onlySections(array $sectionIds): static diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index 079762f0..133528b7 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -3,6 +3,7 @@ declare(strict_types=1); use Filament\Forms\Components\Field; +use Filament\Infolists\Components\Entry; use Filament\Schemas\Components\Component; use Filament\Schemas\Components\Utilities\Get; use Filament\Schemas\Schema as FilamentSchema; @@ -62,6 +63,23 @@ function availableFields(VisibilityComponent $component): array return $method->invoke($component, nullGet()); } +/** + * With SYSTEM_SECTIONS enabled, FormBuilder::values()/InfolistBuilder::values() return one + * component per section, not one per field, so asserting a count on the outer collection + * can't tell "the section kept N fields" from "there are N sections" apart. Reach into the + * section component's raw childComponents (set by ->schema()) to count what it actually + * carries, without needing the full Livewire-attached schema tree just to read it back. + * + * @return array + */ +function sectionFieldComponents(Component $section): array +{ + $property = new ReflectionProperty($section, 'childComponents'); + $property->setAccessible(true); + + return $property->getValue($section)['default'] ?? []; +} + describe('VisibilityComponent available-fields scope resolver', function (): void { it('lists all entity fields when no resolver is registered (backward compatible)', function (): void { $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); @@ -182,7 +200,35 @@ function availableFields(VisibilityComponent $component): array ->toBe('hmis_id_1'); }); - it('passes null as the section id for the sectionless caller, matching current behavior', function (): void { + it('applies a scope closure that returns a new builder instance instead of mutating in place', function (): void { + $outside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Cloned Outside', 'code' => 'cloned_outside']); + $inside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Cloned Inside', 'code' => 'cloned_inside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $outside->id, + 'entity_type' => Post::class, + 'name' => 'HMIS ID', + 'code' => 'hmis_id', + 'type' => 'text', + ]); + + /* + * Builder::where() mutates and returns the same instance, so a mutation-style scope + * closure would pass this even if the code discarded its return value. clone() is + * what actually discriminates: it hands back a distinct instance, so only code that + * reassigns $query to the closure's return value picks the narrowed clone up. + */ + CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $type === 'field' && $sectionId !== null + ? fn (Builder $query): Builder => $query->clone()->where('custom_field_section_id', $sectionId) + : null + ); + + expect(CodeGenerator::generateUniqueFieldCode('HMIS ID', Post::class, sectionId: $inside->id)) + ->toBe('hmis_id'); + }); + + it('passes null as the section id to the resolver when generateUniqueFieldCode() is called without one', function (): void { $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Sectionless', 'code' => 'sectionless']); CustomField::factory()->create([ @@ -276,13 +322,20 @@ function (string $entityType, string $type, int|string|null $sectionId) use (&$r ]); } - expect( - CustomFields::form() - ->forModel(Post::class) - ->onlySections([$section->id]) - ->only(['keep_me']) - ->values() - )->toHaveCount(1); + /* + * With SYSTEM_SECTIONS enabled, values() returns one component per section (there is + * only ever the one section here), so asserting a count on the outer collection can't + * tell "only() dropped a field" from "only() dropped nothing" apart — it stays 1 + * either way. Assert on the section's own field count instead so this discriminates. + */ + $sections = CustomFields::form() + ->forModel(Post::class) + ->onlySections([$section->id]) + ->only(['keep_me']) + ->values(); + + expect($sections)->toHaveCount(1) + ->and(sectionFieldComponents($sections->first()))->toHaveCount(1); }); }); @@ -363,7 +416,7 @@ function (string $entityType, string $type, int|string|null $sectionId) use (&$r * onlySections() — a false negative unrelated to scoping. Distinct codes per * section are what let the assertion actually discriminate scoped vs. unscoped. */ - it('honors the section scope through build(), not just values()', function (): void { + it('honors the section scope through FormBuilder::build(), not just values()', function (): void { $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built A', 'code' => 'built_a']); $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Built B', 'code' => 'built_b']); @@ -400,4 +453,42 @@ function (string $entityType, string $type, int|string|null $sectionId) use (&$r ->and($fieldNames)->toContain('custom_fields.built_a_field') ->and($fieldNames)->not->toContain('custom_fields.built_b_field'); }); + + it('honors the section scope through InfolistBuilder::build(), not just values()', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Info Built A', 'code' => 'info_built_a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Info Built B', 'code' => 'info_built_b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Info Built A Field', + 'code' => 'info_built_a_field', + 'type' => 'text', + ]); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Info Built B Field', + 'code' => 'info_built_b_field', + 'type' => 'text', + ]); + + $container = CustomFields::infolist() + ->forModel(Post::class) + ->onlySections([$sectionA->id]) + ->build(); + + $schema = FilamentSchema::make(livewire(CreatePost::class)->instance()) + ->model(Post::class) + ->components([$container]); + + $entryNames = collect($schema->getFlatComponents()) + ->filter(fn (object $component): bool => $component instanceof Entry) + ->map(fn (Entry $component): string => $component->getName()); + + expect($entryNames)->toHaveCount(1) + ->and($entryNames)->toContain('custom_fields.info_built_a_field') + ->and($entryNames)->not->toContain('custom_fields.info_built_b_field'); + }); }); From 8aa2f4ed246cfcacd29ec35c222ce5a769e51568 Mon Sep 17 00:00:00 2001 From: Manuk Date: Fri, 7 Aug 2026 18:28:10 +0400 Subject: [PATCH 10/16] docs: correct data-model invariants, document builder scoping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../1.getting-started/1.installation.md | 5 +- .../1.getting-started/3.upgrade-guide.md | 29 +++++++ docs/content/2.essentials/6.data-model.md | 4 +- .../content/2.essentials/7.builder-scoping.md | 83 +++++++++++++++++++ 4 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 docs/content/2.essentials/7.builder-scoping.md diff --git a/docs/content/1.getting-started/1.installation.md b/docs/content/1.getting-started/1.installation.md index fb4f4908..a4ed1f49 100644 --- a/docs/content/1.getting-started/1.installation.md +++ b/docs/content/1.getting-started/1.installation.md @@ -97,6 +97,5 @@ php artisan vendor:publish --tag="custom-fields-migrations" php artisan migrate ``` -v3.7.0, for example, relaxes the `custom_fields` unique key so a field code can be reused -across sections (needed for `onlySections()`). Existing installs need this step to pick -that change up. +See the [upgrade guide](/getting-started/upgrade-guide#picking-up-new-migrations) for a +concrete example of a release that requires this step. diff --git a/docs/content/1.getting-started/3.upgrade-guide.md b/docs/content/1.getting-started/3.upgrade-guide.md index e1a7bf93..75dbb6d3 100644 --- a/docs/content/1.getting-started/3.upgrade-guide.md +++ b/docs/content/1.getting-started/3.upgrade-guide.md @@ -45,6 +45,35 @@ php artisan custom-fields:upgrade --skip=clear-caches php artisan custom-fields:upgrade --skip=email-format,phone-format ``` +## Picking Up New Migrations + +Package migrations are not run automatically by `php artisan migrate` after a version +bump — they're only copied into your app on first install, or when you explicitly +republish them: + +```bash +php artisan vendor:publish --tag="custom-fields-migrations" +php artisan migrate +``` + +For example, a release may relax the `custom_fields` unique key from +`(code, entity_type[, tenant])` to `(code, entity_type[, tenant], custom_field_section_id)` +so a field code can be reused across different sections — the shape `onlySections()` (see +[Builder Scoping](/essentials/builder-scoping)) relies on. Existing installs need the +republish-and-migrate step above to pick that change up; it is not applied automatically. + +**Before rolling that migration back**, resolve any rows that ended up sharing a code +across sections. The migration checks for this first and aborts with a clear error rather +than dropping the wide key and then failing to recreate the narrow one, which would leave +the table with no unique key at all. + +**NULL is not unique-constrained.** `custom_field_section_id` is nullable, and both MySQL +and Postgres treat `NULL` as distinct from every other value in a unique index — including +one that includes it. So after this migration, two sectionless fields +(`custom_field_section_id IS NULL`) can still share a code for the same entity type, a +collision the narrow key used to prevent. There is no schema-level fix for this; keep +sectionless codes unique at the application layer if you rely on that guarantee. + ## Breaking Changes ### High Impact diff --git a/docs/content/2.essentials/6.data-model.md b/docs/content/2.essentials/6.data-model.md index f130878d..62f90c62 100644 --- a/docs/content/2.essentials/6.data-model.md +++ b/docs/content/2.essentials/6.data-model.md @@ -27,7 +27,7 @@ The Custom Fields plugin employs a **Hybrid Entity-Attribute-Value (EAV) with Ty |--------|------|-------------| | `id` | bigint | Primary key | | `entity_type` | string | Polymorphic entity class | - | `code` | string | Unique identifier | + | `code` | string | Unique per entity type (+ tenant, when multi-tenancy is enabled) | | `name` | string | Display name | | `type` | string | Section type | | `width` | string | Section layout width (`CustomFieldWidth` enum: 25/33/50/66/75/100). Requires the `UI_SECTION_WIDTH_CONTROL` feature. | @@ -44,7 +44,7 @@ The Custom Fields plugin employs a **Hybrid Entity-Attribute-Value (EAV) with Ty | `id` | bigint | Primary key | | `custom_field_section_id` | bigint | Parent section | | `entity_type` | string | Polymorphic entity class | - | `code` | string | Unique identifier | + | `code` | string | Unique per entity type and section (+ tenant, when multi-tenancy is enabled) — not globally unique, so the same code can exist in two different sections. See [Builder Scoping](/essentials/builder-scoping) | | `name` | string | Display name | | `type` | string | Field type (text, number, etc.) | | `lookup_type` | string | For lookup fields | diff --git a/docs/content/2.essentials/7.builder-scoping.md b/docs/content/2.essentials/7.builder-scoping.md new file mode 100644 index 00000000..51dc6389 --- /dev/null +++ b/docs/content/2.essentials/7.builder-scoping.md @@ -0,0 +1,83 @@ +--- +title: Builder Scoping +description: Scope custom-field resolution to specific sections, and hook into code-uniqueness resolution +navigation: + icon: i-lucide-filter +--- + +## onlySections() + +By default, every builder (`CustomFields::form()`, `::infolist()`, `::table()`, +`::exporter()`, `::importer()`) resolves fields for the whole entity type. `onlySections()` +constrains that resolution to a specific set of `custom_field_sections` rows: + +```php +use Relaticle\CustomFields\Facades\CustomFields; + +CustomFields::form() + ->forModel($model) + ->onlySections([$sectionA->id, $sectionB->id]) + ->build(); +``` + +Passing `[]` (the default) means "no scope" — existing call sites that never call +`onlySections()` are unaffected. + +This exists for consumers that version their form definitions — for example, cloning a +section (and its fields) per form version. Field codes are normally unique per entity +type, so two versions of "the same field" would collide on `code` unless resolution is +scoped by section. `onlySections()` lets each version's builder only see its own +section(s), so the same code can live in more than one section without one bleeding into +the other's schema. + +`onlySections()` is inherited by every builder from `BaseBuilder`, including through +`FormBuilder::build()` / `InfolistBuilder::build()` — the scope is threaded through +`FormContainer` / `InfolistContainer` as well, so it applies whether you call `->build()` +or `->values()`. + +### The persistence contract — read this before relying on section-scoped codes + +`onlySections()` narrows *resolution* (which fields get loaded onto a form, infolist, or +table). It does not change how values are *saved*. `UsesCustomFields::saveCustomFields()` +iterates the model's custom-field-values relationship and writes each submitted value by +**field code**. If two sections share a code and that relationship isn't scoped to match +`onlySections()`, `saveCustomFields()` will silently write the same value to **both** +field rows — a data-corrupting outcome that has nothing to do with whether resolution +scoping itself is working correctly. + +If you use `onlySections()`, scope your model's custom-field-values relationship +(`customFields()` by default, or your override) to the same section(s). It is +entity-scoped by default and overridable per model. + +## CodeGenerator::resolveUniquenessScopeUsing() + +Auto-generated codes (`FIELD_CODE_AUTO_GENERATE`) are checked for collisions before use. +Register a callback to narrow that check the same way `onlySections()` narrows +resolution: + +```php +use Illuminate\Database\Eloquent\Builder; +use Relaticle\CustomFields\Support\CodeGenerator; + +CodeGenerator::resolveUniquenessScopeUsing( + fn (string $entityType, string $type, int|string|null $sectionId): ?Closure => $sectionId !== null + ? fn (Builder $query): Builder => $query->where('custom_field_section_id', $sectionId) + : null +); +``` + +The callback receives: + +- `$entityType` — the entity the field or section belongs to. +- `$type` — `'field'` or `'section'`, so you can scope differently per kind of code. +- `$sectionId` — the section the code is being generated within, or `null` when there + isn't one (for example, the sectionless field-management table). + +Return `null` to leave the uniqueness check global — the default, backward-compatible +behavior. Return a closure to narrow it: the closure receives the in-progress `Builder` +and must **return** the query to apply. `where()`-style mutation also works (it returns +the same instance), but a closure that hands back a different instance — e.g. +`$query->clone()->where(...)` — is honored too, since the return value is always what's +used. + +Register the callback once, typically in a service provider's `boot()` method. From 5d8dcf3178c14c249fcd8011db2fa0f96043e195 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 00:13:51 +0400 Subject: [PATCH 11/16] fix: add code-uniqueness rule modifier hook to FieldForm 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. --- src/Filament/Management/Schemas/FieldForm.php | 55 +++++++-- tests/Feature/ConsumerScopeHooksTest.php | 116 ++++++++++++++++++ 2 files changed, 162 insertions(+), 9 deletions(-) diff --git a/src/Filament/Management/Schemas/FieldForm.php b/src/Filament/Management/Schemas/FieldForm.php index c05ead07..7e7d7d0f 100644 --- a/src/Filament/Management/Schemas/FieldForm.php +++ b/src/Filament/Management/Schemas/FieldForm.php @@ -42,6 +42,9 @@ class FieldForm implements FormInterface /** @var ?Closure(?CustomFieldSection): ?Closure */ private static ?Closure $uniqueNameRuleModifierResolver = null; + /** @var ?Closure(?CustomFieldSection): ?Closure */ + private static ?Closure $uniqueCodeRuleModifierResolver = null; + /** * Register a resolver that scopes the field-name uniqueness rule beyond the default * entity-type (+ tenant) scope. The resolver receives the section the field belongs to @@ -56,6 +59,22 @@ public static function resolveUniqueRuleModifierUsing(?Closure $resolver): void self::$uniqueNameRuleModifierResolver = $resolver; } + /** + * Register a resolver that scopes the field-code uniqueness rule beyond the default + * entity-type (+ tenant) scope. Same contract as resolveUniqueRuleModifierUsing(), kept + * as a separate hook because code and name typically need different scoping: name is a + * cosmetic label a consumer may want unique per parent form only, while code is a + * stable identity other systems (e.g. reporting) key off of, so its scope is usually + * "everything except a defined set of related forms" rather than "this one form". + * Register once. + * + * @param ?Closure(?CustomFieldSection $section): ?Closure $resolver + */ + public static function resolveUniqueCodeRuleModifierUsing(?Closure $resolver): void + { + self::$uniqueCodeRuleModifierResolver = $resolver; + } + private static function resolveUniqueNameRuleModifier(?CustomFieldSection $section): ?Closure { if (self::$uniqueNameRuleModifierResolver instanceof Closure) { @@ -65,6 +84,15 @@ private static function resolveUniqueNameRuleModifier(?CustomFieldSection $secti return null; } + private static function resolveUniqueCodeRuleModifier(?CustomFieldSection $section): ?Closure + { + if (self::$uniqueCodeRuleModifierResolver instanceof Closure) { + return (self::$uniqueCodeRuleModifierResolver)($section); + } + + return null; + } + /** * Disable field when editing a system-defined custom field. */ @@ -137,6 +165,7 @@ private static function getValidationSchema(): array public static function schema(bool $withOptionsRelationship = true, ?CustomFieldSection $section = null): array { $uniqueNameRuleModifier = self::resolveUniqueNameRuleModifier($section); + $uniqueCodeRuleModifier = self::resolveUniqueCodeRuleModifier($section); $optionsRepeater = Repeater::make('options') ->table([ @@ -293,15 +322,23 @@ public static function schema(bool $withOptionsRelationship = true, ?CustomField table: CustomFields::customFieldModel(), column: 'code', ignoreRecord: true, - modifyRuleUsing: fn (Unique $rule, Get $get) => $rule - ->where('entity_type', $get('entity_type')) - ->when( - FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY), - fn (Unique $rule) => $rule->where( - config('custom-fields.database.column_names.tenant_foreign_key'), - TenantContextService::getCurrentTenantId() - ) - ) + modifyRuleUsing: function (Unique $rule, Get $get) use ($uniqueCodeRuleModifier): Unique { + $rule = $rule + ->where('entity_type', $get('entity_type')) + ->when( + FeatureManager::isEnabled(CustomFieldsFeature::SYSTEM_MULTI_TENANCY), + fn (Unique $rule) => $rule->where( + config('custom-fields.database.column_names.tenant_foreign_key'), + TenantContextService::getCurrentTenantId() + ) + ); + + if ($uniqueCodeRuleModifier instanceof Closure) { + return $uniqueCodeRuleModifier($rule, $get); + } + + return $rule; + } ) ->afterStateUpdated(function (Set $set, ?string $state): void { $set('code', Str::of($state)->slug('_')->toString()); diff --git a/tests/Feature/ConsumerScopeHooksTest.php b/tests/Feature/ConsumerScopeHooksTest.php index 133528b7..1c2fdd48 100644 --- a/tests/Feature/ConsumerScopeHooksTest.php +++ b/tests/Feature/ConsumerScopeHooksTest.php @@ -17,6 +17,8 @@ use Relaticle\CustomFields\FeatureSystem\FeatureConfigurator; use Relaticle\CustomFields\Filament\Management\Forms\Components\VisibilityComponent; use Relaticle\CustomFields\Filament\Management\Schemas\FieldForm; +use Relaticle\CustomFields\Livewire\ManageCustomField; +use Relaticle\CustomFields\Livewire\ManageCustomFieldSection; use Relaticle\CustomFields\Models\CustomField; use Relaticle\CustomFields\Models\CustomFieldSection; use Relaticle\CustomFields\Support\CodeGenerator; @@ -39,6 +41,7 @@ afterEach(function (): void { VisibilityComponent::resolveAvailableFieldsScopeUsing(null); FieldForm::resolveUniqueRuleModifierUsing(null); + FieldForm::resolveUniqueCodeRuleModifierUsing(null); CodeGenerator::resolveUniquenessScopeUsing(null); }); @@ -141,6 +144,119 @@ function sectionFieldComponents(Component $section): array }); }); +describe('FieldForm unique-code modifier resolver', function (): void { + beforeEach(function (): void { + config()->set('custom-fields.features', FeatureConfigurator::configure() + ->enable(CustomFieldsFeature::FIELD_CONDITIONAL_VISIBILITY, CustomFieldsFeature::SYSTEM_SECTIONS) + ); + + $this->actingAs(User::factory()->create()); + }); + + it('builds the field schema unchanged when no resolver is registered (backward compatible)', function (): void { + $schema = FieldForm::schema(); + + expect($schema)->toBeArray()->not->toBeEmpty(); + }); + + it('rejects a duplicate code across sections with no resolver registered (backward compatible)', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Existing', + 'code' => 'shared_code', + 'type' => 'text', + ]); + + livewire(ManageCustomFieldSection::class, ['entityType' => Post::class, 'section' => $sectionB]) + ->callAction('createField', data: [ + 'name' => 'New Field', + 'code' => 'shared_code', + 'type' => 'text', + ]) + ->assertHasActionErrors(['code']); + }); + + it('allows editing a field to reuse a code excluded by the registered resolver', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionA->id, + 'entity_type' => Post::class, + 'name' => 'Original', + 'code' => 'reused_code', + 'type' => 'text', + ]); + + $clone = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Cloned', + 'code' => 'reused_code', + 'type' => 'text', + ]); + + /* + * Mirrors a consumer that scopes code uniqueness to "everything outside this set of + * related sections" (e.g. a version lineage) rather than "just this one section" -- + * the shape CRITICAL-1 in the whole-branch review required. + */ + FieldForm::resolveUniqueCodeRuleModifierUsing( + fn (?CustomFieldSection $s): ?Closure => $s instanceof CustomFieldSection + ? fn ($rule) => $rule->whereNotIn('custom_field_section_id', [$sectionA->id, $sectionB->id]) + : null + ); + + livewire(ManageCustomField::class, ['field' => $clone]) + ->callAction('edit', data: [ + 'name' => 'Cloned', + 'code' => 'reused_code', + 'type' => 'text', + ]) + ->assertHasNoActionErrors(); + }); + + it('still rejects a code collision outside the resolver-defined scope', function (): void { + $sectionA = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'A', 'code' => 'a']); + $sectionB = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'B', 'code' => 'b']); + $sectionOutside = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Outside', 'code' => 'outside']); + + CustomField::factory()->create([ + 'custom_field_section_id' => $sectionOutside->id, + 'entity_type' => Post::class, + 'name' => 'Outside field', + 'code' => 'outside_code', + 'type' => 'text', + ]); + + $target = CustomField::factory()->create([ + 'custom_field_section_id' => $sectionB->id, + 'entity_type' => Post::class, + 'name' => 'Target', + 'code' => 'target_code', + 'type' => 'text', + ]); + + FieldForm::resolveUniqueCodeRuleModifierUsing( + fn (?CustomFieldSection $s): ?Closure => $s instanceof CustomFieldSection + ? fn ($rule) => $rule->whereNotIn('custom_field_section_id', [$sectionA->id, $sectionB->id]) + : null + ); + + livewire(ManageCustomField::class, ['field' => $target]) + ->callAction('edit', data: [ + 'name' => 'Target', + 'code' => 'outside_code', + 'type' => 'text', + ]) + ->assertHasActionErrors(['code']); + }); +}); + describe('CodeGenerator uniqueness scope resolver', function (): void { it('suffixes a colliding code when no resolver is registered (backward compatible)', function (): void { $section = CustomFieldSection::factory()->create(['entity_type' => Post::class, 'name' => 'Scope Default', 'code' => 'scope_default']); From 472cd55693a997d7fb0dc99330f160e2523a8af4 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 17:14:26 +0400 Subject: [PATCH 12/16] fix: drop unreachable is_numeric arm in formatJsValue 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. --- src/Services/Visibility/FrontendVisibilityService.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Services/Visibility/FrontendVisibilityService.php b/src/Services/Visibility/FrontendVisibilityService.php index 91eb9ca5..b532d7e8 100644 --- a/src/Services/Visibility/FrontendVisibilityService.php +++ b/src/Services/Visibility/FrontendVisibilityService.php @@ -605,9 +605,6 @@ private function formatJsValue(mixed $value): string is_string($value) => $this->toJsString($value), is_int($value) => (string) $value, is_float($value) => number_format($value, 10, '.', ''), - is_numeric($value) => str_contains($value, '.') - ? number_format((float) $value, 10, '.', '') - : (string) ((int) $value), is_array($value) => collect($value) ->map(fn (mixed $item): string => $this->formatJsValue($item)) ->pipe( From 86d03e449cdb2d6bbbe40842022295a7e8099a06 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 17:14:33 +0400 Subject: [PATCH 13/16] style: apply rector fixes to section-scope code 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. --- src/Support/CodeGenerator.php | 2 +- tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Support/CodeGenerator.php b/src/Support/CodeGenerator.php index 4c46fcee..88857a16 100644 --- a/src/Support/CodeGenerator.php +++ b/src/Support/CodeGenerator.php @@ -117,7 +117,7 @@ private static function codeExists( $query->where($model->getKeyName(), '!=', $ignoreId); } - if (self::$uniquenessScopeResolver !== null) { + if (self::$uniquenessScopeResolver instanceof Closure) { $scope = (self::$uniquenessScopeResolver)($entityType, $type, $sectionId); if ($scope !== null) { diff --git a/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php index 536dd5e1..304b7968 100644 --- a/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php +++ b/tests/Feature/RelaxCustomFieldsUniqueKeyMigrationTest.php @@ -165,6 +165,7 @@ function customFieldsIndexNames(string $table): Collection $defaultUniqueIndexName = new ReflectionMethod($this->migration, 'defaultUniqueIndexName'); $defaultUniqueIndexName->setAccessible(true); + $actual = $defaultUniqueIndexName->invoke($this->migration, 'custom_fields', $columns); expect($actual)->toBe('custom_fields_code_entity_type_unique'); From 91a3be4f9754599de2362d87fd8130a9b5782793 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 17:24:36 +0400 Subject: [PATCH 14/16] test: pin numeric-string condition values to string comparison in visibleJs 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. --- .../FieldVisibilityIntegrationTest.php | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/Feature/FieldVisibilityIntegrationTest.php b/tests/Feature/FieldVisibilityIntegrationTest.php index 48e78cd2..4d872799 100644 --- a/tests/Feature/FieldVisibilityIntegrationTest.php +++ b/tests/Feature/FieldVisibilityIntegrationTest.php @@ -228,3 +228,64 @@ ->and($visibleJs)->toBe($expectedJs); }); }); + +describe('Numeric-string condition values in the generated visibility JS', function (): void { + $buildEqualsJs = function (string|int|float $conditionValue): string { + $section = CustomFieldSection::factory()->create([ + 'name' => 'Numeric Condition Section', + 'entity_type' => Post::class, + 'active' => true, + ]); + + $triggerField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Quantity', + 'code' => 'quantity', + 'type' => 'text', + 'entity_type' => Post::class, + ]); + + $conditionalField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Gated', + 'code' => 'gated', + 'type' => 'text', + 'entity_type' => Post::class, + 'settings' => [ + 'visibility' => [ + 'mode' => VisibilityMode::SHOW_WHEN, + 'logic' => VisibilityLogic::ALL, + 'conditions' => [[ + 'field_code' => 'quantity', + 'operator' => VisibilityOperator::EQUALS, + 'value' => $conditionValue, + 'source' => ConditionSource::CustomField, + ]], + ], + ], + ]); + + return (string) app(FrontendVisibilityService::class) + ->buildVisibilityExpression($conditionalField, collect([$triggerField, $conditionalField])); + }; + + it('emits a numeric string as a JS string literal, and still coerces when the field holds a number', function () use ($buildEqualsJs): void { + expect($buildEqualsJs('42')) + ->toContain("const compareVal = '42';") + ->toContain("typeof fieldVal === 'number' && typeof compareVal === 'string'") + ->toContain("typeof fieldVal === 'string' && typeof compareVal === 'number'"); + }); + + it('keeps a numeric string comparable as a string, matching the backend operator', function () use ($buildEqualsJs): void { + // VisibilityOperator::evaluateEquals() compares two strings as strings, so '42.5' and + // '42.50' are NOT equal on the server. Emitting the condition as a JS number would make + // parseFloat('42.5') === 42.5 true on the client and silently desync the two engines. + expect($buildEqualsJs('42.50'))->toContain("const compareVal = '42.50';") + ->and(VisibilityOperator::EQUALS->evaluate('42.5', '42.50'))->toBeFalse(); + }); + + it('emits genuine int and float condition values as JS numbers', function () use ($buildEqualsJs): void { + expect($buildEqualsJs(42))->toContain('const compareVal = 42;') + ->and($buildEqualsJs(42.5))->toContain('const compareVal = 42.5000000000;'); + }); +}); From fb7080ff0f3294804d97d62c2a741e95b7414791 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 17:53:45 +0400 Subject: [PATCH 15/16] fix: compare strings case-insensitively in generated visibility JS 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. --- .../Visibility/FrontendVisibilityService.php | 8 ++ .../FieldVisibilityIntegrationTest.php | 96 ++++++++++++------- 2 files changed, 67 insertions(+), 37 deletions(-) diff --git a/src/Services/Visibility/FrontendVisibilityService.php b/src/Services/Visibility/FrontendVisibilityService.php index b532d7e8..2f4b270e 100644 --- a/src/Services/Visibility/FrontendVisibilityService.php +++ b/src/Services/Visibility/FrontendVisibilityService.php @@ -347,6 +347,10 @@ private function buildNotEqualsExpression( /** * Build standard equals expression for non-optionable fields. + * + * Two strings are compared case-insensitively to mirror VisibilityOperator::evaluateEquals(), + * which folds both sides through strtolower(). Without this the server shows a field for + * "active" vs "Active" while the client hides it. */ private function buildStandardEqualsExpression( string $fieldValue, @@ -367,6 +371,10 @@ private function buildStandardEqualsExpression( const fieldVal = {$fieldValue}; const compareVal = {$jsValue}; + if (typeof fieldVal === 'string' && typeof compareVal === 'string') { + return fieldVal.toLowerCase() === compareVal.toLowerCase(); + } + if (typeof fieldVal === typeof compareVal) { return fieldVal === compareVal; } diff --git a/tests/Feature/FieldVisibilityIntegrationTest.php b/tests/Feature/FieldVisibilityIntegrationTest.php index 4d872799..9b78bb94 100644 --- a/tests/Feature/FieldVisibilityIntegrationTest.php +++ b/tests/Feature/FieldVisibilityIntegrationTest.php @@ -229,46 +229,46 @@ }); }); -describe('Numeric-string condition values in the generated visibility JS', function (): void { - $buildEqualsJs = function (string|int|float $conditionValue): string { - $section = CustomFieldSection::factory()->create([ - 'name' => 'Numeric Condition Section', - 'entity_type' => Post::class, - 'active' => true, - ]); - - $triggerField = CustomField::factory()->create([ - 'custom_field_section_id' => $section->id, - 'name' => 'Quantity', - 'code' => 'quantity', - 'type' => 'text', - 'entity_type' => Post::class, - ]); - - $conditionalField = CustomField::factory()->create([ - 'custom_field_section_id' => $section->id, - 'name' => 'Gated', - 'code' => 'gated', - 'type' => 'text', - 'entity_type' => Post::class, - 'settings' => [ - 'visibility' => [ - 'mode' => VisibilityMode::SHOW_WHEN, - 'logic' => VisibilityLogic::ALL, - 'conditions' => [[ - 'field_code' => 'quantity', - 'operator' => VisibilityOperator::EQUALS, - 'value' => $conditionValue, - 'source' => ConditionSource::CustomField, - ]], - ], +$buildEqualsJs = function (string|int|float $conditionValue, VisibilityOperator $operator = VisibilityOperator::EQUALS): string { + $section = CustomFieldSection::factory()->create([ + 'name' => 'Text Condition Section', + 'entity_type' => Post::class, + 'active' => true, + ]); + + $triggerField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Status', + 'code' => 'status', + 'type' => 'text', + 'entity_type' => Post::class, + ]); + + $conditionalField = CustomField::factory()->create([ + 'custom_field_section_id' => $section->id, + 'name' => 'Gated', + 'code' => 'gated', + 'type' => 'text', + 'entity_type' => Post::class, + 'settings' => [ + 'visibility' => [ + 'mode' => VisibilityMode::SHOW_WHEN, + 'logic' => VisibilityLogic::ALL, + 'conditions' => [[ + 'field_code' => 'status', + 'operator' => $operator, + 'value' => $conditionValue, + 'source' => ConditionSource::CustomField, + ]], ], - ]); + ], + ]); - return (string) app(FrontendVisibilityService::class) - ->buildVisibilityExpression($conditionalField, collect([$triggerField, $conditionalField])); - }; + return (string) app(FrontendVisibilityService::class) + ->buildVisibilityExpression($conditionalField, collect([$triggerField, $conditionalField])); +}; +describe('Numeric-string condition values in the generated visibility JS', function () use ($buildEqualsJs): void { it('emits a numeric string as a JS string literal, and still coerces when the field holds a number', function () use ($buildEqualsJs): void { expect($buildEqualsJs('42')) ->toContain("const compareVal = '42';") @@ -289,3 +289,25 @@ ->and($buildEqualsJs(42.5))->toContain('const compareVal = 42.5000000000;'); }); }); + +describe('Case-insensitive string equality parity between the two visibility engines', function () use ($buildEqualsJs): void { + it('folds both sides before comparing, matching the backend operator', function () use ($buildEqualsJs): void { + // Server: strtolower('active') === strtolower('Active') is true. A case-sensitive client + // comparison hides the field the server would show. + expect(VisibilityOperator::EQUALS->evaluate('active', 'Active'))->toBeTrue() + ->and($buildEqualsJs('Active')) + ->toContain("typeof fieldVal === 'string' && typeof compareVal === 'string'") + ->toContain('fieldVal.toLowerCase() === compareVal.toLowerCase()'); + }); + + it('applies the same folding to not_equals, which negates the equals expression', function () use ($buildEqualsJs): void { + expect(VisibilityOperator::NOT_EQUALS->evaluate('active', 'Active'))->toBeFalse() + ->and($buildEqualsJs('Active', VisibilityOperator::NOT_EQUALS)) + ->toContain('fieldVal.toLowerCase() === compareVal.toLowerCase()'); + }); + + it('still distinguishes genuinely different strings', function () use ($buildEqualsJs): void { + expect(VisibilityOperator::EQUALS->evaluate('inactive', 'Active'))->toBeFalse() + ->and($buildEqualsJs('Active'))->toContain("const compareVal = 'Active';"); + }); +}); From 5fa87dab2acec5461aece81a3c9ccf862d4767d8 Mon Sep 17 00:00:00 2001 From: Manuk Date: Sat, 8 Aug 2026 18:07:49 +0400 Subject: [PATCH 16/16] fix: align server and client evaluation of the equals operator 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. --- src/Enums/VisibilityOperator.php | 63 +++++++++++++++++-- .../Visibility/FrontendVisibilityService.php | 29 +++++---- .../FieldVisibilityIntegrationTest.php | 42 ++++++++++++- 3 files changed, 116 insertions(+), 18 deletions(-) diff --git a/src/Enums/VisibilityOperator.php b/src/Enums/VisibilityOperator.php index 1d4e57b8..3269e6a9 100644 --- a/src/Enums/VisibilityOperator.php +++ b/src/Enums/VisibilityOperator.php @@ -62,9 +62,13 @@ public function evaluate(mixed $fieldValue, mixed $expectedValue): bool }; } + /** + * The arm order here mirrors the JavaScript emitted by FrontendVisibilityService, so a + * condition resolves the same way on the server and in the live form. Reordering an arm + * desyncs the two engines. + */ private function evaluateEquals(mixed $fieldValue, mixed $expectedValue): bool { - // Handle null values if ($fieldValue === null && $expectedValue === null) { return true; } @@ -73,20 +77,69 @@ private function evaluateEquals(mixed $fieldValue, mixed $expectedValue): bool return false; } - // Handle arrays + // Two collections compare as sets, matching the client's sorted-JSON equality. + if (is_array($fieldValue) && is_array($expectedValue)) { + return $this->normalizeSet($fieldValue) === $this->normalizeSet($expectedValue); + } + + // A multi-value field against a single condition value means membership. Compared on the + // string form so a field holding [42] still matches the condition a text input stored + // as '42' - the same coercion the client applies. if (is_array($fieldValue)) { - return in_array($expectedValue, $fieldValue, true); + return in_array($this->stringifyScalar($expectedValue), $this->normalizeSet($fieldValue), true); + } + + if (is_array($expectedValue)) { + return false; } - // Handle strings (case-insensitive) + // formatJsValue() emits booleans and the literals 'true'/'false' alike as JS booleans, + // so the client cannot tell a real bool from its spelling. Compare on the spelling. + if (is_bool($fieldValue) || is_bool($expectedValue)) { + return strtolower($this->stringifyScalar($fieldValue)) === strtolower($this->stringifyScalar($expectedValue)); + } + + // Two strings compare as strings, so '42.5' and '42.50' stay distinct. if (is_string($fieldValue) && is_string($expectedValue)) { return strtolower($fieldValue) === strtolower($expectedValue); } - // Handle numeric values + // Mixed number/numeric-string compares numerically, matching the client's parseFloat + // branches. A numeric custom field reads back as an int while its condition is stored + // as the string a text input produced, so strict identity never matched. + if (is_numeric($fieldValue) && is_numeric($expectedValue)) { + return (float) $fieldValue === (float) $expectedValue; + } + return $fieldValue === $expectedValue; } + /** + * @param array $values + * @return array + */ + private function normalizeSet(array $values): array + { + $normalized = array_map(fn (mixed $value): string => $this->stringifyScalar($value), array_values($values)); + + sort($normalized); + + return $normalized; + } + + private function stringifyScalar(mixed $value): string + { + if (is_bool($value)) { + return $value ? 'true' : 'false'; + } + + if (is_scalar($value)) { + return (string) $value; + } + + return serialize($value); + } + private function evaluateContains(mixed $fieldValue, mixed $expectedValue): bool { if ($fieldValue === null || $expectedValue === null) { diff --git a/src/Services/Visibility/FrontendVisibilityService.php b/src/Services/Visibility/FrontendVisibilityService.php index 2f4b270e..e868d0b6 100644 --- a/src/Services/Visibility/FrontendVisibilityService.php +++ b/src/Services/Visibility/FrontendVisibilityService.php @@ -363,32 +363,39 @@ private function buildStandardEqualsExpression( const fieldVal = {$fieldValue}; const compareVal = {$jsValue}; if (!Array.isArray(fieldVal) || !Array.isArray(compareVal)) return false; - return JSON.stringify(fieldVal.sort()) === JSON.stringify(compareVal.sort()); + const norm = a => JSON.stringify(a.map(v => String(v)).sort()); + return norm(fieldVal) === norm(compareVal); })()"; } return "(() => { const fieldVal = {$fieldValue}; const compareVal = {$jsValue}; + const isBlank = v => v === null || v === undefined; + const isNumericLike = v => typeof v !== 'boolean' && String(v).trim() !== '' && !isNaN(Number(v)); - if (typeof fieldVal === 'string' && typeof compareVal === 'string') { - return fieldVal.toLowerCase() === compareVal.toLowerCase(); + if (isBlank(fieldVal) && isBlank(compareVal)) { + return true; } - if (typeof fieldVal === typeof compareVal) { - return fieldVal === compareVal; + if (isBlank(fieldVal) || isBlank(compareVal)) { + return false; } - if ((fieldVal === null || fieldVal === undefined) && (compareVal === null || compareVal === undefined)) { - return true; + if (Array.isArray(fieldVal)) { + return fieldVal.map(v => String(v)).includes(String(compareVal)); } - if (typeof fieldVal === 'number' && typeof compareVal === 'string' && !isNaN(parseFloat(compareVal))) { - return fieldVal === parseFloat(compareVal); + if (typeof fieldVal === 'boolean' || typeof compareVal === 'boolean') { + return String(fieldVal).toLowerCase() === String(compareVal).toLowerCase(); + } + + if (typeof fieldVal === 'string' && typeof compareVal === 'string') { + return fieldVal.toLowerCase() === compareVal.toLowerCase(); } - if (typeof fieldVal === 'string' && typeof compareVal === 'number' && !isNaN(parseFloat(fieldVal))) { - return parseFloat(fieldVal) === compareVal; + if (isNumericLike(fieldVal) && isNumericLike(compareVal)) { + return Number(fieldVal) === Number(compareVal); } return String(fieldVal) === String(compareVal); diff --git a/tests/Feature/FieldVisibilityIntegrationTest.php b/tests/Feature/FieldVisibilityIntegrationTest.php index 9b78bb94..ad7b0b4d 100644 --- a/tests/Feature/FieldVisibilityIntegrationTest.php +++ b/tests/Feature/FieldVisibilityIntegrationTest.php @@ -272,8 +272,8 @@ it('emits a numeric string as a JS string literal, and still coerces when the field holds a number', function () use ($buildEqualsJs): void { expect($buildEqualsJs('42')) ->toContain("const compareVal = '42';") - ->toContain("typeof fieldVal === 'number' && typeof compareVal === 'string'") - ->toContain("typeof fieldVal === 'string' && typeof compareVal === 'number'"); + ->toContain('isNumericLike(fieldVal) && isNumericLike(compareVal)') + ->and(VisibilityOperator::EQUALS->evaluate(42, '42'))->toBeTrue(); }); it('keeps a numeric string comparable as a string, matching the backend operator', function () use ($buildEqualsJs): void { @@ -311,3 +311,41 @@ ->and($buildEqualsJs('Active'))->toContain("const compareVal = 'Active';"); }); }); + +describe('VisibilityOperator equality mirrors the client expression', function (): void { + it('matches a numeric field value against the string a text input stored', function (): void { + // A NUMERIC field reads back from integer_value as an int, while its condition is + // whatever the text input persisted. Strict identity meant such a condition never matched. + expect(VisibilityOperator::EQUALS->evaluate(42, '42'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate('42', 42))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(42.5, '42.50'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(42, '43'))->toBeFalse(); + }); + + it('keeps two strings on string comparison so trailing zeros still differ', function (): void { + expect(VisibilityOperator::EQUALS->evaluate('42.5', '42.50'))->toBeFalse(); + }); + + it('compares a boolean against its spelling, which is all the client can see', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(true, 'true'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate('TRUE', true))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(false, 'false'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(true, 'false'))->toBeFalse() + ->and(VisibilityOperator::EQUALS->evaluate(true, '42'))->toBeFalse(); + }); + + it('compares two collections as sets rather than looking for one inside the other', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(['a', 'b'], ['b', 'a']))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(['a', 'b'], ['a']))->toBeFalse(); + }); + + it('treats a single condition value against a multi-value field as membership', function (): void { + expect(VisibilityOperator::EQUALS->evaluate(['a', 'b'], 'a'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate([42], '42'))->toBeTrue() + ->and(VisibilityOperator::EQUALS->evaluate(['a', 'b'], 'c'))->toBeFalse(); + }); + + it('never matches a scalar field against a collection condition', function (): void { + expect(VisibilityOperator::EQUALS->evaluate('a', ['a', 'b']))->toBeFalse(); + }); +});