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

Allow for Eloquent model attributes to be type juggled (casted). #4948

Closed
wants to merge 3 commits into from
Closed

Allow for Eloquent model attributes to be type juggled (casted). #4948

wants to merge 3 commits into from

Conversation

daylerees
Copy link
Contributor

Right now if you retrieve an attribute from an Eloquent model, it may not be in the right format. For example, a boolean value in the database will be returned as an integer because Eloquent has no knowledge about the table schema.

This PR solves this problem by providing the $cast array where you can specify default set/get attribute types for Eloquent model attributes. Here's an example.

<?php

class Book extends Eloquent
{
    protected $cast = [
        'name'          => 'string',
        'read'          => 'bool',
        'read_when'     => 'date',
        'price'         => 'double'
    ];
}

Now all of the attributes provided as keys in the above array, are set and retrieved in the appropriate type. As you can see, dates have been added to this $cast array as a replacement for $dates due to their similarity. This change has been made backwards compatible, and will respect the original $dates array to allow for hotfix.

@mikehayesuk
Copy link
Contributor

Awesome! I have a lot of setters and getters converting integers to booleans at the moment. Hopefully this makes its way in, good work.

@martindilling
Copy link

Great idea, this is a lot more readable than a lot of accessors ;)

@danharper
Copy link
Contributor

Installing the php-mysqlnd extension will correctly cast data from MySQL to PHP variables, instead of everything being a string.

@Garbee
Copy link
Contributor

Garbee commented Jul 2, 2014

@danharper Not everyone has enough control to setup PHP with the myslqnd driver. 😦

@daylerees
Copy link
Contributor Author

@danharper thanks :) I suppose this also allows you to override that without the need for a bunch of accessors. Almost everything in this PR can be done already with accessors, but it's a lot more code.

@milesj
Copy link

milesj commented Jul 2, 2014

Why is this necessary? This defeats the whole point of a dynamic type system.

It doesn't matter what is returned from the database. As '1' or 1 evaluate to true.

If anything, this should be handled automatically on the DBAL level, not the model level.

@daylerees
Copy link
Contributor Author

Say you return an Eloquent model instance as the result of a route, forming a simple JSON API. All of the boolean values in the payload will be integers. Some floats might be strings. This ensures consistency over the types. You'd need either the extension that Dan mentioned, or a presenter to have the data in the correct format.

@Ilyes512
Copy link
Contributor

Ilyes512 commented Jul 2, 2014

This is great! And as this doesnt change things for people that don't want to use it, makes it even nicer!

@marcorivm
Copy link

Looks great!

@crynobone
Copy link
Member

Should it cast to type if the value is NULL?

@milesj
Copy link

milesj commented Jul 3, 2014

@daylerees - JavaScript is also dynamic. Unless an external API is consuming the data, it's irrelevant.

@Spir
Copy link

Spir commented Jul 3, 2014

@milesj just for external API this will be very helpful! I had issue in the past with boolean not being cast to bool and that broke everything. Thanks a lot for this code @daylerees

@daylerees
Copy link
Contributor Author

@crynobone good call man. I've just sent a patch that should allow nulls to be respected, for testing. I may change it to remove the outer value checks, or perhaps the other way around, still thinking about it.

I'll squash these commits when people are happy with it.

@stayallive
Copy link
Contributor

This would come in very handy indeed! Thanks @daylerees 👍

@HiroKws
Copy link

HiroKws commented Jul 3, 2014

Gooooood 👍 :)

@mukaken
Copy link

mukaken commented Jul 3, 2014

👍

1 similar comment
@ytake
Copy link
Contributor

ytake commented Jul 3, 2014

👍

@localdisk
Copy link

Good! 👍

@gotodeploy
Copy link

+1

@GrahamCampbell
Copy link
Member

Cool. :)

@Anahkiasen
Copy link
Contributor

👍

1 similar comment
@takamichi
Copy link

👍

@KevinCreel
Copy link

+1 This would also be huge for us stuck using MS SQL databases. Running a select * on a table with unsupported data types without casting them makes PDO silently fail without trigger any exceptions. Selecting columns by name ends up return false which obviously isn't very helpful.

*
* @return array
*/
public function getDates()
public function getCasts()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be done once in the boot method? Just augment the casts property and be done with it. Do we really have to spin through all this for every single attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was replicating what was there for the getDates() but I'll take a look at the possibility of using boot. :)

@JosephSilber
Copy link
Member

Also, we don't need this as an accessor. We should mutate the attributes in newFromBuilder as well as setAttribute (which you have already done). Once we've got that, there's no need for casting accessors.

{
if ($value)
{
$value = $this->fromDateTime($value);
if ($casts[$key] === 'date')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already handled in castAttribute, so no need to do it here again.

@daylerees
Copy link
Contributor Author

@JosephSilber Fair comments. :) I see what you mean about mutating in newFromBuilder to essentially mutate on hydration/setting. Will give this a go.

Thanks buddy!

@JosephSilber
Copy link
Member

@daylerees Instead of flooding your PR with a gazillion more comments, I decided to make a sample implementation of what I was thinking. See here: github.com/JosephSilber/framework.

That change also neatly implements the dates for deleted_at, which was lost when soft deleting moved to the trait.

Let me know what you think.

JosephSilber referenced this pull request in JosephSilber/framework Jul 9, 2014
@jasonlewis
Copy link
Contributor

I like it but other tools such as Fractal already solve the problem of data being returned as JSON and not being of the correct type. That takes it away from the model and also gives you more control over the data returned.

@JosephSilber
Copy link
Member

@jasonlewis I don't see how taking this away from the model is a net benefit. IMO this falls squarely within the domain of the model.

The model's job is to return the data as found in the DB, not some poor bastardized version of it. The fact that it is PDO's fault doesn't make it any better; as it stands, the model does not return the data as it is in the DB, and that responsibility lies solely on its shoulders.

@milesj
Copy link

milesj commented Jul 11, 2014

@JosephSilber Casting is not the model's responsibility, it's the DBAL. Whether it be the repository, or data mapper, the model shouldn't be casting.

@JosephSilber
Copy link
Member

@milesj that might be true in a purer architecture, but Eloquent is an Active Record implementation and is therefore both the judge and the executioner.

@milesj
Copy link

milesj commented Jul 12, 2014

Eloquent still plugs into the Database layer though, which is completely separate.

@hipsterjazzbo
Copy link

FWIW, I agree with @milesj. If this is going to go into Laravel, it shouldn't be done in Eloquent, this should be done lower down in the query builder or something.

@dalabarge
Copy link
Contributor

In case anyone is interested, the Esensi/Emerson Media team just released a new 0.3.3 version of the esensi/model package which now includes a type casting/juggling trait which can be used for much more than just casting to internal types and dates. @daylerees, thanks for this PR for nudging us to finally open source our trait solution!

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

Successfully merging this pull request may close these issues.