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

Carbon object in Entity::toArray() #15

Closed
anthonysterling opened this issue Apr 15, 2015 · 2 comments
Closed

Carbon object in Entity::toArray() #15

anthonysterling opened this issue Apr 15, 2015 · 2 comments

Comments

@anthonysterling
Copy link

Hello.

I'd just like to sound this out, and see if this is expected behaviour.

I have the following piece of code to find an authenticated user:-

        $user = $this->mapper
            ->where('email', $command->getEmail())
            ->first()
        ;

        if ( ! $user) {
            return false;
        }

        if ( ! password_verify($command->getPassword(), $user->password)) {
            return false;
        }

        if (password_needs_rehash($user->password, PASSWORD_DEFAULT)) {
            $user->password = password_hash($command->getPassword(), PASSWORD_DEFAULT);
            $this->mapper->store($user);
        }

        return $user->toArray();

If seems that when the password needs rehashing, and I subsequently update the user, I'm getting an instance of Carbon returned within the toArray call for the updated_at property. This means I get mixed results from this method, which of course isn't ideal.

I suspect it's related to the user table having aCURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP.

Is this something you would like me to look at? I assume it should just return an updated string, but I wanted to the implications of changing this behaviour.

Thanks!

@RemiCollin
Copy link
Member

You're right as in the update process, the timestamps attributes are set with Carbon::now() which the Illuminate Query translate into the correct datetime format, but the inverse is not true, as date attribute is hydrated as a string, which leads to this inconsistency.

Several ways to solve this :

  1. Cast the carbon object to string after the update/create query.
  2. Hydrate date attributes with instance of Carbon.
  3. Add a $dates attribute to the entity map to customize this behaviour. If the attribute is contained in the array, it will be hydrated with an instance of Carbon, if it's not with a string. Of course this raises the question of which behaviour to choose as a default for the timestamps. I'd tend to go on the Carbon side, but i'd like to make some benchmarks to see if it's not too costy in terms of hydration performance.

That's for the big picture.

Concerning the toArray() method, I think we can simply test if the attribute is an instance of carbon, and casting it to string if it's so. This will work whatever option we choose for the hydration behaviour. Feel free to PR it ;)

@RemiCollin
Copy link
Member

Fixed the casting in v2.1.7 release

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

2 participants