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
537 stars 58 forks source link

CamelCaseModel doesn't work with hidden/visible/dates fields #28

Closed cgwyllie closed 8 years ago

cgwyllie commented 8 years ago

Hi!

I've been attempting to use this trait in a Laravel 5.1 application, but have hit a problem with the $hidden property.

If I define a property to be hidden in camel case, it is ignored:

...
protected $hidden = [
    'lastLogin', // Ignored (model renders out lastLogin field)
    'is_active', // Hidden as expected
];
...

This appears to be the case for the $dates and $visible properties as well (and there could be others I guess).

Digging in to the core Eloquent model, it seems that getHidden could be overridden to correct this behaviour, but the withHidden method would not work properly either because it uses the $hidden array directly instead of through the accessor.

Are there any plans to support these properties in a consistent way?

This one came as a bit of a surprise and it has some security implications if the developer expects their fields to be hidden and they are not.

Thanks, Chris

cgwyllie commented 8 years ago

Here's the POC implementation I'm using for now for hidden and date fields:

    public function getHidden()
    {
        return array_map('snake_case', $this->hidden);
    }

    public function getDates() {
        $dates = parent::getDates();
        return array_map('snake_case', $dates);
    }
kirkbushell commented 8 years ago

Thanks for pointing this out. More overloading to be done! hehe

If you don't mind putting together a PR, I can get this in sooner.

cgwyllie commented 8 years ago

Sure! Working on putting it together, need to add some tests and decide what to do about the withHidden method, hopefully land this early next week for you.

(working on https://github.com/cgwyllie/eloquence/tree/feature/camel-case-hidden-and-date-fields)

cgwyllie commented 8 years ago

Hey, sorry for the delay, finally getting this together for you.

Think the CI build for PHP5.4 has freaked out, but passes on all other versions.

https://github.com/kirkbushell/eloquence/pull/30

Let me know if there's anything you want tidied up for merge :)

kirkbushell commented 8 years ago

Hi @cgwyllie. I've merged this in and the tests are all so passing, so you happy for me to close this now? I'll be doing a new released base on this code and a couple of other PRs.

cgwyllie commented 8 years ago

@kirkbushell Sure thing, happy to close.

Thanks :)