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

Bug: Time loses microseconds #9079

Open
wkolaczek opened this issue Jul 26, 2024 · 8 comments · Fixed by #9081
Open

Bug: Time loses microseconds #9079

wkolaczek opened this issue Jul 26, 2024 · 8 comments · Fixed by #9081
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@wkolaczek
Copy link

wkolaczek commented Jul 26, 2024

PHP Version

8.2

CodeIgniter4 Version

4.5.3

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

MariaDB 10.4

What happened?

Second's fractional part of datetime is lost when casting filelds with 'datetime[ms|us]'.

Steps to Reproduce

  1. Create a table with DATETIME(6) field, and fill a record there e.g. "2024-07-09 09:13:34.123567".
  2. Create Model class with
protected array $casts  = ['my_dt_field' => 'datetime[us]' ];
  1. Load the record and see the value of 'my_dt_field'. It's "2024-07-09 09:13:34.000000'

Expected Output

When using 'datetime[ms|us]' caster, the fractional part shouldn't be lost.

Anything else?

The problem is with hardcoded formats in \CodeIgniter\I18n\TimeTrait used by DatetimeCast. They completly ignore the fractional part.
DatetimeCast::get() uses it, which gives wrong results here:

return Time::createFromFormat($format, $value);

The fractional part is kept, when raw DateTime clasess are used. This code doesn't lose anything:

$dt = DateTimeImmutable::createFromFormat("Y-m-d H:i:s.u", "2024-07-09 09:13:34.123567");

Perhaps I18n isn't valid in the entity/model/data layer, when dealing with datetime values. We should leave it rather to the UI layer of the app. Floats do it right: no I18n when casting = no problems with separators and fractional part lost, users can choose what they see.

@wkolaczek wkolaczek added the bug Verified issues on the current code behavior or pull requests that will fix them label Jul 26, 2024
@kenjis kenjis changed the title Bug: Bug: Time loses microseconds Jul 26, 2024
@kenjis
Copy link
Member

kenjis commented Jul 26, 2024

Thank you for reporting.
Please try #9081

@wkolaczek
Copy link
Author

Works like a charm. That was pretty fast. Thanks a lot!
:)

@ddevsr ddevsr reopened this Jul 26, 2024
@ddevsr
Copy link
Collaborator

ddevsr commented Jul 26, 2024

@wkolaczek This issue automatically close when PR merged

@kenjis
Copy link
Member

kenjis commented Jul 27, 2024

@wkolaczek Updated #9081 to fix other cases. Supporting microseconds in Time is more difficult than what I expected.
If you still find any bug, feel free to report.

@wkolaczek
Copy link
Author

wkolaczek commented Jul 28, 2024

Hi, thanks for looking at it. There is still a bug in the opposite direction - when storing records in db which should have datetime[us] the TimeTrais::_toString() just formats it as "Y-m-d H:i:s", so fractional part is lost when generating SQL. I've fixed it in my code temporarily in DataCaster\Cast\DatetimeCast.php

    public static function set(
        mixed $value,
        array $params = [],
        ?object $helper = null
    ): string {
        if (! $value instanceof Time) {
            self::invalidTypeValueError($value);
        }

        if (! $helper instanceof BaseConnection) {
            $message = 'The parameter $helper must be BaseConnection.';

            throw new InvalidArgumentException($message);
        }

        $format = match ($params[0] ?? '') {
            ''      => $helper->dateFormat['datetime'],
            'ms'    => $helper->dateFormat['datetime-ms'],
            'us'    => $helper->dateFormat['datetime-us'],
            default => throw new InvalidArgumentException('Invalid parameter: ' . $params[0]),
        };

        return $value->format($format);
    }

but you need to check if that's the proper way of handling it...

@wkolaczek
Copy link
Author

wkolaczek commented Jul 28, 2024

I've tested few more cases, and perhaps this needs to be addressed in the docs:

$now = \CodeIgniter\I18n\Time::now();
model('SomeModel')->where('my_dt_field', $now)->findAll();
echo $now->format("Y-m-d H:i:s.u"); // Outputs: 2024-07-28 18:57:58.900326
echo model("SomeModel")->db->getLastQuery(); // Outputs: SELECT * FROM `my_table` WHERE `my_dt_field` = '2024-07-28 18:57:58'

it seems like a proper way is to handle it in the app layer with
->where('my_dt_field', $now->format('Y-m-d H:i:s.u'))
as Model's casting feature is just Model's, not related to query builder...

@kenjis
Copy link
Member

kenjis commented Jul 29, 2024

Fixed DatetimeCast::set() and added notes to the docs. #9081

@kenjis
Copy link
Member

kenjis commented Jul 31, 2024

This will be fixed in v4.6.0 by #9081.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants