[Bug] Update of entity can not be saved in tests with error "No query results for model [App\Models\User]"

[BernhardK91]

Bug report

What I did

1. UserCRUDController uses UpdateOperation trait.
2. Created a test that pushes the updated values to PUT users/[id].

Values f.e.:

[
    'first_name' => 'My first name',
    'last_name' => 'My last name',
]

What I expected to happen

The update should be solved.

What happened

Error: No query results for model [App\Models\User].

What I've already tried to fix it

It is fixed, by adding id parameter to the body params. That is not an issue in Backpack UI, as there is a hidden field with the id.

The CrudControllerTest is doing exactly what I was doing, but is not covering that issue. The reason is that it uses a custom controller for the testing. But that controller defines the routing methods by itself and is not using the UpdateOperation.

The update() method in UpdateOperation trait has the following schema:

public function update()
{
    //...
}

So the $id from the route is not forwarded to the method.

I think we should update that to use the $id from the route and not from the body paramater. For backwards compatibility perhaps both.

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.1.6 (cli) (built: May 11 2022 08:55:59) (ZTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.1.6, Copyright (c) Zend Technologies

### LARAVEL VERSION:
v9.20.0@c99868f1c9bf2f5d250993121838db905591864f

### BACKPACK VERSION:
5.1.4@4539c787a2e052d2e2e2c00188c38432dc2a7ea9

[welcome]

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

[pxpm]

Hello @BernhardK91

I think our implementation does not allow us to have the parameters in those functions or it would break the apps for developers using nested resources.

I've never went through the rabbit hole of the route system in Backpack, I know there were DOZENS of issues regarding this in the past 2016~2018. We found a general solution at that time and it works until now so I guess there is no plans to add that parameter back here without a major rethinking of the route system, sorry.

@tabacitu am I right on this or something slipped me here ?

[tabacitu]

I'm sorry guys, maybe I'm being particularly dumb today, but I don't understand the issue. @BernhardK91 can you please detail

Created a test that pushes the updated values to PUT users/[id].

Do you mean you created a unit test, that makes a PUT request to the users/{id} route? Can I see that test, please? Any chance you can rephrase the problem?

Thanks. Cheers!

[BernhardK91]

No worries. This is one of the PHPUnit tests:

public function test_if_admin_user_can_update_client(): void
{
    // Arrange
    $user = User::factory()->create([
        'is_admin' => true,
    ]);

    $clientToBeUpdated = Client::factory()->create([
        'name' => 'Old Client Name',
        'is_active' => false,
    ]);

    $updatedName = 'new first name';
    $updatedStatus = true;

    // Act
    $response = $this->actingAsBackpackUser($user)
        ->put(route('clients.update', $clientToBeUpdated->id), [
            'id' => $clientToBeUpdated->id,
            'name' => $updatedName,
            'is_active' => $updatedStatus,
        ]);

    // Assert
    $response->assertStatus(302)
        ->assertRedirect(route('clients.index'))
        ->assertSessionHasNoErrors();

    // ... database assertions removed for readability ...
}

This test works. But removing 'id' => $clientToBeUpdated->id, from the put request data results in the above described error.

To have to provide id in url and in body paramaters feels not right and intuitive for me as normally the url paramater should be enough.

[pxpm]

@tabacitu to sum up, our update() method does not accept any parameter like usually update does, update($id).
As far as I researched past threads that parameter was removed because when using nested routes smt/1/bla/3 the function would receive 2 parameters and fail.

I've been way from nested resources in Backpack since those times as I was having troubles myself with it and never looked back 👀

It's a bit we should revisit in future, to make sure we are doing things properly, if we do, fine, if not it will be a breaking change to fix for sure. (even if not broken we can write some docs/examples on it).

[tabacitu]

Ooooh I understand now, thank you @BernhardK91 & @pxpm . Excellent thinking with nested resources @pxpm .

Yes, for non-nested Updates it would probably be as simple as changing UpdateOperation:99 to take the parameter, instead of the input in the form... But because of nested resources... we should keep using the form input.

What we COULD do, but in the future (it would be a breaking change): we could change the update route... no point in it being update/$id since we don't use that, we could just point to update, that way it responds to both:

• GET - show the form
• PUT/POST - save the form

But until we can do anything like that, @BernhardK91 , I'm afraid we won't be changing anything, you'll have to keep the ID in two places, sorry 🙏👀😀 Then again... even in v6, I think this breaking change would be too big, for too little benefit... I'll sleep on it... But in principle I think we should just say "no" to doing anything about this, sorry... 🤷‍♂️

[BernhardK91]

Thank you @pxpm and @tabacitu for your detailed responses and the open discussion. I fully understand what the problem is.

Perhaps a "Testing" area in the documentation, where also this behavior is described, could avoid time for debugging for other users.

[tabacitu]

Totally, noted, thanks @BernhardK91 ! Not only that, we plan to add a feature to DevTools, to be able to generate tests for your CRUD. 🤯 , right? 😅

That being said, I've slept on it... it's weird with the double-ids, but... the breaking change really wouldn't be justified by this... I guess it just has to be a "known quick" for a while... 😔 Sorry @BernhardK91 .