[6.x] Added a new mutator cast type - `time` #30931

andrey-helldar · 2019-12-25T08:43:21Z

Adding a new type of mutator will make it easier to work with time comparisons.

For example:

$model = App\Models\Foo::first();
$now = now();

return $now->gte($this->open_at) && $now->lt($this->close_at);
    ? __('Opened')
    : __('Closed');

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

brunogaspar · 2019-12-25T12:14:08Z

Not saying that the mutator is not useful, it might be, but why aren't you using Carbon itself for the above comparisons/checks?

Using the lte() and lt() methods from Carbon should make the above work without the need for such a new cast mutator on the framework, it also takes the date into the equation instead of just the time, which the above does not, unless you don't care about date, which is fine i suppose, but still, seems a bit of broken functionality on your application for future requirements.

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

mfn · 2019-12-25T12:32:37Z

What bothers me a bit is that there is not true "time representation" by itself. It is always a full date object (this is simply an intrinsic behaviour of \DateTime itself, unrelated to carbon et al).

When it comes down to it, the only difference between the date and the time cast is really the createFromFormat('H:i:s', …) part.

andrey-helldar · 2019-12-25T13:11:33Z

Not saying that the mutator is not useful, it might be, but why aren't you using Carbon itself for the above comparisons/checks?

Using the lte() and lt() methods from Carbon should make the above work without the need for such a new cast mutator on the framework, it also takes the date into the equation instead of just the time, which the above does not, unless you don't care about date, which is fine i suppose, but still, seems a bit of broken functionality on your application for future requirements.

I have 47 applications in my work in which I need to display different information during working hours and after hours. Since all sites have the same code section, putting functionality into a package is, in my opinion, an overhead. Therefore, I suggested adding a new type of castes "in the box."

There is a TIME field type in the database and I think it’s right to work with it this way.

andrey-helldar

What bothers me a bit is that there is not true "time representation" by itself. It is always a full date object (this is simply an intrinsic behaviour of \DateTime itself, unrelated to carbon et al).

When it comes down to it, the only difference between the date and the time cast is really the createFromFormat('H:i:s', …) part.

The difference is not only in the code, but also in the database, in which the TIME type field contains only time in the format "H: i: s" meaning that this time is "today".

I think it’s right to wrap the functionality in Carbon since it may require an easy use of this feature.

For example, a simple case:
(It looks awful, but the goal is to show the opportunity.)

// Model
protected function getIsOpenedAttribute()
{
    $now = now();

    return $now->gte($this->open_at) && $now->lt($this->close_at);
}

// Blade
@if($model->is_opened)
    @lang('Opened')
@else
    @lang('Closed')
@endif

And another example:

$now    = now()->startOfSecond();
$carbon = Carbon::createFromFormat('H:i:s', date('H:i:s'))->startOfSecond();

dd([
    '$time === $carbon' => $now === $carbon,
    '$time == $carbon'  => $now == $carbon,

    '$time < $carbon' => $now < $carbon,
    '$time > $carbon' => $now > $carbon,

    '$now'    => $now,
    '$carbon' => $carbon,
]);

array:6 [
  "$time === $carbon" => false
  "$time == $carbon" => true
  "$time < $carbon" => false
  "$time > $carbon" => false
  "$now" => Illuminate\Support\Carbon @1577281351 {#32
    date: 2019-12-25 16:42:31.0 Europe/Moscow (+03:00)
  }
  "$carbon" => Carbon\Carbon @1577281351 {#881
    date: 2019-12-25 16:42:31.0 Europe/Moscow (+03:00)
  }
]

andrey-helldar · 2019-12-25T13:45:55Z

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php

+    {
+        return $value instanceof CarbonInterface
+            ? Date::instance($value)
+            : Date::createFromFormat('H:i:s', $value)->startOfSecond();


I added a microsecond reset because without it the time comparison would be wrong.

The TIME field type of the database does not store information about milliseconds, and when the now () functions are called in sequence, the values will be different.
The startOfSecond () method leads them to a single value, as a result of which the comparison will be correct.

The result of the verification showed here: #30931 (review)

andrey-helldar · 2019-12-25T13:47:26Z

If the text is not written correctly in docblock, please tell me how to.
Unfortunately, I do not speak English and translate with the help of Google Translator, but he is mistaken. You know.

taylorotwell · 2019-12-25T15:04:41Z

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

andrey-helldar · 2019-12-25T18:06:30Z

@taylorotwell, this is not a bug. This is a feature addition.

I got your point. But I believe that this functionality in several lines of code is not advisable to put in a package.

I want to ask: do you think it's worth putting control of castes into an instance based on value-object pattern?

I can do it. But is it necessary?

andrey-helldar changed the title ~~Added a new mutator cast type - time~~ [6.x] Added a new mutator cast type - time Dec 25, 2019

brunogaspar reviewed Dec 25, 2019

View reviewed changes

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php Outdated Show resolved Hide resolved

mfn reviewed Dec 25, 2019

View reviewed changes

src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php Outdated Show resolved Hide resolved

andrey-helldar force-pushed the patch-2019-12-25-11-01 branch 2 times, most recently from 503c20c to d9d2f42 Compare December 25, 2019 13:08

andrey-helldar commented Dec 25, 2019

View reviewed changes

Added a new mutator cast type - time

9e70f31

andrey-helldar force-pushed the patch-2019-12-25-11-01 branch from d9d2f42 to 9e70f31 Compare December 25, 2019 13:40

andrey-helldar commented Dec 25, 2019

View reviewed changes

taylorotwell closed this Dec 25, 2019

This was referenced Dec 27, 2019

[6.x] Added creation of custom Cast classes for Eloquent #30953

Closed

[6.x] Added creation of custom Cast classes for Eloquent #30958

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[6.x] Added a new mutator cast type - `time` #30931

[6.x] Added a new mutator cast type - `time` #30931

andrey-helldar commented Dec 25, 2019 •

edited

Loading

brunogaspar commented Dec 25, 2019

mfn commented Dec 25, 2019

andrey-helldar commented Dec 25, 2019

andrey-helldar left a comment •

edited

Loading

andrey-helldar Dec 25, 2019

andrey-helldar commented Dec 25, 2019

taylorotwell commented Dec 25, 2019

andrey-helldar commented Dec 25, 2019

[6.x] Added a new mutator cast type - time #30931

[6.x] Added a new mutator cast type - time #30931

Conversation

andrey-helldar commented Dec 25, 2019 • edited Loading

brunogaspar commented Dec 25, 2019

mfn commented Dec 25, 2019

andrey-helldar commented Dec 25, 2019

andrey-helldar left a comment • edited Loading

Choose a reason for hiding this comment

andrey-helldar Dec 25, 2019

Choose a reason for hiding this comment

andrey-helldar commented Dec 25, 2019

taylorotwell commented Dec 25, 2019

andrey-helldar commented Dec 25, 2019

[6.x] Added a new mutator cast type - `time` #30931

[6.x] Added a new mutator cast type - `time` #30931

andrey-helldar commented Dec 25, 2019 •

edited

Loading

andrey-helldar left a comment •

edited

Loading