Skip to content
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

Require precision and scale for decimal columns #5631

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Aug 29, 2022

Similar to the string columns (#3263), having the DBAL define the precision and scale of decimal columns doesn't make much sense. The values of precision and scale should reflect the properties of the model represented by the schema.

TODO:

@morozov morozov added this to the 4.0.0 milestone Aug 29, 2022
@morozov morozov force-pushed the require-decimal-precision-scale branch 2 times, most recently from a8fc85e to 372df4a Compare August 29, 2022 20:01
@morozov morozov force-pushed the require-decimal-precision-scale branch from 372df4a to 08630c6 Compare September 1, 2022 15:25
@morozov morozov marked this pull request as ready for review September 1, 2022 16:07
} else {
$precision = $column['precision'];
if (! isset($column['precision'])) {
throw ColumnPrecisionRequired::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this typically going to be caught and improved with additional context elsewhere, or should we add additional context based on information contained in $column?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got mislead by the variable name (which should probably be $columnOptions, I don't think there is actually going to be anything helpful in there. The concern still stands though: are these exceptions going to be handled at a level where the column's fully qualified name is available?

Copy link
Member Author

@morozov morozov Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the problem. We have the same problem in other places, e.g.

protected function getVarcharTypeDeclarationSQLSnippet(?int $length): string
{
if ($length === null) {
throw ColumnLengthRequired::new($this, 'VARCHAR');
}

And it's even more acute there since the column name is not available in the scope from where the exception is thrown.

I cannot think of a simple solution at this point. I don't want to introduce a dependency on $column['name'] in the code being added just to use it as a parameter for the exception because the exception is unrelated to the name.

A proper solution might be to improve the exception hierarchy:

  1. All these three exceptions should have a common type (e.g. InvalidColumnType).
  2. The code that calls the functions that may throw such exceptions should have the column name in its scope and should catch these exceptions and throw a more user-friendly exception (e.g. InvalidColumnDefinition).

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since as you pointed out, this also happens elsewhere. Let's create an issue out of #5631 (comment) ?

@morozov
Copy link
Member Author

morozov commented Sep 1, 2022

The new issue is #5645.

@morozov morozov merged commit 2d57c31 into doctrine:4.0.x Sep 1, 2022
@morozov morozov deleted the require-decimal-precision-scale branch September 1, 2022 18:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants