Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

result of serializeDate is being parsed but shouldn't be #20116

Closed
dsandber opened this issue Jul 18, 2017 · 20 comments
Closed

result of serializeDate is being parsed but shouldn't be #20116

dsandber opened this issue Jul 18, 2017 · 20 comments

Comments

@dsandber
Copy link

dsandber commented Jul 18, 2017

  • Laravel Version: 5.4.28
  • PHP Version: 7.1.1
  • Database Driver & Version:

mssql - github -- microsoft/mssql-server-linux

Description:

My understanding is that I can introduce my own way of serializing data by overriding serializeDate like this in a model:

protected function serializeDate(DateTimeInterface $date) {
     return "foo";
} 

However, when I do this the "foo" tries to get parsed as a date:

Psy Shell v0.8.9 (PHP 7.1.1 — cli) by Justin Hileman
>>> (string) \App\Models\Holiday::first()
PHP Fatal error:  Method App\Models\Holiday::__toString() must not throw an exception, caught InvalidArgumentException: A four digit year could not be found
Data missing in /Users/dsandber/arb/lms/vendor/psy/psysh/src/Psy/ExecutionLoop/Loop.php(90) : eval()'d code on line 0
PHP Stack trace:
...

Steps To Reproduce:

Just override serializeDate as above. Why is the result of my serializeDate trying to be parsed as a date/time?

@dsandber
Copy link
Author

This only happens if "updated_at" / "created_at" are listed in $dates -- shouldn't this be allowed?

@srmklive
Copy link
Contributor

Because the function expects the value being passed should be an instance of DateTime and should return a valid DateTime value. That's what i think is the behavior here.

@dsandber
Copy link
Author

That doesn't make a lot of sense -- if it takes a DateTime and returns a DateTime then what is the function supposed to do? Also, why does it work fine if updated_at and created_at are NOT listed in $dates?

@devcircus
Copy link
Contributor

By adding an attribute to the $dates array, you're telling Eloquent that you want it to be mutated to a Carbon date. You must start with a format that can be converted to Carbon and you end with a Carbon instance. If you want to have something other than a datish format, you shouldn't add it to the $dates array.

@dsandber
Copy link
Author

dsandber commented Jul 18, 2017

Sorry, I must be missing something. I thought "serializeDate" would control how a DateTimeInterface would get converted into JSON or array format.

If that is correct, then I don't understand how its return value should matter. If that is incorrect, then what exactly is serializeDate meant to do?

@dsandber
Copy link
Author

Seems like Laravel should have two functions, one that handles input from an arbitrary object and converts to a DateTimeInterface object, and another that takes a DateTimeInterface and converts to an arbitrary object for inclusion in serialized arrays/JSON.

No?

@themsaid
Copy link
Member

Return a correct date format and you're good to go.

Closing since it's not a bug, but feel free to continue the discussion if you want.

@dsandber
Copy link
Author

dsandber commented Jul 18, 2017

Yes, please help me understand. I thought the purpose of that function was only to determine how a date looks in an array or in JSON. If not that, what is the purpose?

I think there's a bug here -- either the function purpose is improperly documented, or the function is being invoked incorrectly, or something is amiss with how created_at and updated_at are handled.

@dsandber
Copy link
Author

Please explain why returning "zar" causes no problem if updated_at and created_at are not in $dates but other variables are? If the result of the function must be a DateTime or date-ish, then why is there no problem with it returning an arbitrary string in most cases?

@devcircus
Copy link
Contributor

Best way to see what is happening is go to Illuminate\Database\Eloquent\Concerns\HasAttributes and see exactly how it works.

Using $dates, calls "asDate" which calls "asDateTime". These are located in the same class(HasAttributes) as "serializeDate".

@dsandber
Copy link
Author

dsandber commented Jul 18, 2017

I'm sure the code explains the trouble I'm experiencing. But it doesn't tell me if the code does what is intended.

Can someone please explain what "serializeDate" is supposed to do if it's not to format a DateTime to the user's preferred JSON/array serialized format?

@dsandber
Copy link
Author

I looked at the code and am pretty sure there's a bug here. Here's what I think is happening:

If you add updated_at or created_at into $dates, then it ends up being there twice because array_merge will allow duplicates in.

Now that it's in $dates twice, it will be processed twice. The first time serializeDate is called, it will replace the Carbon with whatever it returns (such as a string). The second time it runs, there'll be an error because the string can't be converted to a DateTime.

So I believe I am correct that the ouput of serializeDate isn't supposed to matter (and according to the source-code, the result is supposed to be a string). The result is only mattering because it's being processed twice.

@dsandber
Copy link
Author

In short, it's an easy fix if we just make sure we don't allow duplicates when merging created_at and updated_at. Specifically, the problem is here:

    public function getDates()
    {
        $defaults = [static::CREATED_AT, static::UPDATED_AT];

        return $this->usesTimestamps() ? array_merge($this->dates, $defaults) : $this->dates;
    }

@dsandber
Copy link
Author

Can this please be re-opened? I I found a bug and detailed the cause of it.

@themsaid
Copy link
Member

This only happens if "updated_at" / "created_at" are listed in $dates

Don't include them, they're handled by default.

@dsandber
Copy link
Author

It's extremely counter-intuitive behavior that if you put created_at or updated_at in $dates that serializeDate will stop working.

Why is there so much resistance to designating this a bug?

@themsaid
Copy link
Member

@dsandber because I don't see a bug, it's just how it works, can you please share a real life scenario that you aren't able to accomplish because of the current behaviour? What are you trying to do?

@dsandber
Copy link
Author

@themsaid If a patient went to a doctor and told them "When I stand-up, I have chest pains" and the doctor said "Then don't stand up" I would say the doctor is treating the symptom rather than the root cause of the issue.

Similarly, the root cause of this issue is that updated_at and created_at are merged twice in the getDates() function, and I am trying to help fix that. I am not having any trouble with my code, but rather am trying to improve Laravel so that the next person doesn't have to spend hours on this issue.

As an example, here's another person who had to wrestle with this thing that can be fixed in five minutes:

https://laracasts.com/discuss/channels/laravel/formatting-dates-in-jsonapi-response-iso-8601-format

If putting 'created_at' or 'updated_at' in $dates is an error, then Laravel should give an error. If it is not an error, then things should work properly. The currently behavior of serializeDate breaking when $dates included 'created_at' is counter-intuitive behavior and as such qualifies as a bug.

@themsaid
Copy link
Member

Here you go #20193

@dsandber
Copy link
Author

Wonderful, thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants