-
Notifications
You must be signed in to change notification settings - Fork 894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PostgresAdapter not returning limit for char and varchar #2214
Conversation
if (isset($columnInfo['numeric_precision'])) { | ||
$column->setPrecision($columnInfo['numeric_precision']); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe rather change the type check above, and explicitly check for the numeric types for which Phinx supports specifying precision? This "is not of type" check seems very prone to possible future regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I first tried to add STRING and CHAR to the !in_array
above. This did work, but, as a neophyte to Phinx, I was concerned I might be missing a type, which echoes your concerns.
https://www.postgresql.org/docs/current/infoschema-columns.html says numeric_precision contains "the (declared or implicit) precision" for numeric types and null for all others. I think the implicit precisions is how we got to you !in_array
.
Reviewing Phinx to see where getPrecision()
is used, I see some adapters use it if it contains a numeric value, no matter what type is being used. However, PostgresAdapter only uses it for:
- PHINX_TYPE_DECIMAL
- PHINX_TYPE_TIME
- PHINX_TYPE_TIMESTAMP (
getSqlType
looks to turn a PHINX_TYPE_DATETIME read by getColumns() into an associated array with 'name' => 'timestamp', so this works with theif (in_array($columnType, [static::PHINX_TYPE_TIME, static::PHINX_TYPE_DATETIME], true)) {
, but it did drive me in a marry little circle.
So, I think PHINX_TYPE_DECIMAL is the extent of it. Yeah?
The following code does pass locally:
if (in_array($columnType, [static::PHINX_TYPE_TIME, static::PHINX_TYPE_DATETIME], true)) {
$column->setPrecision($columnInfo['datetime_precision']);
} elseif ($columnType === self::PHINX_TYPE_DECIMAL) {
$column->setPrecision($columnInfo['numeric_precision']);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it very much looks like decimal is the only type for which the Postgres adapter supports numerical precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any further changes?
…PHINX_TYPE_DECIMAL Added testAddStringWithLimit to PostgresAdapterTest to assure that limits are being properly set. Updated ManagerTest->testMigrationWithCustomColumnTypes to no longer expect pgsql to return null for limit of custom columns.
Fixes #2213
Check to see if numeric_precision is set, before calling setPrecision in PostgresAdapter, to avoid unsetting the limit previously set while getting columns from the database.
Added testAddStringWithLimit to PostgresAdapterTest to ensure that limits are being properly set.
Updated ManagerTest->testMigrationWithCustomColumnTypes to no longer expect pgsql to return null for limit of custom columns.