From fb3468e82d67822d4db66ad935989f347a73c44b Mon Sep 17 00:00:00 2001 From: Daniele Barbaro Date: Tue, 8 Sep 2026 11:36:42 +0200 Subject: [PATCH 1/2] ci(databases): run the suite on mysql and postgres The README named MySQL and PostgreSQL as supported while the workflow installed sqlite and pdo_sqlite and nothing else, so no query had ever run against either. The two operators added for this release are the ones most likely to differ: contains_any and contains_all go through whereJsonContains, which the three drivers compile into json_each, json_contains and a jsonb containment, and the contains filter carries an explicit escape clause. A databases job runs the suite against MySQL 8.4 and PostgreSQL 17 as service containers. TestCase reads DB_DRIVER, DB_HOST, DB_PORT, DB_DATABASE, DB_USERNAME and DB_PASSWORD and falls back to the in memory SQLite it used before, so nothing changes for a local run. Both operator families and the escape clause turned out to work on all three. Two real defects did not. Definitions came back in whatever order the database chose, so getCustomFields() and the generated filter and sort lists were ordered by insertion on SQLite and arbitrarily on a server, and the sort_order column that exists for this was never read. All three definition queries now order by sort_order and then by slug. A decimal value read back differently per driver. MySQL and PostgreSQL return the full scale of the decimal(20,6) column, so a stored 12.5 came back as 12.500000 while SQLite gave 12.5. DecimalType trims the fraction so the value the consumer wrote is the value it reads. The host tables the fixtures create are dropped on both ends of a test rather than only created, since a server keeps what the previous test left. The column type assertions accept the name each driver reports for a kind rather than the SQLite one. Verified locally against mysql:8.4 and postgres:17 in Docker: 115 tests pass on all three drivers. --- .github/workflows/tests.yml | 75 +++++++++++++++++++++++++ CHANGELOG.md | 15 +++++ README.md | 11 ++++ src/Concerns/HasCustomFields.php | 2 +- src/CustomFields.php | 2 + src/Types/DecimalType.php | 13 ++++- src/Validation/ValueValidator.php | 2 + tests/Feature/CustomFieldValueTest.php | 2 +- tests/Feature/KeyTypeTest.php | 30 ++++++++-- tests/TestCase.php | 77 ++++++++++++++++++++++---- tests/Unit/FieldTypeTest.php | 4 +- 11 files changed, 212 insertions(+), 21 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e5fb17a..7627913 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -73,3 +73,78 @@ jobs: - name: Test Suite (Windows) if: runner.os == 'Windows' run: vendor/bin/pest + + databases: + runs-on: ubuntu-latest + + name: ${{ matrix.database }} + + strategy: + fail-fast: false + matrix: + include: + - database: MySQL 8.4 + driver: mysql + port: 3306 + username: root + password: '' + - database: PostgreSQL 17 + driver: pgsql + port: 5432 + username: postgres + password: secret + + services: + mysql: + image: mysql:8.4 + env: + MYSQL_ALLOW_EMPTY_PASSWORD: yes + MYSQL_DATABASE: custom_fields + ports: + - 3306:3306 + options: >- + --health-cmd="mysqladmin ping --silent" + --health-interval=10s + --health-timeout=5s + --health-retries=10 + + postgres: + image: postgres:17 + env: + POSTGRES_PASSWORD: secret + POSTGRES_DB: custom_fields + ports: + - 5432:5432 + options: >- + --health-cmd=pg_isready + --health-interval=10s + --health-timeout=5s + --health-retries=10 + + steps: + - name: Checkout code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Setup PHP + uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2 + with: + php-version: 8.4 + extensions: dom, curl, libxml, mbstring, zip, fileinfo, sqlite, pdo_sqlite, mysql, pdo_mysql, pgsql, pdo_pgsql, bcmath, intl + ini-values: error_reporting=E_ALL + tools: composer:v2 + coverage: none + + - name: Install dependencies + run: | + composer update --prefer-stable --prefer-dist --no-interaction --no-progress --no-scripts + composer run prepare + + - name: Test Suite + env: + DB_DRIVER: ${{ matrix.driver }} + DB_HOST: 127.0.0.1 + DB_PORT: ${{ matrix.port }} + DB_DATABASE: custom_fields + DB_USERNAME: ${{ matrix.username }} + DB_PASSWORD: ${{ matrix.password }} + run: composer test:unit diff --git a/CHANGELOG.md b/CHANGELOG.md index d1147a7..2e138ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Added + +- The test suite runs against MySQL 8.4 and PostgreSQL 17 on every build, alongside the + SQLite matrix. `DB_DRIVER` points it at a server locally. + +### Fixed + +- `getCustomFields()` and the generated filters and sorts come back ordered by + `sort_order` and then by slug. The order was whatever the database returned, so the same + code gave a different order on SQLite and on a server, and the `sort_order` column was + never read. +- A `decimal` value reads back the same on every driver. MySQL and PostgreSQL return the + full scale of the column, so a stored `12.5` came back as `12.500000` while SQLite gave + `12.5`. + ## 0.1.0 - 2026-09-07 First public release. diff --git a/README.md b/README.md index 5460480..339709e 100644 --- a/README.md +++ b/README.md @@ -776,6 +776,17 @@ on purpose, so a refactoring it proposes is read before it is taken: vendor/bin/rector process ``` +The suite runs against an in memory SQLite database by default. Point it at a server with +`DB_DRIVER`, which is how the workflow exercises MySQL and PostgreSQL on every build: + +```bash +DB_DRIVER=mysql DB_PORT=3306 DB_USERNAME=root vendor/bin/pest +DB_DRIVER=pgsql DB_PORT=5432 DB_USERNAME=postgres DB_PASSWORD=secret vendor/bin/pest +``` + +`DB_HOST` and `DB_DATABASE` are read the same way and default to `127.0.0.1` and +`custom_fields`. + ## Contributing Thank you for considering contributing to Laravel Custom Fields! Please review our [contributing guide](.github/CONTRIBUTING.md) to get started. diff --git a/src/Concerns/HasCustomFields.php b/src/Concerns/HasCustomFields.php index c486c6c..6fa6f49 100644 --- a/src/Concerns/HasCustomFields.php +++ b/src/Concerns/HasCustomFields.php @@ -198,7 +198,7 @@ private function customFieldDefinitions(bool $includeInactive = false): array $query->where('is_active', true); } - return $query->get() + return $query->orderBy('sort_order')->orderBy('slug')->get() ->keyBy(static fn (CustomField $field): string => (string) $field->getAttribute('slug')) ->all(); } diff --git a/src/CustomFields.php b/src/CustomFields.php index c6a6f23..98e8d45 100644 --- a/src/CustomFields.php +++ b/src/CustomFields.php @@ -235,6 +235,8 @@ private function queryableFields(Model|string $model): Collection $fields = $fieldModel::query() ->where('entity_type', $this->entityKey($model)) ->where('is_active', true) + ->orderBy('sort_order') + ->orderBy('slug') ->get(); return $fields; diff --git a/src/Types/DecimalType.php b/src/Types/DecimalType.php index a85f7a6..1bbfaf1 100644 --- a/src/Types/DecimalType.php +++ b/src/Types/DecimalType.php @@ -23,9 +23,20 @@ public function serialize(mixed $value, Model $field): mixed return $value === null ? null : (string) $value; } + /** + * MySQL and PostgreSQL hand back the full scale of the decimal column while + * SQLite returns what was written, so the fraction is trimmed and the same + * stored value reads the same on every driver. + */ public function deserialize(mixed $value, Model $field): mixed { - return $value === null ? null : (string) $value; + if ($value === null) { + return null; + } + + $value = (string) $value; + + return str_contains($value, '.') ? rtrim(rtrim($value, '0'), '.') : $value; } public function rules(Model $field): array diff --git a/src/Validation/ValueValidator.php b/src/Validation/ValueValidator.php index 96c9187..6dcc89c 100644 --- a/src/Validation/ValueValidator.php +++ b/src/Validation/ValueValidator.php @@ -24,6 +24,8 @@ public function definitions(Model|string $model): array /** @var array $definitions */ $definitions = $fieldModel::query() ->where('entity_type', CustomFields::entityKey($model)) + ->orderBy('sort_order') + ->orderBy('slug') ->get() ->keyBy(static fn (Model $field): string => (string) $field->getAttribute('slug')) ->all(); diff --git a/tests/Feature/CustomFieldValueTest.php b/tests/Feature/CustomFieldValueTest.php index 0c74c99..3185112 100644 --- a/tests/Feature/CustomFieldValueTest.php +++ b/tests/Feature/CustomFieldValueTest.php @@ -44,8 +44,8 @@ function field(array $attributes): CustomField ], complete: true); expect($article->getCustomFields())->toBe([ - 'rank' => 10, 'email' => 'person@example.com', + 'rank' => 10, ]); }); diff --git a/tests/Feature/KeyTypeTest.php b/tests/Feature/KeyTypeTest.php index 142a4f4..66e36a8 100644 --- a/tests/Feature/KeyTypeTest.php +++ b/tests/Feature/KeyTypeTest.php @@ -19,14 +19,30 @@ function columnType(string $table, string $column): string return is_array($found) ? (string) $found['type_name'] : ''; } +/** + * Every driver names its column types differently, so a kind lists what SQLite, + * MySQL and PostgreSQL each report for it. Asserting the kind keeps the test + * about the key shape rather than about the vocabulary of one database. + * + * @return array + */ +function columnTypesOf(string $kind): array +{ + return match ($kind) { + 'integer' => ['integer', 'bigint', 'int8'], + 'uuid' => ['varchar', 'char', 'uuid'], + 'ulid' => ['varchar', 'char', 'bpchar'], + }; +} + it('keeps integer keys on both tables by default', function (): void { CustomFields::registerEntity(Article::class, 'article'); CustomField::create(['entity_type' => 'article', 'name' => 'Rank', 'type' => 'number']); $article = Article::create(['title' => 'A']); $article->setCustomField('rank', 5); - expect(columnType('custom_fields', 'id'))->toBe('integer') - ->and(columnType('custom_field_values', 'valuable_id'))->toBe('integer') + expect(columnType('custom_fields', 'id'))->toBeIn(columnTypesOf('integer')) + ->and(columnType('custom_field_values', 'valuable_id'))->toBeIn(columnTypesOf('integer')) ->and($article->customFieldValues()->first()?->getKey())->toBeInt() ->and((new CustomField)->getKeyType())->toBe('int') ->and((new CustomField)->getIncrementing())->toBeTrue(); @@ -37,6 +53,7 @@ function columnType(string $table, string $column): string config()->set('laravel-custom-fields.morph_key_type', 'uuid'); $this->rebuildPackageTables(); + Schema::dropIfExists('uuid_documents'); Schema::create('uuid_documents', function (Blueprint $table): void { $table->uuid('id')->primary(); $table->string('title'); @@ -49,8 +66,8 @@ function columnType(string $table, string $column): string $document->setCustomField('rank', 7); $row = $document->customFieldValues()->first(); - expect(columnType('custom_fields', 'id'))->toBe('varchar') - ->and(columnType('custom_field_values', 'valuable_id'))->toBe('varchar') + expect(columnType('custom_fields', 'id'))->toBeIn(columnTypesOf('uuid')) + ->and(columnType('custom_field_values', 'valuable_id'))->toBeIn(columnTypesOf('uuid')) ->and(Str::isUuid((string) $field->getKey()))->toBeTrue() ->and(Str::isUuid((string) $row?->getKey()))->toBeTrue() ->and($row?->getAttribute('valuable_id'))->toBe($document->getKey()) @@ -62,6 +79,7 @@ function columnType(string $table, string $column): string config()->set('laravel-custom-fields.morph_key_type', 'ulid'); $this->rebuildPackageTables(); + Schema::dropIfExists('ulid_documents'); Schema::create('ulid_documents', function (Blueprint $table): void { $table->ulid('id')->primary(); $table->string('title'); @@ -74,8 +92,8 @@ function columnType(string $table, string $column): string $document->setCustomField('rank', 7); $row = $document->customFieldValues()->first(); - expect(columnType('custom_fields', 'id'))->toBe('varchar') - ->and(columnType('custom_field_values', 'valuable_id'))->toBe('varchar') + expect(columnType('custom_fields', 'id'))->toBeIn(columnTypesOf('ulid')) + ->and(columnType('custom_field_values', 'valuable_id'))->toBeIn(columnTypesOf('ulid')) ->and(Str::isUlid((string) $field->getKey()))->toBeTrue() ->and(Str::isUlid((string) $row?->getKey()))->toBeTrue() ->and($row?->getAttribute('valuable_id'))->toBe($document->getKey()) diff --git a/tests/TestCase.php b/tests/TestCase.php index 5627d4b..6cc5bd4 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -24,26 +24,72 @@ protected function getPackageProviders($app): array } /** - * The suite always runs against an in memory database. - * - * Without this, a database/database.sqlite left behind by composer build - * makes Testbench prefer the file, and the package migrations it published - * into the skeleton then collide with the ones loaded below. + * The suite runs against an in memory database unless DB_DRIVER asks for + * one of the servers, which is how the workflow exercises MySQL and + * PostgreSQL. Pinning the connection also keeps a database/database.sqlite + * left behind by composer build from making Testbench prefer the file, + * whose published migrations would then collide with the ones loaded below. */ protected function defineEnvironment($app): void { $app['config']->set('database.default', 'testing'); - $app['config']->set('database.connections.testing', [ - 'driver' => 'sqlite', - 'database' => ':memory:', - 'prefix' => '', - 'foreign_key_constraints' => true, - ]); + $app['config']->set('database.connections.testing', $this->connectionConfiguration()); + } + + /** @return array */ + protected function connectionConfiguration(): array + { + return match ($this->fromEnvironment('DB_DRIVER', 'sqlite')) { + 'mysql' => [ + 'driver' => 'mysql', + 'host' => $this->fromEnvironment('DB_HOST', '127.0.0.1'), + 'port' => $this->fromEnvironment('DB_PORT', '3306'), + 'database' => $this->fromEnvironment('DB_DATABASE', 'custom_fields'), + 'username' => $this->fromEnvironment('DB_USERNAME', 'root'), + 'password' => $this->fromEnvironment('DB_PASSWORD', ''), + 'charset' => 'utf8mb4', + 'collation' => 'utf8mb4_unicode_ci', + 'prefix' => '', + ], + 'pgsql' => [ + 'driver' => 'pgsql', + 'host' => $this->fromEnvironment('DB_HOST', '127.0.0.1'), + 'port' => $this->fromEnvironment('DB_PORT', '5432'), + 'database' => $this->fromEnvironment('DB_DATABASE', 'custom_fields'), + 'username' => $this->fromEnvironment('DB_USERNAME', 'postgres'), + 'password' => $this->fromEnvironment('DB_PASSWORD', ''), + 'charset' => 'utf8', + 'prefix' => '', + 'search_path' => 'public', + ], + default => [ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => '', + 'foreign_key_constraints' => true, + ], + }; + } + + /** Reads a workflow variable without the env() helper the arch test forbids. */ + protected function fromEnvironment(string $key, string $default): string + { + $value = getenv($key); + + return $value === false || $value === '' ? $default : $value; } + /** + * The host tables belong to the fixtures rather than to the package. + * + * An in memory database starts empty for every test, while a MySQL or a + * PostgreSQL server keeps what the previous test left, so they are dropped + * on both ends of the test rather than only created. + */ protected function defineDatabaseMigrations(): void { $this->loadMigrationsFrom(__DIR__.'/../database/migrations'); + $this->dropHostTables(); Schema::create('authors', function (Blueprint $table): void { $table->id(); @@ -62,6 +108,15 @@ protected function defineDatabaseMigrations(): void $table->string('title'); $table->timestamps(); }); + + $this->beforeApplicationDestroyed(fn () => $this->dropHostTables()); + } + + protected function dropHostTables(): void + { + foreach (['articles', 'projects', 'authors'] as $table) { + Schema::dropIfExists($table); + } } /** diff --git a/tests/Unit/FieldTypeTest.php b/tests/Unit/FieldTypeTest.php index 6bdeb91..5978a93 100644 --- a/tests/Unit/FieldTypeTest.php +++ b/tests/Unit/FieldTypeTest.php @@ -108,9 +108,11 @@ function typedField(string $type, ?array $options = null): CustomField typedField('multiselect', [['key' => 'alpha', 'label' => 'Alpha', 'is_active' => true]]); $article = Article::create(['title' => 'A']); + // Definitions come back ordered by sort_order and then by slug, so the + // order is the same on every driver rather than the order rows went in. expect($article->getCustomFields())->toBe([ - 'number-field' => null, 'multiselect-field' => [], + 'number-field' => null, ]); }); From a418082555aeaaf134894e72e428a700a8314e5e Mon Sep 17 00:00:00 2001 From: Daniele Barbaro Date: Tue, 8 Sep 2026 12:32:08 +0200 Subject: [PATCH 2/2] fix(tests): run the database job serially and reset leftovers composer test:unit runs Pest in parallel, and the two processes shared the one database the service container provides, so they created and dropped the same tables at the same time: SQLSTATE[42S01]: Table 'custom_field_values' already exists The job now calls Pest directly. Verified by reproducing the failure with --parallel against mysql:8.4 and confirming the serial run passes. That crash also exposed something worse: the interrupted run left its tables behind, and every later run against the same server failed on a schema it had never created. An in memory database starts empty each time, a server does not. The test case now drops what a previous run left before the migrator starts, so a server is usable again without being recreated by hand. Verified by poisoning both a MySQL and a PostgreSQL database with a crashed parallel run and watching the next serial run come back green. --- .github/workflows/tests.yml | 5 ++++- README.md | 4 +++- tests/TestCase.php | 28 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7627913..92754cc 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -139,6 +139,9 @@ jobs: composer update --prefer-stable --prefer-dist --no-interaction --no-progress --no-scripts composer run prepare + # Serial on purpose. composer test:unit runs Pest in parallel, and the + # processes would create and drop the same tables in the one database the + # service container provides. - name: Test Suite env: DB_DRIVER: ${{ matrix.driver }} @@ -147,4 +150,4 @@ jobs: DB_DATABASE: custom_fields DB_USERNAME: ${{ matrix.username }} DB_PASSWORD: ${{ matrix.password }} - run: composer test:unit + run: vendor/bin/pest diff --git a/README.md b/README.md index 339709e..01b5fb9 100644 --- a/README.md +++ b/README.md @@ -785,7 +785,9 @@ DB_DRIVER=pgsql DB_PORT=5432 DB_USERNAME=postgres DB_PASSWORD=secret vendor/bin/ ``` `DB_HOST` and `DB_DATABASE` are read the same way and default to `127.0.0.1` and -`custom_fields`. +`custom_fields`. Run Pest directly rather than through `composer test:unit` when you point +it at a server: that script runs in parallel, and the processes would create and drop the +same tables in the same database. ## Contributing diff --git a/tests/TestCase.php b/tests/TestCase.php index 6cc5bd4..dd63961 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -88,6 +88,7 @@ protected function fromEnvironment(string $key, string $default): string */ protected function defineDatabaseMigrations(): void { + $this->dropLeftoverTables(); $this->loadMigrationsFrom(__DIR__.'/../database/migrations'); $this->dropHostTables(); @@ -119,6 +120,33 @@ protected function dropHostTables(): void } } + /** + * Clears whatever a previous run left on a server. + * + * An in memory database has nothing to clear. A MySQL or a PostgreSQL + * server keeps the tables of a run that was interrupted, and the migrator + * would then try to create them again, so every later test fails on a + * schema it never made. + */ + protected function dropLeftoverTables(): void + { + if ($this->fromEnvironment('DB_DRIVER', 'sqlite') === 'sqlite') { + return; + } + + foreach ([ + 'custom_field_values', + 'custom_fields', + 'uuid_documents', + 'ulid_documents', + 'migrations', + ] as $table) { + Schema::dropIfExists($table); + } + + $this->dropHostTables(); + } + /** * Recreates the package tables under the key configuration the test just set. *