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

Allow precision 0 in Carbon DBAL types #2537

Merged
merged 3 commits into from
Jan 21, 2022
Merged

Allow precision 0 in Carbon DBAL types #2537

merged 3 commits into from
Jan 21, 2022

Conversation

kdaniel95
Copy link
Contributor

@kdaniel95 kdaniel95 commented Jan 13, 2022

Allow explicitly defined 0 precision for DBAL types without explicitly setting Datetime precision through DateTimeDefaultPrecision::set() because right now explicit 0 precision is treated same as null.

@kdaniel95 kdaniel95 marked this pull request as ready for review January 13, 2022 12:03
@kylekatarnls
Copy link
Collaborator

Sadly, @ORM\Column() is assigning 0 as property default value:
https://github.com/doctrine/orm/blob/2.11.x/lib/Doctrine/ORM/Mapping/Column.php#L32

which means this is a breaking change.

If an application is currently doing:

/**
 * @ORM\Column(type="datetime")
 */

without specifying any precision value, they are currently interpreted as precision=10 so it uses the maximum precision the DB engine can provide. If we merge this MR as is, those will suddenly become 0, it will lead to all second fractional data to be lost and this precision is often critical (that's why initially we provided only precision=6 with no option to change it, then in a next iteration, we allowed to customize it so it can be increased but without breaking loosing precision for existing apps).

I will check the other possibilities. But I would generally advocate for keeping maximum precision for datetimes. You won't regret having precision you don't use, on the opposite side, settle for second-precision at a given moment means the data you're saving now will never have second fraction, and if you need it later, it will be too late for the existing data. Also it's one less thing to bother if have all your datetime/timestamp with constant precision in your DB and don't have to determine which ones will need more or less.

@TamasSzigeti
Copy link

@kylekatarnls What about an extra argument to explicitly set 0 then? That shouldn't hurt BC

@kylekatarnls
Copy link
Collaborator

Indeed, some secondPrecision as a boolean could be a backward compatible solution.

@kdaniel95
Copy link
Contributor Author

So in this case, is it enough that I check if there's a key secondPrecision in $fieldDeclaration (and if its also true), then I set $precision to 0?

So something like this:

$precision = isset($fieldDeclaration['secondPrecision']) && $fieldDeclaration['secondPrecision']?0:($fieldDeclaration['precision'] ?: 10) === 10

@kdaniel95
Copy link
Contributor Author

@kylekatarnls Could you please confirm if above approach is correct or if I have to supply the flag from somewhere else? Thank you!

@kylekatarnls
Copy link
Collaborator

Getting the flag from here would be fine, then as an alternative to nested ternary, the code could be like:

$precision = $fieldDeclaration['precision'] ?: 10;

if ($fieldDeclaration['secondPrecision'] ?? false) {
    $precision = 0;
}

if ($precision === 10) {
    $precision = DateTimeDefaultPrecision::get();
}

@kdaniel95
Copy link
Contributor Author

@kylekatarnls Okay, got it, changed it and the tests as well. Could you check it out and if all is good (there's no breaking change or things of sort), run the other workflows (and hopefully at the end, merge the changes)?

Thank you!

Co-authored-by: Kyle <kylekatarnls@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants