[Model] DateTime hardcoded format `Y-m-d H:i:s` drives to error on some databases

[mikeabbott10]

PHP Version

8.1

CodeIgniter4 Version

4.3.0

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

SQL Server 2019

What happened?

My db expects datetime format like 'd-m-Y H:i:s'. Unfortunately, the format 'Y-m-d H:i:s' is hardcoded in several parts of the CI code and trying to insert something into a table with, for example, 'created_at' column drives me to varchar-to-datetime conversion error.

Steps to Reproduce

Extend CodeIgniter\Model and try to save something to a table with 'created_at' (or 'updated_at') column, without explicitly set that value (doing so leaves BaseModel the task to handle the 'created_at' column value). The table is hosted in a db with DMY dateformat.

Expected Output

The insertion should work

Anything else?

The discussion started here. Initially, I thought it was only a CodeIgniter Shield problem, but it's not.

[kenjis]

Personally, I think it is a bad practice to change the date/time format in a program depending on the locale.
Locale should only be considered when displaying to end users.

However, if there is a database that does not accept Y-m-d H:i:s datetime, then it is better that we have the date/time format setting for database in somewhere.

[mikeabbott10]

A possible solution would be to allow a transiction when inserting/updating from inside the Model (so we can call SET DATEFORMAT ymd before), but I think just choosing the desired format for the pure insertion/update command is easier and does not change the logic.

If you would agree, I could go ahead and make a PR changing the logic behind the hardcoded format inside BaseModel, DeleteModelTest and FindModelTest (these are the classes I was able to notice that work with databases and dates). In case, if you think of anything else, please let me know.

[kenjis]

so we can call SET DATEFORMAT ymd before

It is not efficient.

@codeigniter4/database-team
Adding a property for datetime format to BaseModel is simple solution?

[michalsn]

Sounds like this is a model "problem", so I would make a change there... if we really want to make this happen.

Now, we can choose the format from datetime, date and int. Maybe something like this would be a good idea?

protected $dateFormat = 'datetime[d-m-Y H:i:s]';

It would be optional. No parameter would mean a standard format (Y-m-d H:i:s).

Although I don't know whether to expect the same from date. Only that then we can mess everything up pretty easily with date[Y-m-d H:i:s]. It would be nice to avoid the chaos...

[kenjis]

Only that then we can mess everything up pretty easily with date[Y-m-d H:i:s]. It would be nice to avoid the chaos...

Checks if it contains only Y m d ?

[kenjis]

But after all, we need the properties for the format strings (datetime and date)?
We need to solve codeigniter4/shield#608

[michalsn]

I think I would simplify it. We have datetime, date and int. And these values translate to the desired formats. In addition, we should add support for "normal" formatting, like:

protected $dateFormat = 'd-m-Y H:i:s';

And that's it. We can use $this->dateFormat in the linked LoginModel in Shield. That would resolve the issue, I think.

But changing $dateFormat may be challenging if we deal with a package like Shield. Probably we want to avoid copying every model to set a custom date format. Therefore, maybe we should add some master config in Shield or CodeIgniter core to set the default value for $modelDateFormat. The default value would be null which would favor the settings set in the model file.

[tswagger]

A parallel issue that is unrelated to locale... There are times where one will want to store microseconds.

A global setting would work. A model-level setting would be nice. A column-level setting would be really nice. Ideally with a cascading defaults (i.e. if column-level is not set, use model-level; if model-level is not set, use global setting).

[MGatner]

I hate how infrastructure-dependent our Model class is. It would be good to start a new solution for this, rather than continuing to massage Model/BaseModel to work with any database. What if Model just sticks with using Time values and then passes its properties off to a database-specific translator to handle the value conversions? This would have an immediate benefit of moving a bunch of bloat out of Model/BaseModel, and v5 benefit of allowing an infrastructure-agnostic model.

[kenjis]

@tswagger I sent a PR for Entity bug fix: #7995
If it is merged, we can set column-level behavior with Entity (Casting).

[kenjis]

New PR #8243 will solve this issue.
https://github.com/codeigniter4/CodeIgniter4/pull/8243/files#diff-3f4b24e9a6653ecb595b2b4a6f46322fdb47cd1c8355d8b19e661f12ceb15aa3R40-R42

[kenjis]

This issue is, after all, a database date/time format config. So, wouldn't it be a good idea to add dateFormat to the database config array?

Even if we set the date/time format in the Model property, we cannot change it in a library like Shield, because there is no configuration file for Models.

[michalsn]

It seems that this setting is only related to the model, so adding this setting to the database configuration doesn't seem right to me. I would rather see an additional Model configuration file to handle this (app/Config/Model.php).