Accessors with DateTimeInterface (Carbon) return value won't get casted to datestring on json-response by default

JuBlatt commented 4 years ago

Laravel Version: 6.11.0
PHP Version: 7.2.15
Database Driver & Version: MySQL 5.6.44

Description:

If we declare a field, let it be named "our_date" in the $dates array to be casted as dates, it will be handled as a DateTime object. So if we get the value, we can work with a carbon instance and if we return this model instance in any json-response, it will be casted to a date string representation (e.g. 2020-01-15 14:00:00). So far, so good.

If we - for some reason - want to manipulate the returned date value for a field with a accessor /get-mutator (in our project, we fetch a date from a parent model, if no date is available on model instance), this default date casting will not take place, even if we make sure to return a Carbon Instance.

Instead of 2020-01-15 14:00:00 we will get 2020-01-15T14:00:00.000000Z returned.

This behaviour comes from the Illuminate\Database\Eloquent\Concerns\HasAttributes@attributesToArray method, which is called internally when a model is returned in a json-response and handles the casting / mutation of attributes when a model is casted to array.

There you can find following note:

    // If an attribute is a date, we will cast it to a string after converting it
    // to a DateTime / Carbon instance. This is so we will get some consistent
    // formatting while accessing attributes vs. arraying / JSONing a model.

which is a very good approach, I would say. However, in my opinion, it would be nice if get-mutators (which are respected on toArray) that return a DateInstance for a field which is declared as date in the $dates-array on model are casted to a string on json-responses in the same manner.

Steps to reproduce

Declare any DateTime field for date-casting on $dates array.
Define an accessor for the same attribute with a carbon object as return value.
For testing, just return $this->asDate($value) in accessor to make sure you run the same code for the datetime value laravel would run internally.
Return the model on json-response.

You will have an date-object with timezone information in response instead of a string representation which is not consistent with what you get for dates that are casted without an accessor.

Possible solution

I don't know if this would have an effect on any other components I can't think of now, but as I can see, we only would need to change the order in which the mutation / date casting functions are called in the attributesToArray-function, to achieve this:

I could "fix" this "issue" for me by changing this:

    /**
     * Convert the model's attributes to an array.
     *
     * @return array
     */
    public function attributesToArray()
    {
        // If an attribute is a date, we will cast it to a string after converting it
        // to a DateTime / Carbon instance. This is so we will get some consistent
        // formatting while accessing attributes vs. arraying / JSONing a model.
        $attributes = $this->addDateAttributesToArray(
            $attributes = $this->getArrayableAttributes()
        );

        $attributes = $this->addMutatedAttributesToArray(
            $attributes, $mutatedAttributes = $this->getMutatedAttributes()
        );

        // Next we will handle any casts that have been setup for this model and cast
        // the values to their appropriate type. If the attribute has a mutator we
        // will not perform the cast on those attributes to avoid any confusion.
        $attributes = $this->addCastAttributesToArray(
            $attributes, $mutatedAttributes
        );

        // Here we will grab all of the appended, calculated attributes to this model
        // as these attributes are not really in the attributes array, but are run
        // when we need to array or JSON the model for convenience to the coder.
        foreach ($this->getArrayableAppends() as $key) {
            $attributes[$key] = $this->mutateAttributeForArray($key, null);
        }

        return $attributes;
    }

to this:

    /**
     * Convert the model's attributes to an array.
     *
     * @return array
     */
    public function attributesToArray()
    {
        $attributes = $this->addMutatedAttributesToArray(
            $this->getArrayableAttributes(), $mutatedAttributes = $this->getMutatedAttributes()
        );

        // If an attribute is a date, we will cast it to a string after converting it
        // to a DateTime / Carbon instance. This is so we will get some consistent
        // formatting while accessing attributes vs. arraying / JSONing a model.
        $attributes = $this->addDateAttributesToArray(
            $attributes
        );

        // Next we will handle any casts that have been setup for this model and cast
        // the values to their appropriate type. If the attribute has a mutator we
        // will not perform the cast on those attributes to avoid any confusion.
        $attributes = $this->addCastAttributesToArray(
            $attributes, $mutatedAttributes
        );

        // Here we will grab all of the appended, calculated attributes to this model
        // as these attributes are not really in the attributes array, but are run
        // when we need to array or JSON the model for convenience to the coder.
        foreach ($this->getArrayableAppends() as $key) {
            $attributes[$key] = $this->mutateAttributeForArray($key, null);
        }

        return $attributes;
    }

In our application I actually just did an override of that function on the only model where this problem exists for us like the following:

        $attributes = parent::attributesToArray();

        $attributes = $this->addDateAttributesToArray(
            $attributes
        );

        return $attributes;

Well, one could say "But the mutator should exclusively decide, what the representation of the mutated value is" - which is right. But in that case, the attribute should not be in the $dates-array anyway.

Discussion

If the mutator returns a valid carbon instance, why shouldn't it be casted to string in the same manner like any other dates on response? Is there some reason I don't see here? I don't see any convenient way to make sure my mutated value is casted to the same datestring as all other dates (e.g. updated_at) in my application, aside from introducing API resources for the affected models just for that case.

Please let me know what you guys think and If something is missing in this issue. I'm not really into creating issues / contributing. Shame on me.

driesvints commented 4 years ago

Please fill out the full issue template

JuBlatt commented 4 years ago

@driesvints I re-arranged the initial comment and added the php / db information.

driesvints commented 4 years ago

@BillyBoeklunder what is your exact v6 version? Why did you add a 5.4 version?

JuBlatt commented 4 years ago

Hey @driesvints, I did that because I could see that the relevant code part didn't change in the version range I added. But you're right. Let's focus on only one version, as the others are not supported with fixes anyway. I did some tests on the latest version, added the version on initial comment and added a response example.

driesvints commented 4 years ago

Hey @BillyBoeklunder, thanks for reporting this. I am however, transferring this to the ideas repo as I believe the current behaviour to be the expected behaviour. You seem to be wanting a change in behaviour so this discussion is more suited for the ideas repo and not the bug tracker of the framework.

I do want to note that some changes are coming to the default serialization of dates in Laravel 7. I don't think that directly influences what you'd like to change here but it might be of interest to you: https://github.com/laravel/framework/pull/30715

JuBlatt commented 4 years ago

Hey @driesvints, thank you for your quick response.

You seem to be wanting a change in behaviour

I don't really want a behaviour to change, I actually want a behaviour to be applied consistantly for attributes that are declared as "date" on a model, even if they are additionally processed by an accessor. If it comes to json serialize of a model, I would expect that attributes with the same data-type are all processed in the same manner. I don't really understand why someone would expect a different representation for the same type.

Anyway, I'm fine with moving it to "ideas". Just wanted to express my opinion to clarify. I don't want to tell anybody how you guys should treat things in a project where I'm not really involved. I'm still new on open source projects of that scale. I'm sure that I miss a lot of things about the framework and if somebody with more knowledge about it sees it as a bug in the future, we can still change it.

Thank you for your work.

laravel / ideas