kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
543 stars 58 forks source link

Model::create() fails when using CamelCaseModel trait and camelCased attribute keys #18

Closed rogue780 closed 9 years ago

kirkbushell commented 9 years ago

I use create all over the place with camelcased attribute keys - am not seeing this issue.

Could you elaborate some more and provide some code examples?

rogue780 commented 9 years ago

@kirkbushell check out my code here:

https://github.com/rogue780/camel-case-problem

I recreated the problem in a fresh Laravel 5 project.

To recreate it, set up your .env file for the database then run

composer install
php artisan migrate --seed
php artisan test:command

Then you should see two entries in your database. If it worked, one entry should have "snake_case!" in the some_long_string_name field, and the other should have "camelCase!" in the some_long_string_name field.

In my database, I only see the "snake_case!" one.

rogue780 commented 9 years ago

screen shot 2015-03-05 at 12 22 07 am

rogue780 commented 9 years ago

I should also mention that the entry that is created via the seed (not shown in my db) does work. So something is lost when a model is created from within a command? I'm not sure of the cause exactly.

rogue780 commented 9 years ago

Updated screenshot screen shot 2015-03-05 at 12 42 21 am

kirkbushell commented 9 years ago

This isn't broken - you've set it up incorrectly.

All you've done is included the camel case model within the namespace - but you haven't set it up on the class.

kirkbushell commented 9 years ago

Please see comments on the repository you added (that was very helpful, btw).

rogue780 commented 9 years ago

@kirkbushell I set it up in the model here: app/BaseModel.php:8 screen shot 2015-03-05 at 10 12 02 am

My test model then extends BaseModel to inherit the trait:

screen shot 2015-03-05 at 10 13 44 am

I accidentally left the namespace reference in the TestModel.

I've since made Test extend Model directly and then use CamelCaseModel in there directly and the problem still exists. I've pushed that change.

And, I'm sorry I'm kind of newish to the features in github. I don't see where you left comments.

rogue780 commented 9 years ago

Found the comments. I've addressed those in code, repushed, and the issue still persists.

kirkbushell commented 9 years ago

My apologies @rogue780 - I didn't see that base model.

Okay, let me have a bit of a play with the Eloquence tests and see if I can replicate.

kirkbushell commented 9 years ago

Have you tried setting $fillable using camelCase?

rogue780 commented 9 years ago

@kirkbushell I just changed it to use a sqlite db so it shouldn't require any setup on your end to run migrations and test if you're inclined to do so.

kirkbushell commented 9 years ago

@rogue780 not hat's okay, I'll do it within the eloquence project.

Digging a bit further I think I can see where the issue may lie, now. The fillable methods definitely aren't going through my methods, so - time for an update :)

rogue780 commented 9 years ago

@kirkbushell, setting $fillable with the camelCase attributes made it work. I'll accept that if that's the way it's supposed to work. I was expecting it to work with snake_case since I had understood the idea was with the traits I didn't really need to change attributes to camelCase in my model. Would it be possible to make $fillable work with either camelCase or snake_case?

rogue780 commented 9 years ago

@kirkbushell lol. I keep replying and then seeing your new reply after the fact. That'd be great if you could update that. Thanks so much for your patience.

kirkbushell commented 9 years ago

@rogue780 no, thank you. Really appreciate the time and effort you put in to reporting the issue, even when I wasn't seeing it :)

I've found the bug now anyway, so that'll be resolved very shortly. Thanks again!

kirkbushell commented 9 years ago

@rogue780 try the master branch now, I think I've fixed it. Was able to duplicate in the tests.

kirkbushell commented 9 years ago

@rogue780 you should leave fillable as snake_case - I get the feeling setting it as camelCase may create problems for Eloquent. So, leave fillable/guarded as snake_case, but you can now work with those attributes when dealing with your model from the outside as you would expect, using camelCase.

kirkbushell commented 9 years ago

@rogue780 changing my mind on this - implementing a solution that supported both was nasty. In short - if you're using camel cased models, you should now be using camel-cased fillable attribute arrays, as well.

rogue780 commented 9 years ago

@kirkbushell I'm running into an issue now where if I make my $fillable array have the camelCase attributes, then that gets passed to the database as camelCase instead of snake_case. Additionally, if I make the $fillable array snake_case, then any attributes that are coming in as camelCased and would need translation are ignored during a fill.

kirkbushell commented 9 years ago

Can you provide some code examples?

rogue780 commented 9 years ago

@kirkbushell yeah, I'm trying to update that project I shared with you, but things seem to be working there. I'm looking into my real project to see what the difference is now. It might be a ID10-T issue on my part. I'll update you either way.

kirkbushell commented 9 years ago

Are you running on 1.3.3? That's the latest now.

rogue780 commented 9 years ago

@kirkbushell yeah. I think I've found the problem. The model I am using the trait in has a setAttribute() method and it's preventing the camel case trait's setAttribute from being called.

kirkbushell commented 9 years ago

@rogue780 that'll do it :)

rogue780 commented 9 years ago

@kirkbushell yeah, that seemed to be it. I moved my code out to its own trait and then did some method aliasing to avoid collisions and then other crap to make it all play nice.

kirkbushell commented 9 years ago

You could still keep it in your own class - just either alias it (if it's on the same class) or call the parent method (iirc you had the trait on a parent "base model" class).

That'll be easier/cleaner than extracting to a trait and then aliasing.