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

This Breaks Many To Many #17

Closed CWSpear closed 9 years ago

CWSpear commented 9 years ago

In Laravel 5, if I have a Many to Many relationship, such as:

public function roles()
{
    return $this->belongsToMany(Role::class);
}

and have the tables all set up correct, when I do this in a controller:

return User::with('roles')->all();

it will always return an empty array for roles if the User model has use CamelCaseModel in it. If I remove that line, it grabs the relationship as expected.

I am still investigating, but I suspect it has something to do with Laravel doing something specific with attribute starting with pivot_ in these cases, but it's not there cuz we camel cased it, so it can't connect the relationship...?

kirkbushell commented 9 years ago

Have you bound the eloquent model to the one provided in Eloquence? If so, then there could be a bug there - as it does some things with pivot models.

CWSpear commented 9 years ago

I actually have been using a base class, so every model extends a custom BaseModel that has use CamelCaseModel in it. Both the Role and User model extend BaseModel.

(I didn't create a BaseModel just so I didn't have to put use CamelCaseModel; in every model. I have a few common functions (such as a saveOrFail) and figured since I want camelCase everywhere, it'd be good to put it on the base class!)

kirkbushell commented 9 years ago

Nono, what I mean is - in order to provide proper, complete functionality for camel casing - you need to bind the eloquent model implementation to the one in Eloquence. If you haven't done that, I'm not sure how it would be converting.

That said - have you tried defining the full relationship manually to see if that gets around it?

CWSpear commented 9 years ago

Ok, sorry. I've been trying to chase this down for a while, and was messing with some stuff.

I didn't have your service provider in my config (I think that's what you're talking about...? If not, I am confused). I added it, but it didn't change anything.

When you say, "define the full relationship manually," what do you mean?

Do you mean this?

public function roles()
{
    return $this->belongsToMany(Role::class, 'role_user', 'userId', 'roleId');
}

cuz that causes an error:

Unknown column 'role_user.userId' in 'field list'
CWSpear commented 9 years ago

I think the problem is on lines 195-232 of BelongsToMany.php in Laravel

There is a call to cleanPivotAttributes that is looking for pivot_ in the attribute.

CWSpear commented 9 years ago

The $model->getAttributes() in the highlighted section returns the camelCase version... that's where the crux of it appears to be. Laravel is explicitly looking for snake_case there, and so it breaks down. I can tweak the code in cleanPivotAttributes to convert them to snake_case first and it works.

rogue780 commented 9 years ago

I am also having this problem.

rogue780 commented 9 years ago

I've confirmed that I've done the binding. @kirkbushell can you confirm this is correct in my config/app.php file?

'providers' => [ 'Barryvdh\Debugbar\ServiceProvider',

    /*
     * Laravel Framework Service Providers...
     */
    'Illuminate\Foundation\Providers\ArtisanServiceProvider',
    'Illuminate\Auth\AuthServiceProvider',
    'Illuminate\Bus\BusServiceProvider',
    'Illuminate\Cache\CacheServiceProvider',
    'Illuminate\Foundation\Providers\ConsoleSupportServiceProvider',
    'Illuminate\Routing\ControllerServiceProvider',
    'Illuminate\Cookie\CookieServiceProvider',
    'Illuminate\Database\DatabaseServiceProvider',
    'Illuminate\Encryption\EncryptionServiceProvider',
    'Illuminate\Filesystem\FilesystemServiceProvider',
    'Illuminate\Foundation\Providers\FoundationServiceProvider',
    'Illuminate\Hashing\HashServiceProvider',
    'Illuminate\Mail\MailServiceProvider',
    'Illuminate\Pagination\PaginationServiceProvider',
    'Illuminate\Pipeline\PipelineServiceProvider',
    'Illuminate\Queue\QueueServiceProvider',
    'Illuminate\Redis\RedisServiceProvider',
    'Illuminate\Auth\Passwords\PasswordResetServiceProvider',
    'Illuminate\Session\SessionServiceProvider',
    'Illuminate\Translation\TranslationServiceProvider',
    'Illuminate\Validation\ValidationServiceProvider',
    'Illuminate\View\ViewServiceProvider',
    'LucaDegasperi\OAuth2Server\Storage\FluentStorageServiceProvider',
    'LucaDegasperi\OAuth2Server\OAuth2ServerServiceProvider',

    /*
     * Application Service Providers...
     */
    'App\Providers\AppServiceProvider',
    'App\Providers\BusServiceProvider',
    'App\Providers\ConfigServiceProvider',
    'App\Providers\EventServiceProvider',
    'App\Providers\RouteServiceProvider',
    'Eloquence\EloquenceServiceProvider',

],
kirkbushell commented 9 years ago

@CWSpear sorry for belated response.

This line:

return $this->belongsToMany(Role::class, 'role_user', 'userId', 'roleId');

Is incorrect. Eloquence doesn't manage relationships (maybe a future patch), so the fields need to be in snake case.

Let me know if that resolves it for you.

CWSpear commented 9 years ago

No, it does not. By default, it uses user_id and role_id. This works if I remove use CamelCaseModel;.

I am fairly confident it has to do with the code I pointed out before in the core.

kirkbushell commented 9 years ago

Okay, good to know. It's not a core issue, it's definitely an issue with Eloquence as you've pointed out.

If you can drill down further, feel free to create a PR that resolves the issue and I'll merge it in pronto. I can release it at the same time as the new 1.3 features :)

CWSpear commented 9 years ago

Well, either side could make a change to fix it, but obviously we'd want to fix it on our end =)

But it does appear to be that the core is explicitly looking for pivot_. I'll look into making a PR, but I'm actually at ng-conf right now, so dunno when I'll get to it! If you're bored, the solution may have something to do with skipping camelCasing values that start with pivot_. That has a very low chance of BC breaks tho? i.e. if someone had a column that starts with pivot_?

kirkbushell commented 9 years ago

@CWSpear yeah that's exactly my suspicions as well.

kirkbushell commented 9 years ago

It's not a point of boredom, however - it's a point of time :)

CWSpear commented 9 years ago

Yeah, I know how it is =)

kirkbushell commented 9 years ago

@CWSpear feel free to try out the master branch now - I believe I've resolved it. As you suggested, the CamelCaseModel trait will now skip over pivot_ keys.

CWSpear commented 9 years ago

It is working for me. I do think it could break for people with columns that start with pivot_, but I'm okay with that!

Also, the items on the pivot table looks like they don't get camelCased, which is probably okay, too.

example:

{
    ...
    firstName: "Cameron",
    roles: [
        {
            id: 1,
            name: "Developer",
            createdAt: "2015-03-05 17:43:05",
            updatedAt: "2015-03-05 17:43:05",
            pivot: {
                user_id: 1,
                role_id: 1
            }
        }
    ]
}

Thanks!

kirkbushell commented 9 years ago

Hmmm, the pivot table ones should be - that's the whole idea of having the eloquent model rebound to your own implementation.