vanilophp / framework

The truly Laravel E-commerce Framework
https://vanilo.io
MIT License
816 stars 103 forks source link

withTrashed()-method for route model binding isn't working with Vanilo #155

Closed netzknecht closed 8 months ago

netzknecht commented 1 year ago

Hello,

when playing around with vanilophp framework, I've noticed that the withTrashed()-method for implicit model binding isn't working with vanilophp, like described in laravel doc's in chapter "Routing, Soft Deleted Models". Query logging still show's "[..] and users.deleted_at is null limit 1" for the query fired for implicit model binding, even the "withTrashed()"-method is given. It seems to me, that the "resolveSoftDeletableRouteBinding"-method isn't called if vanilophp framework is active, instead the "resolveRouteBinding"-method is called always.

Because I can't find the exact reason why and running out of time, I helped me out with customizing the resolution logic resolveRouteBinding()-method and use it in a trait for relevant routes eg. models for now.

<?php

namespace App\Models\Traits;

use Illuminate\Database\Eloquent\SoftDeletes;

trait WithTrashed {

    /**
     * Fixes withTrashed()-method for implicit model binding with vanilophp
     * @see https://github.com/vanilophp/framework/issues
     * @see https://laravel.com/docs/10.x/routing#implicit-soft-deleted-models
     *
     * @param mixed $value
     * @param string|null $field
     * @return \Illuminate\Database\Eloquent\Model|null
     */
    public function resolveRouteBinding($value, $field = null)
    {
        return in_array(SoftDeletes::class, class_uses_recursive($this))
            ? $this->resolveSoftDeletableRouteBinding($value, $field)
            : $this->resolveRouteBinding($value, $field);
    }

}
fulopattila122 commented 1 year ago

Hi @netzknecht,

First, it's not "vanilophp" framework; it's called "Vanilo". "vanilophp" is just the GitHub handle since "vanilo" was taken.

Second, Vanilo is not manipulating that part in any way, but I may help figure out the root cause. Questions:

  1. Does it only affect Vanilo models or other models your application defines?
  2. Is the controller part of your application, or is it happening with Vanilo Admin?
  3. How do you inject the model(s) to your controller? (code sample might help)

Having the answers for this might help to figure it out.

netzknecht commented 1 year ago

Hi @fulopattila122,

sorry for choosing the wrong term and many thanks for the effort for this project. I've also noticed, that the issue is related to route model binding generally, no matter if implicit or explicit, so I'll change the title of this topic.

  1. It affects only Vanilo models with SoftDeletes-trait.

  2. No, I've tested it without a controller and without Vanilo Admin installed.

  3. You should reproduce it with a soft deleted user model and the following route:

    Route::get('user/{user}', function(\Konekt\User\Models\User $user) {
    dd($user);
    })->withTrashed();

    In addition:

  4. It doesn't matter if the model \Konekt\User\Models\User or the contract \Konekt\User\Contracts\User is type-hinted.

  5. Tested in a fresh Laravel 10.10 "sails" environment + Sanctum 3.2 + Telescope 4.15 + Vanilo 3.1.

fulopattila122 commented 1 year ago

Got it, thanks for the investigation. I'll take a look next week. I suspect it could be related to Concord's model features.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 8 months ago

Closing because of being stalled for 3 days without activity.

fulopattila122 commented 8 months ago

I think I've found something: If you rename the parameter to, e.g., $uzer, then it works. Otherwise, it doesn't work - exactly as you described.

The route cause is the usage of Laravel's explicit Route binding:

Possible Solution 1 - Simple

  1. Rename the route parameter from user to something else
  2. Make sure to typehint in the route the actual class you need, most likely App\Models\User:
    Route::get('user/{uzer}', function(\App\Models\User $uzer) {
      dd($uzer);
    })->withTrashed();

Possible Solution 2

  1. Disable automatic route model binding in Concord in the config/concord.php file
    
    <?php

return [ 'register_route_models' => false, 'modules' => [ //... ] ];

2. When registering your own model, make sure to pass `false` as the third parameter (prevents explicit route model binding)
  ```php
// app/Providers/AppServiceProvider.php
class AppServiceProvider extends ServiceProvider
{
    public function boot(): void
    {
        $this->app->concord->registerModel(\Konekt\User\Contracts\User::class, \App\Models\User::class, false);
    }
}

This way, the route you posted will properly use the trashing mechanism without renaming the user parameter: image