Skip to content

Allow specific column type#1071

Open
Erwane wants to merge 1 commit intocakephp:5.nextfrom
Erwane:1068-add-specific-type
Open

Allow specific column type#1071
Erwane wants to merge 1 commit intocakephp:5.nextfrom
Erwane:1068-add-specific-type

Conversation

@Erwane
Copy link
Copy Markdown

@Erwane Erwane commented Apr 17, 2026

Allow addition of specific adapter column type.

@dereuromark
Copy link
Copy Markdown
Member

dereuromark commented Apr 17, 2026

Refs #1068

Small issue: SqliteAdapter silently ignores the registration

The PR puts $specificColumnTypes + addSpecificColumnType() on AbstractAdapter, implying universal support. But I checked all four adapters' getColumnTypes():

  • MysqlAdapter.php:1335 — array_merge(parent::getColumnTypes(), static::$specificColumnTypes) ✓
  • PostgresAdapter.php:1172 — same pattern ✓
  • SqlserverAdapter.php:1076 — same pattern ✓
  • SqliteAdapter.php:1664 — return array_keys(static::$supportedColumnTypes) — different property, no merge

So SqliteAdapter::addSpecificColumnType('foo') writes to AbstractAdapter::$specificColumnTypes (via LSB, since Sqlite doesn't redeclare it) and then isValidColumnType() never consults that list. Silent no-op on Sqlite. Either fix Sqlite to also merge, or keep the API Postgres/MySQL/SqlServer-only and document that.

@dereuromark dereuromark added this to the 5.next milestone Apr 17, 2026
@dereuromark
Copy link
Copy Markdown
Member

Should also target 5.next

@LordSimal LordSimal changed the base branch from 5.x to 5.next April 19, 2026 08:22
@dereuromark
Copy link
Copy Markdown
Member

Gap: no reset / removal API

static::$specificColumnTypes[] mutates a class-static array with no teardown. The bundled test registers 'my_custom_type' and
leaves it registered forever — fine for now because no other test uses that string, but:

  • Real apps that register 'hstore' and later run a test asserting hstore is rejected will see the test flake
  • Order-dependent tests are a long-term maintenance drag

Worth adding removeSpecificColumnType(string) and/or resetSpecificColumnTypes() — five lines plus a tearDown clearing them.

Gap: the test doesn't live where it ought to

testAddColumnWithSpecificColumnType is in tests/TestCase/Db/Table/TableTest.php, which makes sense as a general-machinery
test. But there's no per-adapter integration test proving the registered type actually round-trips through create() → DDL →
schema reflection. Given Cake's dialect fallback (strtoupper($type) for unknown types emits HSTORE correctly), a real Postgres
test adding 'hstore' and asserting the table was created with an hstore column would be ~10 extra lines and catch regressions
if the dialect ever changes that fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants