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

v2.72 breaks doctrine date/time precision with mariadb 10.11 #2897

Closed
n3o77 opened this issue Dec 8, 2023 · 11 comments · Fixed by CarbonPHP/carbon-doctrine-types#7
Closed

Comments

@n3o77
Copy link

n3o77 commented Dec 8, 2023

Hello,

since updating to 2.72 doctrine migrations are broken when using the doctrine types. From first investigations it uses the default doctrine column precision of 10 which is not supported for date/time columns (max. 6).

doctrine:
    dbal:
        types:
            datetime_immutable: \Carbon\Doctrine\DateTimeImmutableType
            datetime: \Carbon\Doctrine\DateTimeType

Error:

Query 1 ERROR: Too big precision specified for 'created_at'. Maximum is 6

Carbon version: 2.72
PHP version: 8.3.0
MariaDB: 10.11.2-MariaDB-1:10.11.2+maria~ubu2204

Doctrine versions:

doctrine/annotations                2.0.1
doctrine/cache                      2.2.0
doctrine/collections                2.1.4
doctrine/common                     3.4.3
doctrine/data-fixtures              1.7.0
doctrine/dbal                       3.7.2
doctrine/deprecations               1.1.2
doctrine/doctrine-bundle            2.11.1
doctrine/doctrine-fixtures-bundle   3.5.1
doctrine/doctrine-migrations-bundle 3.3.0
doctrine/event-manager              2.0.0
doctrine/inflector                  2.0.8
doctrine/instantiator               2.0.0
doctrine/lexer                      2.1.0
doctrine/migrations                 3.7.2
doctrine/orm                        2.17.1
doctrine/persistence                3.2.0
doctrine/sql-formatter              1.1.3

Downgrading to 2.71 solves the issue for me.

Unfortunately i'm short on time right now to debug further. In https://github.com/CarbonPHP/carbon-doctrine-types/blob/main/src/Carbon/Doctrine/CarbonTypeConverter.php#L38 the $fieldDeclaration['precision'] is set to 10 which seems to be a doctrine default value as nowhere in my code the precision is defined.

Thanks you very much for your time!

@kylekatarnls
Copy link
Collaborator

Hello,

I recommend you set your default explicitly:

\Carbon\Doctrine\DateTimeDefaultPrecision::set(6);

Or set the precision for each field.

You can also choose to install composer require carbonphp/carbon-doctrine-types:^1.0 which will replace 10 (maximum) with 6 (default).

Default value can no longer be handled the same way with dbal v4. That's why it's now externalized in a separate package carbonphp/carbon-doctrine-types So you can upgrade separately nesbot/carbon and doctrine/dbal then pick the version of carbonphp/carbon-doctrine-types that better fits for you.

@n3o77
Copy link
Author

n3o77 commented Dec 8, 2023

Thanks for getting back so quick.

I recommend you set your default explicitly:

\Carbon\Doctrine\DateTimeDefaultPrecision::set(6);

Not sure if this will work as this is only called if the $fieldDeclaration['precision'] is null.

Setting it on each field in my current project would mean to abandon some shared functionality like TimestampableEntity Trait from https://github.com/doctrine-extensions/DoctrineExtensions

Downgrading carbon-doctrine-types might work but seems to limit doctrine/dbal to 3.6. For now it makes more sense to stay at 2.71, at least in the current project i'm working on.

@kylekatarnls
Copy link
Collaborator

Not sure if this will work as this is only called if the $fieldDeclaration['precision'] is null.

It should because (using latest versions: carbon 2.71, dbal 4, carbon-doctrine-types 3) and setting default precision to 6, I can't see anything that would still gives you 10. At least it would not come from Carbon, nor carbon-doctrine-types (nor from Doctrine default values AFAIK).

If DateTimeDefaultPrecision::set() is not enough we can think about a way to register callback function so you would be able to customize how it calculated precision.

In the meantime you can also extend the class:

