orchidsoftware / platform

Orchid is a @laravel package that allows for rapid application development of back-office applications, admin/user panels, and dashboards.
https://orchid.software
MIT License
4.44k stars 652 forks source link

`withTrashed()` method for routes does not work as expected and still return 404 error for soft deleted models screen #2218

Closed czernika closed 2 years ago

czernika commented 2 years ago

Describe the bug

Hi!

I found out that withTrashed() route method is not working with screen - it will throw 404 error on soft deleted models (User model has SoftDeletes trait and deleted_at column is present)

To Reproduce Steps to reproduce the behavior:

  1. Create screen route (and appropriate Screen with necessary methods filled in)
// platform.php
Route::screen('users/{user}', UserShowScreen::class)
        ->name('platform.systems.users.show')
        ->withTrashed() // this one
        ->breadcrumbs(function (Trail $trail, $user) {
            return $trail
                ->parent('platform.systems.users')
                ->push($user->name, route('platform.systems.users.show', $user));
        });
  1. Softly delete User (for example with id 9)
  2. Go to /admin/users/9
  3. It will show 404 page

Expected behavior User screen with information about deleted user with id 9

Additional context If I change route to get id as a parameter and found soft deleted model by my own within Screen(User::withTrashed()->findOrFail($id)) it will work. So my guess the problem is with model binding of query method

// UserShowScreen.php
class UserShowScreen
{
    // Soft deleted User model does NOT binding 
    public function query(User $user): iterable
    {
        $user->load(['roles']);

        return compact('user');
    }
}

Proposed Solution

When I start digging deeper I found that if I change this lines of ScreenDependencyResolver

$model = $instance->resolveRouteBinding($value);

into

$routeBindingMethod = Route::current()->allowsTrashedBindings() && in_array(SoftDeletes::class, class_uses_recursive($instance))
        ? 'resolveSoftDeletableRouteBinding'
        : 'resolveRouteBinding';

$model = $instance->{$routeBindingMethod}($value);

withTrashed() method will show soft deleted models page when it's present and throw 404 when it's not.

These lines are basically the copy of Laravel way of resolving routes query dependencies for soft deleted models but I'm not entirely sure about this as a lack of knowledge about Laravel - that's why I am addressing this as an issue

tabuna commented 2 years ago

Hi @czernika Yes, we use a different way of defining parameters. And I'm not sure that we should fix it in this class. We have another way of binding that you can change in your screen:

use Orchid\Screen\Resolvers\RouteDependencyResolver;

/**
 * @param string $method
 * @param array  $httpQueryArguments
 *
 * @throws \Illuminate\Contracts\Container\BindingResolutionException
 * @throws \ReflectionException
 *
 * @return array
 */
protected function resolveDependencies(string $method, array $httpQueryArguments = []): array
{
    return app()->make(RouteDependencyResolver::class)->resolveScreen($this, $method, $httpQueryArguments);
}

It already uses the Laravel methods you refer to. Could you confirm the performance or lack of it when using this method?

czernika commented 2 years ago

@tabuna Thanks with this method it works as charm)