Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.08k stars 886 forks source link

[Bug] No way to fluently add a column, when the attribute name is the same as a method on that model (ex: a local scope) #3626

Closed tabacitu closed 3 years ago

tabacitu commented 3 years ago

Bug report

What I did

Inside a Comment model, I have both a hidden attribute, and a hidden local scope:

    /**
     * Scope a query to only hidden comments.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $query
     * @return \Illuminate\Database\Eloquent\Builder
     */
    public function hidden($query)
    {
        return $query->where('hidden', true);
    }

I've created a CRUD for it, and inside the ListOperation I've tried to add a column for the hidden attribute of each entry:

CRUD::column('hidden')->type('check');

What I expected to happen

The column to show up.

What happened

The ListOperation throws a nasty error when trying to load the page:

Too few arguments to function Backpack\CommentManager\Models\Comment::hidden(), 0 passed in /Users/tabacitu/Sites/backpackforlaravel/vendor/backpack/crud/src/app/Library/CrudPanel/CrudPanel.php on line 355 and exactly 1 expected

What happens is that the hidden() scope is called - instead of using the column in the db.

Rephrased:

It's a problem we've talked about before - if a method exists on the model, and you add a column/field for it, we assume it's a relationship. That's because not all relationship methods return a proper type, so we can't know for sure if it's a relationship - so we assume it is. Which sounded fine at the time.

At the time, we thought that... if the user gets into this predicament, they'd just define 'entity' => false and it would no longer try to see if it's a relationship. But. That doesn't work using the fluent syntax.

What I've already tried to fix it

CRUD::column('hidden')->entity(false)->type('check'); - but that doesn't work, still throws the same error because what happens is that the CRUD does add the column as normal (assumes relationship), and only afterwards tries to change entity to false.

What does work is defining that column using the array syntax:

        CRUD::addColumn([
            'name' => 'hidden',
            'type' => 'check',
            'entity' => false,
        ]);

That's because (using the array syntax) the first thing that gets added is with entity set to false.

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

### PHP VERSION:
PHP 7.4.16 (cli) (built: Mar  4 2021 20:52:51) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.16, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v8.31.0@2aa5c2488d25178ebc097052c7897a0e463ddc35

### BACKPACK VERSION:
4.1.37@f829c37501d2f015bc5a4c93ef6dbc4533354ba8
tabacitu commented 3 years ago

Questions:

  1. This can probably be avoided by actually creating the CrudColumn object only after the last definition. I remember we did something like that inside the CrudFilter object, where we automatically call apply(). Basically to call addColumn inside CrudColumn only after everything is defined. But... that involves refactoring how CrudColumn works, and we might break some other things (many other things), so I'm skeptical of doing this. Any other workarounds?

  2. Isn't there anything extra we can do before calling the method, to make sure it's a relationship? Something to determine if that method should be called or not?

  3. What can we do as a workaround for people that still want to use the fluent syntax and run into this problem? If we decide "it's a known issue" and we have a workaround for the array syntax... we should also have one for the fluent syntax. And right now we don't... Right now I'm thinking of like asking the user to define $relationships on the Model and if a Model has $relationships but the method isn't there... we don't call it. But that sounds pretty freakin' broad, and it has the potential of conflicting with the entity's own relationships methods/attributes.

Not sure what we can do here... Thoughts anyone?

pxpm commented 3 years ago

Shouldn't scopes be defined by: public function scopeXXXX($query) { ... } and then $query->xxxx()->first() ?

https://laravel.com/docs/8.x/eloquent#local-scopes

tabacitu commented 3 years ago

Haha - right 🤦‍♂️ me stoopid sometimes. Thanks @pxpm - there's no real issue here, sorry for wasting your time.