Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] setAccessCondition()causing error on create operations #5410

Closed
hagealex opened this issue Dec 25, 2023 · 6 comments · Fixed by Laravel-Backpack/docs#533
Closed

[Bug] setAccessCondition()causing error on create operations #5410

hagealex opened this issue Dec 25, 2023 · 6 comments · Fixed by Laravel-Backpack/docs#533
Assignees
Labels
Possible Bug A bug that was reported but not confirmed yet. Priority: MUST Size: XS 1 hour

Comments

@hagealex
Copy link
Contributor

Bug report

What I did

I wanted to make us of the new setAccessCondition feature for my CRUD controllers. To test it I added this code into the setup() method of one of my controllers:

$this->crud->setAccessCondition(['update', 'delete'], function ($entry) {
    return $entry->id==1 ? true : false;});

What I expected to happen

I expect that the callback is only run for the specific operations.

What happened

When performing a create action I get this error:
grafik
grafik

I didn't expect the callback method to be run here because obviously, during the create operation, there is no $entry.

What I've already tried to fix it

??

Is it a bug in the latest version of Backpack?

Yes

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:

### LARAVEL VERSION:
10.32.1.0

### BACKPACK PACKAGE VERSIONS:
backpack/basset: 1.2.1
backpack/crud: 6.4.2
backpack/generators: v4.0.2
backpack/permissionmanager: 7.1.1
backpack/pro: 2.0.18
backpack/theme-coreuiv4: 1.1.1
Copy link

welcome bot commented Dec 25, 2023

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:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
  • Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

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

@karandatwani92
Copy link
Contributor

Hey @hagealex

The closure function is expected to run/check in every operation, helping us to hide the buttons from the current operation.

Notice that we are on list operation but controlling other operation's access+buttons(update & delete).

The first param is to specify which operation's access will be modified and this is working fine.

BUT you are right too! The documented code is expected to run flawlessly.

Hey @pxpm please check what's wrong here https://backpackforlaravel.com/docs/6.x/crud-operations#handling-access-to-operations

I tried to escape the error, but none worked!

  • if(is_object($entry))
  • if(!is_null($entry))
  • if($entry!==null)

@pxpm
Copy link
Contributor

pxpm commented Jan 2, 2024

Hey @hagealex thanks for the report.

I think the docs are misleading in this example, since what you are telling CRUD is that: run this access closure on EVERY operation, but actually you want to run it in the list operation (when you have entries in the table) or check if there is entry or not in the closure.

There is no $entry on every operation as you said, and Backpack will return null when no entry is present, in that case should be the developer the one to check if he wants to allow something with/without an entry.

We can either change the closure to deal with empty entries: is_null($entry) ? true : ($entry->id === 1 ? true : false) or add that access closure only to the list operation:

$this->crud->operation('list', function() {
        $this->crud->setAccessCondition(['update', 'delete'], function ($entry) {
            return $entry->id==1 ? true : false;
    });
});

I've added this example to the docs, as I think it's the most appropriate scenario.
Let me know if you have any doubts or the example is not clear enough.

Thanks @karandatwani92 for bringing this to my attention 🙏

Cheers 🥂

@hagealex
Copy link
Contributor Author

hagealex commented Jan 5, 2024

@pxpm and @karandatwani92 thank you for your support here! Now it's much more clear for me how to use this feature the right way!

@helesjunior
Copy link

helesjunior commented Oct 28, 2024

Hey guys @pxpm @karandatwani92 ,

I have the same problem, I include the code in the list operation and it still isn't working.

protected function setupListOperation()
    {

        $this->crud->operation('list', function() {
            $this->crud->setAccessCondition(['update', 'delete'], function ($entry) {
                return false;
            });
        });
...

Even making anything false it allows Update and Delete.

image

when I put it in setup(), it removes the icons, but if it is via URL it still allows the update

image

Could you help me?


PHP VERSION:

8.3.10

PHP EXTENSIONS:

Core, bcmath, calendar, ctype, date, filter, hash, iconv, json, SPL, pcre, random, readline, Reflection, session, standard, mysqlnd, tokenizer, zlib, libxml, dom, PDO, openssl, SimpleXML, xml, xmlreader, xmlwriter, ldap, curl, fileinfo, gd, gettext, gmp, intl, imap, mbstring, mysqli, odbc, Phar, pdo_mysql, PDO_ODBC, pdo_pgsql, pgsql, shmop, sqlsrv, pdo_sqlsrv, soap, sqlite3, xsl, zip, Zend OPcache

LARAVEL VERSION:

11.28.1.0

BACKPACK PACKAGE VERSIONS:

backpack/activity-log: 2.0.5
backpack/basset: 1.3.6
backpack/crud: 6.7.41
backpack/generators: v4.0.7
backpack/permissionmanager: 7.2.1
backpack/pro: 2.2.21
backpack/theme-coreuiv2: 1.2.6
backpack/theme-coreuiv4: 1.1.4
backpack/theme-tabler: 1.2.14

@pxpm
Copy link
Contributor

pxpm commented Oct 29, 2024

Hey @helesjunior

Since you don't actually need any condition, you can just do:

public function setupListOperation()
{
    CRUD::denyAccess(['update', 'delete']);
}

I would advise to don't do it that way, as you still leave the "update" and "delete" endpoints open like you stated in your previous comment.

A better approach would be to move that code to the setup() method and remove the operation condition:

public function setup()
{
    if(! backpack_user()->hasRole('admin') { // in this example, if the backpack user is not an admin, the access will be blocked for those operations, either in UI and in the endpoints. 
        $this->crud->denyAccess(['update', 'delete']);
    }
}

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Possible Bug A bug that was reported but not confirmed yet. Priority: MUST Size: XS 1 hour
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants