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

[8.x] Added onlyTrashed to routes as an addition to withTrashed #38462

Closed
wants to merge 2 commits into from
Closed

[8.x] Added onlyTrashed to routes as an addition to withTrashed #38462

wants to merge 2 commits into from

Conversation

SuperDJ
Copy link
Contributor

@SuperDJ SuperDJ commented Aug 19, 2021

Added onlyTrashed to routes as an addition to withTrashed.

Usage:

Route::post('/user/{user}', function (ImplicitBindingModel $user) {
    return $user;
})->middleware(['web'])->onlyTrashed();

@taylorotwell
Copy link
Member

No plans to add this right now.

@goodevilgenius
Copy link
Contributor

@taylorotwell @SuperDJ

This seems really useful for a trashed route.

Route::patch('/documents/{document}/trashed', function (Document $document) {
    $document->restore();

    return 'OK';
})->middleware(['web'])->onlyTrashed();

Route::delete('/documents/{document}/trashed', function (Document $document) {
    $document->forceDelete();

    return '';
})->middleware(['web'])->onlyTrashed();

withTrashed could return a not-deleted model, and restore would be pointless, and forceDelete would be dangerous.

@SuperDJ
Copy link
Contributor Author

SuperDJ commented Aug 20, 2021

@taylorotwell @goodevilgenius

and forceDelete would be dangerous

was the main reason for me of adding this.

This would also avoid checking if a model is soft deleted in every restore or forceDelete related route method.

public function destroy(int $invoice)
{
    $invoice = Invoice::onlyTrashed()->findOrFail( $invoice );
    $invoice->forceDelete()
}

@dennisprudlo
Copy link
Contributor

@SuperDJ You could create a middleware instead that checks if a parameter is an eloquent model then check if it's trashed.

Route::delete('/documents/{document}/trashed', function (Document $document) {
    $document->forceDelete();

    return '';
})->middleware(['web'])->onlyTrashed();

would change to

Route::delete('/documents/{document}/trashed', function (Document $document) {
    $document->forceDelete();

    return '';
})->middleware(['web', 'trashed']);

@tommy66374
Copy link

@dennisprudlo But how to write the middleware if there is many diff model with use softDelete

Route added ->onlyTrashed() is useful just like we can use Route::pattern('document', '[0-9]+');

otherwise we need add many extra code for check the model is deleted in many place

@dennisprudlo
Copy link
Contributor

dennisprudlo commented Sep 6, 2021

@tommy66374 Wouldn't this be sufficient?

class Trashed
{
    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle(Request $request, Closure $next)
    {
        foreach ($request->route()->parameters() as $resolved) {
            if ($resolved instanceof Model && !$resolved->trashed()) {
                abort(403);
            }
        }

        return $next($request);
    }
}

Then add that middleware to the routes

Route::get('/', [ XyzController::class, 'index' ])->middleware(\App\Http\Middleware\Trashed::class);

@martenkoetsier
Copy link
Contributor

@dennisprudlo This is probably not sufficient for nested controllers, e.g.

Route::patch('pages/{page}/contents/{content}/restore', [RestoreContentController::class, 'restore']);

because {page} is usually not softDeleted here.

@taylorotwell After reading about this new, great 'withTrashed' feature I was really hoping to see that there was already an accompanying 'onlyTrashed' method, mainly because of what @goodevilgenius already said: some actions should not be done on non-trashed models.

@dennisprudlo
Copy link
Contributor

@martenkoetsier Of course the middleware can be altered to add the desired functionality. This was just a simple example. In fact this middleware would throw an exception if you route model bind an eloquent model that doesn't use soft deletes. After $resolved instanceof Model you could check if the model uses the SoftDeletes trait to fix it.

If the page parameter is a model that uses soft deletes but you only want to check the last parameter in the list thats an easy change as well. An onlyTrashed() function on the route could behave in both ways too. It would either respond with 404 when the last model is not trashed or when any of the models bound are not trashed. Some have the former use case and some the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants