search

Laravel-Backpack / CRUD

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

Show operation addClause ignoring for SoftDeletes #5204

Closed skater4 closed 1 year ago

skater4 commented 1 year ago

Bug report

What I did

any crud controller, setup method $this->crud->addClause('whereIn', 'feed_id', [1,2,3]); (any condition)

What I expected to happen

show operation should return 404 if current entity doesnt have feed_id in 1,2,3

What happened

if model has SoftDeletes - it return entity page

Is it a bug in the latest version of Backpack?

5.6.1

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.2.6 (cli) (built: May 23 2023 09:42:25) (NTS) Copyright (c) The PHP Group Zend Engine v4.2.6, Copyright (c) Zend Technologies

LARAVEL VERSION:

v9.39.0@67e674709e1e7db14f304a871481f310822d68c5

BACKPACK PACKAGE VERSIONS:

backpack/crud: 5.6.1 backpack/generators: 3.3.16 backpack/permissionmanager: 6.0.17

welcome[bot] commented 1 year 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

tabacitu commented 1 year ago

Hi @skater4 ,

Indeed, the default behaviour is that... if SoftDeletes is present on the Model, the Show operation WILL show that entry and add a soft_deletes column.

You can change that behaviour in the config file for that operation. Just go to config/backpack/operations/show.php and change this to false:

    // If model has SoftDeletes, allow the admin to access the Show page for
    // soft deleted items & add a deleted_at column to ShowOperation?
    'softDeletes' => false,

Let me know if that doesn't work for you.

Cheers!

skater4 commented 1 year ago

Hi @skater4 ,

Indeed, the default behaviour is that... if SoftDeletes is present on the Model, the Show operation WILL show that entry and add a soft_deletes column.

You can change that behaviour in the config file for that operation. Just go to config/backpack/operations/show.php and change this to false:

    // If model has SoftDeletes, allow the admin to access the Show page for
    // soft deleted items & add a deleted_at column to ShowOperation?
    'softDeletes' => false,

Let me know if that doesn't work for you.

Cheers!

i think u didnt understand me. soft deletes are ok, BUTTT for example there are rows like this id feed_id 1 10 2 20 3 30 and i make in setup() $this->crud->addClause('whereIn', 'feed_id', [10,20]); so list operation will show only ids 1 and 2 so by logic show operation should also show only ids 1 and 2 and id = 3 return 404 but id = 3 returns entity page that not allow me do any access restrictions for models with SoftDeletes

tabacitu commented 1 year ago

Oh ok I think I understand now. Let me rephrase:

In your setup() method you did $this->crud->addClause('whereIn', 'feed_id', [10,20]);, and expected this restriction to apply to ALL operations. But that didn't happen. It applied to the ListOperation, but not to the ShowOperation.

That's true, ShowOperation doesn't use the same general $this->crud->query so your addClause() statements won't apply there. It just does a findOrFail(). But... that's the case with ALL entry-level operations. Your statement will not work on UpdateOperation or DeleteOperation either. To customize the query on entry-level operations, you can target those operations individually and prevent them from working:

        // instead of this
        $this->crud->addClause('whereIn', 'feed_id', [10,20]);

        // you should do something like this
        $this->crud->operation(['show', 'update'], function() {
            $id = $this->crud->getCurrentEntryId();
            // TODO: now check that the ID is allowed to be updated/edited, otherwise abort
        });

I do not recommend editing something general like the query in setup() directly. Because it applies to ALL operations, and not all operations will behave the same. It is much better to do your operation customizations either in the setupXxxOperation() methods, or in setup() but in closures that make it clear WHAT operation should have that customization, like instructed above.

The question now becomes... and I'm asking you @pxpm ... SHOULD WE change how we get the current entry... in order to use $this->crud->query? What do you think?

skater4 commented 1 year ago

Oh ok I think I understand now. Let me rephrase:

In your setup() method you did $this->crud->addClause('whereIn', 'feed_id', [10,20]);, and expected this restriction to apply to ALL operations. But that didn't happen. It applied to the ListOperation, but not to the ShowOperation.

