alexeymezenin / laravel-best-practices

Laravel best practices
11.24k stars 2.37k forks source link

Store dates in the standard format. Use accessors and mutators to modify date format #141

Closed JohnnyWalkerDigital closed 1 year ago

JohnnyWalkerDigital commented 2 years ago

I agree with this description and this rule, but the example is unclear and confusing.

Basically we should be illustrating that you should be passing Carbon objects (as per the rest of the guide), and not creating new objects with unique formatting, etc.

spekulatius commented 1 year ago

Agree, it's a bit confusing. I was wondering if it actually should be an attribute. I would make it a method call and pass in the date to format. How would you adjust it to make it more clear @JohnnyWalkerDesign ?

JohnnyWalkerDigital commented 1 year ago

I think the example is just generally confusing (especially compared to the rest of the document), and agree that I'm not sure it should be an attribute. Plus there's no harm in passing around a Carbon object and allowing it to be used appropriately in a given context. It's designed to be flexible.

I think most problems with dates/times come from passing around strings instead of date objects. If you've done any amount of work with dates and timezones, you quickly learn that a string isn't a date (and vice versa), and it's a big mistake to think of them that way.

Maybe that's a separate issue to what this best practice is trying to instil, but the example is still very confusing. And dates comes with far more complexities than than this best practice touches upon.

Sorry, I don't have an improvement to offer. It feels like a can of worms :(

spekulatius commented 1 year ago

I think the example is just generally confusing (especially compared to the rest of the document), and agree that I'm not sure it should be an attribute. Plus there's no harm in passing around a Carbon object and allowing it to be used appropriately in a given context. It's designed to be flexible.

I think most problems with dates/times come from passing around strings instead of date objects. If you've done any amount of work with dates and timezones, you quickly learn that a string isn't a date (and vice versa), and it's a big mistake to think of them that way.

I strongly agree, it's better to keep it in a Carbon. Structured data usually beats unstructured.

Maybe that's a separate issue to what this best practice is trying to instil, but the example is still very confusing. And dates comes with far more complexities than than this best practice touches upon.

Yeah, that's the question. I've looked at some projects of varying age and found different approaches uses.

Sorry, I don't have an improvement to offer. It feels like a can of worms :(

No problem, I looked at it and wondered too: how would I refactor it?

So, to make a step towards an improvement I'll put up some suggests based on the original:

// Model
protected $dates = ['ordered_at', 'created_at', 'updated_at'];
public function getSomeDateAttribute($date)
{
    return $date->format('m-d');
}

// View
{{ $object->ordered_at->toDateString() }}
{{ $object->some_date }}
spekulatius commented 1 year ago

1. Formatter Pattern

An interface with implementation. Most structured, most code:

interface Formatter
{
    public function format(Carbon $date): string;
}

class DateFormatter implements Formatter
{
    public function format(Carbon $date): string
    {
        return $date->format('Y-m-d');
    }
}

// View
{{ (new DateFormatter)->format($object->created_at) }}
spekulatius commented 1 year ago

2. Formatter Method in Model

This formatter is a bit simplified by moving it simply in the model. This saves some code, but isn't reusable. It also contains something I'm not a fan of: passing-in a value into a method of the same object.

// Model
protected $dates = ['ordered_at', 'created_at', 'updated_at'];

public function renderDate(Carbon $date)
{
    return $date->format('m-d');
}

// View
{{ $object->renderDate($object->created_at) }}
spekulatius commented 1 year ago

3. Formatter Trait

Leveraging native PHP we can do better than 2. It's pretty much the same as above, but this time in a trait to allow DRY reusing.

Same as above it also contains the passing-in a value into a method of the same object. It could be worked around by passing an identifier instead, but that's more cosmetics.


trait FormatsDates
{
    public function formatDate(Carbon $date): string
    {
        return $date->format('Y-m-d');
    }
}

// View
{{ $object->formatDate($object->ordered_at) }}
spekulatius commented 1 year ago

I hope the examples are clear enough. I've just written them down quickly without adding much context. It's more a draft than a ready-to-use example.

In regard to a choice, I've got my personal preference, but I'm not sure that this matches the majority.

spekulatius commented 1 year ago

Hey @alexeymezenin

do you have a preference regarding the dates?

Cheers, Peter

alexeymezenin commented 1 year ago

I don't think it's a good idea to set date format in models. I would pass Carbon object to a view and set display format there (i.e. based on current country).

spekulatius commented 1 year ago

Yeah, I see your point. Maybe we just adjust it to something like this:

A date as a string is less reliable than an object instance, e.g. a Carbon-instance. It's recommended to pass Carbon objects between classes instead of date strings. Rendering should be done in the display layer (templates):

Bad

{{ Carbon::createFromFormat('Y-d-m H-i', $object->ordered_at)->toDateString() }}
{{ Carbon::createFromFormat('Y-d-m H-i', $object->ordered_at)->format('m-d') }}

Good:

// Model
protected $dates = [
    'ordered_at',
];

// Blade view
{{ $object->ordered_at->toDateString() }}
{{ $object->ordered_at->format('m-d') }}

This might explain it a bit better.

spekulatius commented 1 year ago

I've updated the information @JohnnyWalkerDesign