Skip to content

Commit

Permalink
Stop Eloquent model setAttribute() modifying date cast string values
Browse files Browse the repository at this point in the history
This affects model attribute casts 'date', 'datetime', 'immutable_date',
and 'immutable_datetime' with a custom date format being used.

e.g., $casts = ['month' => 'datetime:Y-m']

Last week a fix was introduced for attributes being set with Carbon
objects that were mistakenly flagged as "dirty" in the model even though
dates were the same. However that fix broke behavior of date _strings_
such as $request->validated() being filled into a model.

For:

$model->fill(['month' => '2021-09'])->save();

Instead of the SQL query binding being "2021-09", it's "2021-09-01 00:00:00"
because the date string is reformatted through fromDateTime(), using
getDateFormat() and not the custom cast date.

Keep the v8.60.0 behavior that sets the raw date string in the
attributes payload which is eventually bound in the database query.
Avoid going through fromDateTime() that reformats that string.

Also remove the added redundant conditional in originalIsEquivalent()
as method isDateAttribute() already includes isDateCastable().
  • Loading branch information
derekmd committed Sep 18, 2021
1 parent 3806a98 commit 7f90315
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 3 deletions.
6 changes: 4 additions & 2 deletions src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,9 @@ public function setAttribute($key, $value)
// If an attribute is listed as a "date", we'll convert it from a DateTime
// instance into a form proper for storage on the database tables using
// the connection grammar's date format. We will auto set the values.
elseif ($value && $this->isDateAttribute($key)) {
elseif ($value && $this->isDateAttribute($key) && !(
$this->hasCast($key, ['custom_datetime', 'immutable_custom_datetime']) && is_string($value)
)) {
$value = $this->fromDateTime($value);
}

Expand Down Expand Up @@ -1643,7 +1645,7 @@ public function originalIsEquivalent($key)
return true;
} elseif (is_null($attribute)) {
return false;
} elseif ($this->isDateAttribute($key) || $this->isDateCastable($key)) {
} elseif ($this->isDateAttribute($key)) {
return $this->fromDateTime($attribute) ===
$this->fromDateTime($original);
} elseif ($this->hasCast($key, ['object', 'collection'])) {
Expand Down
41 changes: 40 additions & 1 deletion tests/Integration/Database/EloquentModelDateCastingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,46 @@ public function testDatesAreCustomCastable()
$this->assertInstanceOf(Carbon::class, $user->datetime_field);
}

public function testCustomDateCastsAreComparedAsDates()
public function testDatesFormattedAttributeBindings()
{
$bindings = [];

$this->app->make('db')->listen(static function ($query) use (&$bindings) {
$bindings = $query->bindings;
});

$user = TestModel1::create([
'date_field' => '2019-10',
'datetime_field' => new CarbonImmutable('2019-10-01 10:15:20'),
'immutable_date_field' => new CarbonImmutable('2019-10-01'),
'immutable_datetime_field' => '2019-10-01 10:15',
]);

$this->assertSame(['2019-10', '2019-10-01 10:15:20', '2019-10-01 00:00:00', '2019-10-01 10:15'], $bindings);

This comment has been minimized.

Copy link
@macbookandrew

macbookandrew Sep 27, 2021

Contributor

@derekmd I’m working on a fix for #38828 and #38720.

I had to update your test for it to pass against the current 8.x branch: 04ae160

Was there a reason you expected the Y-m and Y-m-d H:i formats in the $bindings, rather than Y-m-d and Y-m-d H:i:s?

This comment has been minimized.

Copy link
@derekmd

derekmd Sep 27, 2021

Author Contributor

@andrewminion-luminfire:

The test confirms v8.60 behavior. It checks Model::create() -> fill() -> setAttribute(), then save() doesn't modify date string values if a custom date cast is defined for the attribute. Your additional commit for that test to add time of day shows strings are being modified by Eloquent's date handling pre-query builder. This is what causes issues with date columns in MySQL strict mode.

The SQL binding date format is completely unrelated to the format indicated in the $casts array which is used by toArray() & toJson(). If any date cast exists with a format, the Eloquent v8.60 setter would leave date strings alone and trust the app knows what it's doing.

Commenting out this test's CarbonImmutable lines, it currently passes on the 8.x HEAD after Taylor's revert commit. As he said earlier, it's doubtful he'll merge another attempt. Dirty fixes have left a graveyard of PRs & issues since they keep introducing new edge cases and there are some test coverage gaps.

}

public function testCustomDateCastsAreComparedAsDatesForStringValues()
{
/** @var TestModel1 */
$user = TestModel1::create([
'date_field' => '2019-10-01',
'datetime_field' => '2019-10-01 10:15:20',
'immutable_date_field' => '2019-10-01',
'immutable_datetime_field' => '2019-10-01 10:15:20',
]);

$user->date_field = '2019-10-01';
$user->datetime_field = '2019-10-01 10:15:20';
$user->immutable_date_field = '2019-10-01';
$user->immutable_datetime_field = '2019-10-01 10:15:20';

$this->assertArrayNotHasKey('date_field', $user->getDirty());
$this->assertArrayNotHasKey('datetime_field', $user->getDirty());
$this->assertArrayNotHasKey('immutable_date_field', $user->getDirty());
$this->assertArrayNotHasKey('immutable_datetime_field', $user->getDirty());
}

public function testCustomDateCastsAreComparedAsDatesForCarbonValues()
{
/** @var TestModel1 */
$user = TestModel1::create([
Expand Down

0 comments on commit 7f90315

Please sign in to comment.