That's true, ShowOperation doesn't use the same general $this->crud->query so your addClause() statements won't apply there. It just does a findOrFail(). But... that's the case with ALL entry-level operations. Your statement will not work on UpdateOperation or DeleteOperation either. To customize the query on entry-level operations, you can target those operations individually and prevent them from working:

        // instead of this
        $this->crud->addClause('whereIn', 'feed_id', [10,20]);

        // you should do something like this
        $this->crud->operation(['show', 'update'], function() {
            $id = $this->crud->getCurrentEntryId();
            // TODO: now check that the ID is allowed to be updated/edited, otherwise abort
        });

I do not recommend editing something general like the query in setup() directly. Because it applies to ALL operations, and not all operations will behave the same. It is much better to do your operation customizations either in the setupXxxOperation() methods, or in setup() but in closures that make it clear WHAT operation should have that customization, like instructed above.

The question now becomes... and I'm asking you @pxpm ... SHOULD WE change how we get the current entry... in order to use $this->crud->query? What do you think?

Thank you for reply. "But that didn't happen" - if model has soft deletes - yes, it works only for list, but not show. But if model DOESN'T have soft deletes - addClause works for ALL operations that breaks logic. I think soft deletes should not affect this logic

pxpm commented 1 year ago

The question now becomes... and I'm asking you @pxpm ... SHOULD WE change how we get the current entry... in order to use $this->crud->query? What do you think?

@tabacitu I've worked on this in: https://github.com/Laravel-Backpack/CRUD/pull/4937 https://github.com/Laravel-Backpack/CRUD/pull/4555

tabacitu commented 1 year ago

So what do you recommend we do @pxpm ?

skater4 commented 1 year ago

The question now becomes... and I'm asking you @pxpm ... SHOULD WE change how we get the current entry... in order to use $this->crud->query? What do you think?

@tabacitu I've worked on this in: #4937 #4555

so it decided like unimportant? because i was surprised when SoftDeletes affected other logic that doesnt matches SoftDeletes

pxpm commented 1 year ago

So what do you recommend we do @pxpm ?

At the moment can't do much, as it's clearly a breaking change. And we didn't "talked through" the PR and potential implications.

That PR came in a bad time and we didn't spent time trying to weight the cons/pros. And I didn't do a great job making my case. I think that's also because I know it's a HUGE change, and it needs a crime partner 👯‍♂️

For example one of the reasons I identified that we use the model directly instead of the query it's because the translatable package overwrites the find methods, to set the locale on model. https://github.com/Laravel-Backpack/CRUD/blob/ec304899873f08b3f0a064d6a3b130541000ce33/src/app/Models/Traits/SpatieTranslatable/HasTranslations.php#L181

When making the call to find() method via query builder we need to manually set the locale after retrieving the model (diff from the POC PR):

-            $this->data['entry'] = $this->crud->getModel()->withTrashed()->findOrFail($id);
+            $this->data['entry'] = $this->crud->query->withTrashed()->findOrFail($id);
+            $this->data['entry'] = $this->crud->setLocaleOnModel($this->data['entry']);

As far as I am aware that was the only reason for that. Can I be wrong? Yes.

People doing Model::find(x) anywhere in the application would still get the model with translations, we just switched to use the query instead of the model and added the locale "manually".

We can do this change specifically here in ShowOperation https://github.com/Laravel-Backpack/CRUD/blob/ec304899873f08b3f0a064d6a3b130541000ce33/src/app/Http/Controllers/Operations/ShowOperation.php#L80 I think this is the line that does not respect the query.

Is it a BC ? We may consider it yes, but it's not that huge PR 🤷

skater4 commented 1 year ago

Maybe u make some fix with backward compatibility? Possible defining deprecated old functionality

pxpm commented 1 year ago

I've submitted #5271 to address this issue in a non-breaking way. Let's continue any conversation over there.

@skater4 let me know if you think the solution would work for you or you may have a better one 🙏

skater4 commented 1 year ago

I've submitted #5271 to address this issue in a non-breaking way. Let's continue any conversation over there.

@skater4 let me know if you think the solution would work for you or you may have a better one pray

Yes, usePanelQuery parameter would be good for now. But any case SoftDeletes behavior still confusing and non logical