feat: QueryBuilder insert/update support Time object

[kenjis]

Description
Closes #6439

Now you can use Time in all locales.

    $time = Time::parse('2022-08-29 13:00');
    $builder->set('last_active', $time)->where('id', 1)->update();

Background:

1. Basically QueryBuilder does not support objects.
2. But it supports stringable objects.
3. Time is stringable.
4. But it is localized, so it will be converted to invalid datetime string in a few locales.
5. So only in a few locales, if you pass Time, it causes a query error.
6. Time is a standard class in CI4.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

I'm going to reference my original comment just so it is included here. I'm glad to see that Deptrac agreed with me 🤗

#6439 (comment)

[iRedds]

What if I want the Time object to be converted to a unix timestamp?
Not sure if this is the correct approach.

[kenjis]

What if I want the Time object to be converted to a unix timestamp?

Pass the timestamp to QueryBuilder.
I don't intend to support much more objects.

Background:

1. Basically QueryBuilder does not support objects.
2. But it supports stringable objects.
3. Time is stringable.
4. But it is localized, so it will be converted to invalid datetime string in a few locales.
5. So only in a few locales, if you pass Time, it causes a query error.
6. Time is a standard class in CI4.

[iRedds]

Pass the timestamp to QueryBuilder.

In the same way, you can also pass a timestamp in ISO format to the QueryBuilder.
It turns out that we are making the "make me happy" button to the developer, who cannot bring the data to the desired format himself.
This is bad practice.

[kenjis]

It turns out that we are making the "make me happy" button to the developer, who cannot bring the data to the desired format himself.

It may be true, but in this case most developers cannot bring the data to the desired format because of the ignorance.
Because $builder->set('last_active', Time::now())->where('id', 1)->update() works in most locales.

Do you want to raise an Exception when any object is passed?

Even if we do so, if a developer write code like $builder->set('last_active', (string) Time::now())->where('id', 1)->update(), it raises a query error in a few locales.

I am not sure, but perhaps the design of Time object that changes behavior depending on the locale is a mistake.

[iRedds]

set('last_active', Time::now()->format()); // or ->toDatabase()

To be honest, I don't know what it's for locale dependency to serve timestamps within the application. Except when it needs to be displayed to the user.

[kenjis]

Yes, every time you pass Time to QueryBuilder, you should call toDatabase().

But I think it is difficult for human beings.
The actual code is not so obvious the value is Time object or not, something like:

set('last_active', $user->last_active)

[kenjis]

To be honest, I don't know what it's for locale dependency to serve timestamps within the application. Except when it needs to be displayed to the user.

Indeed.

It might be better to fix Time::__toString().

[kenjis]

I created another PR #6461 to fix Time.
Which is better?

[MGatner]

I worry that will be a disruptive breaking change for anyone using a locale without decimal numbers.

A third option (not without its own issues) we create DatabaseTime extends Time, override the string function, and change Model & Entity to use that for casting.

[michalsn]

My 2 cents... I don't think we should change anything about the current behavior. We have toDatabase() and format() methods, which should be used.

Also "ignorance of developers" is not a good advertisement for adding this feature. Like... it's not a framework fault when users don't know how to properly handle the date. Hopefully, they will learn when they see a query error.

As for #6461 - "fixing" the Time this way would be a big BC break. IMO not really acceptable for v4.

[kenjis]

Also "ignorance of developers" is not a good advertisement for adding this feature. Like... it's not a framework fault when users don't know how to properly handle the date. Hopefully, they will learn when they see a query error.

It is very difficult to learn or notice it, because ignorant developers never see the query error.
Because most of them don't use the locale like fa.
And they can know they created a bug when they get a bug report.
I mean developers here is a framework/library developers including us.

If we expect developers to handle this properly, we need to find the bug in codeigniter4/shield#392 in the review.
But I think it is too difficult.

[datamweb]

we need to find the bug in codeigniter4/shield#392 in the review.

Because the main cause of this problem has been identified in the locale fa, ar and ... we now expect the developer to have the necessary knowledge.
But in fact, before the problem was identified, in addition to the fact that we did not see the bug in the review, even after the bug was reported, it took time to determine the cause.

[iRedds]

If we expect developers to handle this properly, we need to find the bug in codeigniter4/shield#392 in the review.

The last_active attribute is defined as a date, but DatetimeCast does not implement a set method. This means that if you try to set the last_active attribute to Time, it will not convert to the string representation of the date (that is, as recorded in the database).

That is, for last_active you just need to add a setter that converts to a string.

[michalsn]

This may sound harsh, but... basically, it seems like we missed a "thing" and merged buggy code, and now we're trying to prevent it by changing the class behavior.

Things like this are easy to catch if we use strictOn set to true for database connection - at least in our development environment. I always encourage people to do so.

For me, everything comes down to good habits when working with dates in general. Although working with Entities and Models makes things no hassle, we have to be precise with QueryBuilder. My personal preference would be to keep it that way.

[kenjis]

Things like this are easy to catch if we use strictOn set to true for database connection - at least in our development environment. I always encourage people to do so.

It is not. strictOn does not help us in this case.
Because we probably don't test with locale fa, so we never see the query error.

But I think #6461 is better than this PR now. It does not change QueryBuilder at all.

[kenjis]

That is, for last_active you just need to add a setter that converts to a string.

The setter does not help us in this case.
Because the getter returns a Time object.

[iRedds]

The setter does not help us in this case.
Because the getter returns a Time object.

During saving, the toRawArray() method is called, which returns the desired value.
In this case, calling ->save($user); it would be enough.

[kenjis]

In this case, calling ->save($user); it would be enough.

In this case, we need to avoid updating the updated_at column.

[MGatner]

I think @iRedds is correct. As we've been seeing with a lot of these recent Entity discussions it is important to keep the two concerns strict: persistent storage ("raw" $attributes) and developer properties (anything cast). In this case the lack of a setter for DateTimeCast means that we are holding objects in our persistent storage array. It "works" currently by accident because Time is Stringable and it's string form fits in the database - until the locale changes.

This doesn't solve QueryBuilder if a developer calls set() directly with a Time object, but I think that's a separate decision still under discussion ("how accommodating are we to be").

[kenjis]

@MGatner I also think it is bad design that the current Entity has Time object in $attributes.

It "works" currently by accident because Time is Stringable and it's string form fits in the database

No, BaseModel takes care of Time objects.

[MGatner]

I will add, since this came up organically, that I've been working on an alternate data layer concept that follows the repository pattern more closely and addresses a number of these issues that I think ultimately are architecture problems. Still very much a work in progress but I hope this will be some inspiration for our data access in v5: https://github.com/tattersoftware/codeigniter4-repositories

One of the fundamental differences (which comes straight from the repository pattern) is that an Entity is a domain object and has no concerns about persistence except at the layer boundary where it becomes a repository-specific data-transfer object (array, in my case). Practically this means: Entity values are always "for the developer" and thus favor value objects (self-validating) and cast types, and any persistence concerns happen in toArray() and are specific to the current Entity.

[MGatner]

BaseModel takes care of Time objects

Oh right! I forgot about that one. Still a concern though.

[kenjis]

I would like to proceed #6461 instead of this.

[MGatner]

I agree, that is a better solution.

[sclubricants]

I thought of this. It would be nice if a select query could return date time objects as well. I though of creating a wrapper class around date/time object. Have __toString format according to database and have configuration setting to format for front end.

[kenjis]

#6461 was merged.