doctrine:
    dbal:
        types:
            datetime_immutable: \YourOwnApp\Doctrine\DateTimeImmutableType
            datetime: \YourOwnApp\Doctrine\DateTimeType
namespace YourOwnApp\Doctrine;

class DateTimeImmutableType extends \Carbon\Doctrine\DateTimeImmutableType
{
    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform): string
    {
        $precision = $fieldDeclaration['precision'] ?? DateTimeDefaultPrecision::get();

        $precision = min($precision, 6); // Ensure it's never more than 6
        // Eventually 6 can be computed based on the maximum allowed by $platform

        $type = parent::getSQLDeclaration($fieldDeclaration, $platform);

        if (!$precision) {
            return $type;
        }

        if (str_contains($type, '(')) {
            return preg_replace('/\(\d+\)/', "($precision)", $type);
        }

        [$before, $after] = explode(' ', "$type ");

        return trim("$before($precision) $after");
    }
}

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Dec 8, 2023

BTW, that is an issue still open on DBAL side:

doctrine/dbal#2873 (comment)

Suggestion was to align on DBAL side the maximum precision with what the platform actually supports (instead of either 0 which by default truncate precision, or a constant value that as you can see will break in most platform).

So I'm considering implementing on carbon types, but would be a shame because it would better fit in DBAL as it's also relevant for DateTime (for people not using Carbon) and behavior would benefit to be consistent.

[EDIT]

The branch can be tested with:
composer require carbonphp/carbon-doctrine-types:dev-feature/maximum-per-platform

@n3o77
Copy link
Author

n3o77 commented Dec 8, 2023

So far in all debugging $fieldDeclaration['precision'] was always set to 10 and DateTimeDefaultPrecision::get() never called because of that (and it's not defined anywhere in our code explicitly) https://github.com/CarbonPHP/carbon-doctrine-types/blob/main/src/Carbon/Doctrine/CarbonTypeConverter.php#L38.

I guess because it's the default precision in doctrine dbal (maybe from here: https://github.com/doctrine/dbal/blob/3.7.x/src/Schema/Column.php#L25) but i have not confirmed this and i'm not that familiar with doctrine internals. I'll take some more time to debug further beginning next week.

Thanks again for the suggestion incl. code examples, very much appreciated.

@kylekatarnls
Copy link
Collaborator

never called because of that (and it's not defined anywhere in our code explicitly) https://github.com/CarbonPHP/carbon-doctrine-types/blob/main/src/Carbon/Doctrine/CarbonTypeConverter.php#L38.

It's not "because of that", if $fieldDeclaration['precision'] is always 10 then it means it has been set to 10 by something else (DBAL I guess) before entering getSQLDeclaration and we have no choice but to trust what's in there since it's where the precision can be customized. Also in recent version of dbal (3.7 IIRC) default changed which make it no longer possible to distinguish 10 explicitly asked by developer writing precision="10" and 10 obtained as a default value.

However we could cap the precision by the maximum that the platform support:

You can test using the pull-request:

composer require carbonphp/carbon-doctrine-types:dev-feature/maximum-per-platform

@kylekatarnls
Copy link
Collaborator

Released in carbonphp/carbon-doctrine-types 3.1.0, so simply update everything to latest will fix it.

@n3o77
Copy link
Author

n3o77 commented Dec 11, 2023

Could you release that fix for CarbonPHP/carbon-doctrine-types v2 ? I'm still on doctrine/dbal 3.7 and still have the problem.

@kylekatarnls
Copy link
Collaborator

Yup, I can. May you try:

composer require carbonphp/carbon-doctrine-types:dev-release/2.1.0-release

@n3o77
Copy link
Author

n3o77 commented Dec 11, 2023

Works perfect!

Thank you again, especially taking time over the weekend to work on this, you're awesome!

@kylekatarnls
Copy link
Collaborator

Now released as 2.1.0.

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 a pull request may close this issue.

2 participants