CodeIgniter4
CodeIgniter4 copied to clipboard
codeigniter4
[Model] DateTime hardcoded format `Y-m-d H:i:s` drives to error on some databases
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.
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.
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.
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?
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...
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 ?
But after all, we need the properties for the format strings (datetime and date)? We need to solve https://github.com/codeigniter4/shield/issues/608
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.
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).
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.
@tswagger ~~I sent a PR for Entity bug fix: #7995 If it is merged, we can set column-level behavior with Entity (Casting).~~
New PR #8243 will solve this issue. https://github.com/codeigniter4/CodeIgniter4/pull/8243/files#diff-3f4b24e9a6653ecb595b2b4a6f46322fdb47cd1c8355d8b19e661f12ceb15aa3R40-R42
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.
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).
We could use this new config to handle other things in a model, like: https://github.com/codeigniter4/CodeIgniter4/pull/8455
Yes, so far there does not seem to be any place to use the date format setting other than in Model.
So, it is possible to add a Config file for Model.
I thought again. Currently, this date/time format is only relevant for Model. However, it is essentially an attribute of a database connection.
If you are using two database connections, one with d-m-Y H:i:s and the other with Y-m-d H:i:s, then a Model with a single date/time foramt cannot handle this.
An implementation where the database connection has a date/time format and the model uses it seems correct.
@MGatner
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?
It is fine that Model just use only Time. In that case, the issue will be how do we set the date/time format to the database-specific translator.
At that point it would need to be a concern of the database driver. For example, in Builder any time it added something to $QBSet it could check if the value instanceof Time and convert it to the appropriate string. The alternative is driver-specific datetime field classes:
class MySqlTime extends Time
{
protected string $toStringFormat = 'Y-m-d H:i:s';
}
This bleeds the underlying driver out into userland code though, because setting or modifying that field requires knowing its type.
Honestly: we're cheating right now. We use a Time class whose string representation is compatible with 95% of the database servers in use with CodeIgniter, so it's a non-issue. But really we're leaking MySQL dependency everywhere we use this driver-specific class. It makes sense given how prevalent datetime fields are in databases, but it is also why we keep hitting a wall any time we try to set code boundaries in our Entity-Model-Builder-Connection chain.
In the following layers, I think this datetime format belongs to Connection. So I sent #8525.
Entity
Model
Builder
Connection
I agree, as far as a configurable solution goes. Dates/times are a unique because a) all databases (AFAIK) have a native scalar type for them, while PHP does not, and b) they are super common (unlike, e.g. spatial data). I do wonder if Time is too bulky/opinionated a solution for an equivalent PHP scalar, but it's the framework's universal date/time representation so it seems the obvious choice.
There's one other assumption in the translation we should be mindful of, that Time is always timezone-specific whereas database DATETIME is not (TIMESTAMP is always UTC).
There's one other assumption in the translation we should be mindful of, that
Timeis always timezone-specific whereas databaseDATETIMEis not (TIMESTAMPis always UTC).
I sent PR #8544 for a bug fix regarding time zones.