CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

[Bug] eager loading doesn't work on relationship subfields when editing

Open kartomantas opened this issue 2 years ago • 12 comments

Bug report

What I did

I have a model Hotel. It hasMany HotelHotelAttributeValue. HotelHotelAttributeValue model has these relationships:

public function hotel()
{
    return $this->belongsTo(Hotel::class);
}

public function hotel_attribute()
{
    return $this->belongsTo(HotelAttribute::class);
}

public function hotel_attribute_value()
{
    return $this->belongsTo(HotelAttributeValue::class);
}

HotelAttribute also belongsTo HotelAttributeGroup and HotelAttributeType. So, in other words, the final chain is (from top to bottom): Hotel -> HotelHotelAttributeValue -> HotelAttribute -> HotelAttributeType / HotelAttributeGroup

In my HotelCrudController I have the following code:

$this->crud->addField([
    'label'    => __('admin.hotel_hotel_attribute_values'),
    'name'          => 'hotel_hotel_attribute_values',
    'type'          => 'relationship',
    'subfields'   => HotelHotelAttributeValue::fields()
]);

public static function fields()
{
return [
    [
        'attribute' => 'tree_title',
        'label'    => __('admin.hotel_attribute_id'),
        'name' => 'hotel_attribute_id',
        'type' => 'relationship',
        'ajax' => true,
        'minimum_input_length' => 0,
        'allows_null' => true,
        'data_source' => backpack_url('hotel-attributes/fetch/this')
    ],
    [
        'attribute' => 'tree_title',
        'label'    => __('admin.hotel_attribute_value_id'),
        'name' => 'hotel_attribute_value_id',
        'type' => 'relationship',
        'ajax' => true,
        'minimum_input_length' => 0,
        'allows_null' => true,
        'data_source' => backpack_url('hotel-attribute-values/fetch/this')
    ],
        [
            'label'    => __('admin.value'),
            'name' => 'value',
            'type' => 'text',
        ]
    ];
}

! IMPORTANT - tree_title includes HotelAttributeType and HotelAttributeGroup - that's why eager loading is important.

What I expected to happen

When editing Hotel, I expected backpack to eager load my HotelHotelAttributeValue -> HotelAttribute -> HotelAttributeType / HotelAttributeGroup.

What happened

It keeps creating same queries over and over again for each subfield. Hotels may have up to 30-50 attributes so it goes up to 200-300 queries creating 300-400 models.

What I've already tried to fix it

I tried to add a global scope on HotelHotelAttributeValue model to always eager load relationships (with). Also, I tried adding $this->crud->load('...') in my CRUD and also I tried to add $this->crud->entry->load('...') in edit method. Nothing works. DEBUGBAR shows that those queries are performed but then for each repeatable field seperate query is still made.

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 8.0.6 (cli) (built: May 4 2021 23:31:45) ( ZTS Visual C++ 2019 x64 ) Copyright (c) The PHP Group Zend Engine v4.0.6, Copyright (c) Zend Technologies

PHP Warning: Module "openssl" is already loaded in Unknown on line 0

LARAVEL VERSION:

v9.44.0@60808a7d9acd53461fd69634c08fc7e0a99fbf98

BACKPACK VERSION:

5.4.11@ffbadcdb6478822646600b2cd763490caa927155

kartomantas avatar Dec 19 '22 20:12 kartomantas

@kartomantas thanks for raising the issue. It's a little difficult to understand the chain to be honest. Can you please provide a screenshot/gif/video with the part that's loading too many queries? That should help us better understand this.

An image is worth 1000 words.

tabacitu avatar Jan 02 '23 17:01 tabacitu

@kartomantas thanks for raising the issue. It's a little difficult to understand the chain to be honest. Can you please provide a screenshot/gif/video with the part that's loading too many queries? That should help us better understand this.

An image is worth 1000 words.

Hello, attaching screenshots

first second

kartomantas avatar Jan 09 '23 09:01 kartomantas

Good news @kartomantas ! Pedro has fixed this in https://github.com/Laravel-Backpack/PRO/pull/117 - which we hope will get merged this week or the next. We'll be back with updates once it's done. Thanks for letting us know!

tabacitu avatar Jan 10 '23 13:01 tabacitu

Good news @kartomantas ! Pedro has fixed this in https://github.com/Laravel-Backpack/PRO/pull/117 - which we hope will get merged this week or the next. We'll be back with updates once it's done. Thanks for letting us know!

Thanks. Nevertheless, the link gives 404

kartomantas avatar Jan 10 '23 18:01 kartomantas

Would be nice to get an update on this, I am facing the same issue.

YoanHlebarov avatar Feb 17 '23 16:02 YoanHlebarov

@tabacitu, @pxpm any news on this issue?

prodixx avatar Mar 16 '23 13:03 prodixx

Sorry @prodixx totally missed your comment. It's not merged yet.

It probably requires a little bit of extra work, we are unsure about the solution we found and we'd rather spend a little bit more time on it and make sure we come up with a solution that is future proof and maintainable.

This is on the active development pipeline and is moving foward, I hope we can finish it before v6 🙏

Cheers

pxpm avatar Apr 04 '23 11:04 pxpm

@pxpm at least can you post what changes are in https://github.com/Laravel-Backpack/PRO/pull/117 ? the repo is private and we cannot see them.

i have a project that is going live in may and its a pain using product crud that has this bug. (assuming that v6 is not coming in 1-2 weeks 😄)

thanks

prodixx avatar Apr 07 '23 07:04 prodixx

Hey @prodixx I can do that, but that PR is not going to fix nothing by itself. We found out that PR is not enough to fix the reported problem.

We found out that the changes need to follow the https://github.com/Laravel-Backpack/CRUD/pull/4884 PR too.

Those CRUD changes are scary to do without a major version change. They are "non-breaking code", but there are a lot of ways developers could be using that part of code that we are not aware. And it would mess with the most critical operations (update/list).

You can give it a try to that crud branch, it should solve those query problems without the PRO pr, that is specific to some fields 👍

Let me know, Cheers

pxpm avatar Apr 07 '23 08:04 pxpm

thanks, ill take a look!

prodixx avatar Apr 08 '23 10:04 prodixx

Hey there,

I'm facing the same problem using the pro field. The page loads forever with around 50 MB of data and way too much requests. I've got a model with a n <> n relationship and pivot values. I've got around 180 values to load and to fill, but at the moment it's not doable through this field because it takes forever.

Astriel avatar Sep 06 '23 09:09 Astriel

Hi,

any news on this? Iam facing the same issue.

GregaUNK avatar Apr 21 '24 14:04 GregaUNK

This needs to be enabled in config/backpack/operations/update.php. I've added it as a step 3 of how to use: https://backpackforlaravel.com/docs/6.x/crud-operation-update#how-to-use-1

Thanks again for the report, we will probably make this true by default next major version 🙏

pxpm avatar Jul 25 '24 16:07 pxpm