PHPORM-148 Fix null in datetime fields and reset time in date field with custom format
#2741
Conversation
With a custom format, "date" cast behave like "datetime". The time is not reset to the start of the day. https://github.com/laravel/framework/blob/v10.46.0/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L866-L869
| $castType = $this->getCasts()[$key]; | ||
| if ($this->isCustomDateTimeCast($castType) && str_starts_with($castType, 'date:')) { | ||
| $value->startOfDay(); | ||
| if (str_starts_with($castType, 'date:') || str_starts_with($castType, 'immutable_date:')) { |
There was a problem hiding this comment.
The is no method to check a "date" cast, as opposed to "datetime" cast that keep the time.
| 'immutable_custom_datetime','immutable_datetime' => str_starts_with($this->getCasts()[$key], 'immutable_date:') ? | ||
| $this->asDate($value)->toImmutable() : | ||
| $this->asDateTime($value)->toImmutable(), |
There was a problem hiding this comment.
The cast types immutable_custom_datetime and immutable_datetime are already supported by the parent method: HasAttribute::castAttribute()
There was a problem hiding this comment.
How does $this->getCastType($key) differ from $this->getCasts()[$key]? I don't understand why this would match on immutable_custom_datetime or immutable_datetime but then see a immutable_date: prefix from the latter method.
I did note that the function you linked already handles:
case 'date':
return $this->asDate($value);
...
case 'immutable_date':
return $this->asDate($value)->toImmutable();
There was a problem hiding this comment.
$this->getCasts()[$key]is what is defined in the$castsproperty (date:j.n.Y H:i).$this->getCastType($key)is a classification (custom_datetime)
| if ($this->isCustomDateTimeCast($castType) && str_starts_with($castType, 'date:')) { | ||
| $value->startOfDay(); | ||
| if (str_starts_with($castType, 'date:') || str_starts_with($castType, 'immutable_date:')) { | ||
| $value = $value->startOfDay(); |
There was a problem hiding this comment.
$value is reassigned because of immutable object.
|
|
| 'immutable_custom_datetime','immutable_datetime' => str_starts_with($this->getCasts()[$key], 'immutable_date:') ? | ||
| $this->asDate($value)->toImmutable() : | ||
| $this->asDateTime($value)->toImmutable(), |
There was a problem hiding this comment.
How does $this->getCastType($key) differ from $this->getCasts()[$key]? I don't understand why this would match on immutable_custom_datetime or immutable_datetime but then see a immutable_date: prefix from the latter method.
I did note that the function you linked already handles:
case 'date':
return $this->asDate($value);
...
case 'immutable_date':
return $this->asDate($value)->toImmutable();
| } | ||
|
|
||
| /** @inheritdoc */ | ||
| protected function castAttribute($key, $value) |
There was a problem hiding this comment.
Is the removal of this method what actually fixes the null-handling issue cited in PHPORM-148?
There was a problem hiding this comment.
The actual fix is more in transformModelValue. It seemed to me that this code was an incomplete attempt to remove the time from immutable_date with custom format; which failed with null values.
There was a problem hiding this comment.
LGTM, although I don't fully grasp the moving parts here.
I'm satisfied with the test cases covering the original reproduction in PHPORM-148.
Co-authored-by: Andreas Braun <git@alcaeus.org>
3 participants
Fix PHPORM-148
Fix #2729
Model::castAttribute()that was introduced by Datetime casting with custom format #2658 in 4.1.0. Tests added to ensurenullis allowed indate,datetime,immutable_date,immutable_datetimeand the variants with custom formats.immutable_date:j.n.Y H:i(with custom format), to reset the time. This behave differently than Laravel 10.46 that treats it like aimmutable_datetime.Checklist