Laravel-Backpack / CRUD

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

[Bug] Column with type 'relationship_count' auto eager load relationship #4015

Closed devpreview closed 2 years ago

devpreview commented 2 years ago

Bug report

What I did

  $this->crud->addColumn([
    'name' => 'devices',
    'type' => 'relationship_count',
  ]);

What I expected to happen

Entries without auto eager loaded 'devices' relationship.

What happened

Entries with auto eager loaded 'devices' relationship.

What I've already tried to fix it

Hotfix:

class CrudPanel extends \Backpack\CRUD\app\Library\CrudPanel\CrudPanel
{
    protected function makeSureColumnHasEntity($column): array
    {
        return empty($column['type']) || !in_array($column['type'], [
            'relationship',
        ]) ? $column : parent::makeSureColumnHasEntity($column);
    }
}

Is it a bug in the latest version of Backpack?

After I run composer update backpack/crud the bug... is it still there? Yes

Backpack, Laravel, PHP, DB version

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

### PHP VERSION:
PHP 8.0.13 (cli) (built: Nov 22 2021 09:50:24) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.13, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.13, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v8.75.0@0bb91d3176357da232da69762a64b0e0a0988637

### BACKPACK VERSION:
4.1.61@4400187391eb5b73672c2b166a990cd737277409
welcome[bot] commented 2 years ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 2 years ago

@tabacitu all the relationships used inside columns are eager loaded. https://github.com/Laravel-Backpack/CRUD/blob/370531d2fd637b6ce1872f2732438853b96ebc66/src/app/Library/CrudPanel/Traits/Read.php#L81

What you think would be the best approach if this is something that needs to be fixed ? Is there a good reason to don't auto-eager load some relations ?

I recall we having some trouble with this column before I wonder? Not sure.

Would a key: eager_load in the columns to allow developer to take control of this ?

Let me know, Pedro

tabacitu commented 2 years ago

Hmm... why would you like to NOT eager-load this @devpreview ?

@pxpm I'm not sure about the importance or granularity of that configuration. Not sure we should even do it, but if we did do it... perhaps on the entire List Operation would be enough (for all columns)? I think we need to understand WHEN someone would NOT want their relationships eager-loaded...

And to answer your question - this column isn't supposed to exist. In its docs we provide a way to use the text column for the same thing, which would work much better. But yeah... it does exist, for convenience...

pxpm commented 2 years ago

Since this is inactive for long time, and I still can't find the reason to don't eagerLoad the data in this scenarios, I will be closing this.

Please feel free to continue the discussion, we will re-open if needed. 👍

Thanks