Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

Sep 22, 2025

22

OP

Level 2

Rate my update function

Is my method good I send the id in both the form and a hidden input to insure even if someone chnged it from the console gets error and I used a separate request file with custom messages and for you to not be confused products.create route is the table that has all the records and the route is inside a Rute::controller then Route::middleware to check for authintication and privilege. is it good and ready for production?

Route::put('/update-book/{id}', 'update')->name('products.update');

public function update(StoreProductRequest $request, $id)
{
    if ($id == $request->id) {
        try {
            $product = products::findOrFail($id);
            $product->update($request->only([
                'name',
                'category_id',
                'author',
                'price',
                'discounted_price',
                'description',
            ]));

            return redirect('products.create')->with('success', 'تم تعديل المنتج بنجاح');
        } catch (\Throwable $th) {
            return back()->with('error', 'حدثت مشكلة اثناء تعديل الكتاب');
        }
    } else {
        return back()->with('error', 'حدثت مشكلة اثناء تعديل الكتاب');
    }
}

Level 63

I would rewrite some parts of your code.

You can leverage of implicite model binding, so you don't need to try finding the model, it's done automatically.

public function update(StoreProductRequest $request, Product $product)

If the model isn't found, the ModelNotFoundException exception is triggered automatically. You can catch the Illuminate\Database\Eloquent\ModelNotFoundException exception and display a custom error view.

https://laravel.com/docs/12.x/errors#rendering-exceptions

If a validation fails in you form request, Laravel redirects automatically to the previous url ->back() with an error bag.

Add a gate to check if the authenticated is authorized to update the product.

Use $request->validated() instead of $request->only(). This way, if a data validation fails, you cannot save this data.

public function update(StoreProductRequest $request, Product $product)
{
	Gate::authorize('update', $product);

    $product->update($request->validated([
        'name',
        'category_id',
        'author',
        'price',
        'discounted_price',
        'description',
    ]));

    return redirect('products.show', compact('product')->with('success', 'تم تعديل المنتج بنجاح');
}

1 like

OP

Level 2

but if the user for some reason typed a different id in the form action would model binding bind the request to the wrong product ?

and I will use validated thanks for that.

1 like

Level 63

if the user for some reason typed a different id in the form action

I forgot something important, Ihave updated my previous comment, I have added a gate. Read my previous comment once again ;).

You should also redirect to the show product route and not to the create route.

So here are the modifications.

public function update(StoreProductRequest $request, Product $product)
{
	Gate::authorize('update', $product);

    $product->update($request->validated([
        'name',
        'category_id',
        'author',
        'price',
        'discounted_price',
        'description',
    ]));

    return redirect('products.show', compact('product')->with('success', 'تم تعديل المنتج بنجاح');
}

1 like

Level 63

Viewing your initial code, I recommend you to follow some courses about Laravel.

You have great series here on Laracast to learn all what you need to know about Laravel.

OP

Level 2

in laracast I finished 30 days to learn laravel and planning to go for deffinsive laravel next what do you think

1 like

Level 63

Is this project a personal project or a real project for a client ?

To be ready to code for a client, I recommend you to code several personal projects.

Furthermore you should write the different steps to update a product, for example :

display the product page
click on the edit button (the button is accessible only if the authenticated user can update this product)
display the product form (the product form is displayed only if the authenticated user can update this product, you should check this inside the edit function in the controller)
click on the save button
save the product (check once again if the authenticated user has the rights)
redirect to the product page

1 like

Level 104

Learn and implement the conventions we use as engineers to write readable, consistent and maintainable code. Use the PSR-12 coding standards as a starting point for your PHP code. Adopt the frameworks built-in tooling for Request handling and validation, Route-Model binding and much more.

Understand that "bad" users can target your vulnerabilities in your application code using form inputs, request URLs and other vectors. You should code learn to code defensively - validating everything sent into your application by the user is essential. Putting in place some form of authorization to prevent users interacting with resources that they do not own.

2 likes

OP

Level 2

@vincent15000 I usually use middleware to protect the route based on authintication and privilage this is my first client project so I am not very confident although I finished so far two personal projects one of them is Chirper that laravel upploaded its course

1 like

OP

Level 2

@tykus thanks that is a thing I really need to improve

1 like

Level 104

this is my first client project

In that case, I would strongly urge you to spent more time learning the basics!

Aside from the courses where applications are built using coding standards and TDD, this Laravel Security Through Examples course should be essential viewing.

2 likes

OP

Level 2

thanks again for the source

1 like

Level 63

If this is a client project, I would be very careful ... your code contains security issues. And you showed us only the update() function ... so what about the other parts of your code.

You should really learn the basics ... I mean the basics for coding with Laravel and also the basics for applying the best pratices and for coding securely.

1 like

OP

Level 2

@vincent15000 I updated the post to include the route that is inside a Controller Route and Middleware Route for grouping

1 like

Level 63

I don't understand what you mean.

OP

Level 2

@vincent15000 it is like this

Route::controller(ProductsController::class)->group(function () {

	Route::middleware('auth')->group(function () {

    	Route::put('/update-book/{product}', 'update')->name('products.update');

	});

});

I yet to add the second middleware

1 like

Level 63

This isn't sufficient to protect your route from not authorized users.

You don't explain the context of your application, but you have products. So I imagine there are admins to store new products and clients to buy the products.

With your current route, a client is authorized to update the products.

Unless you have added a policy to prevent from this ?

Level 80

@sekiro Why are you inventing your own URL structure? Laravel already supports resourceful URLs. Your update URL should look like:

Route::put('books/{book}');

You should also decide if your resource is actually called a “book” or a “product”.

Please have a read through the Laravel docs. As you’re missing (or deliberately ignoring) framework conventions.

2 likes

OP

Level 2

@vincent15000 I added a custom middleware that check a column in the users table that referes to his position a normal user, admin and so.

1 like

OP

Level 2

@martinbean thanks, I will focus more on the docs

1 like

Level 63

Without knowing more about your project and the needed rights, I don't know if it is sufficient or not for your application.

You seem to check only the roles, but it's recommended to check permissions rather than roles. I mean each role can have some permissions, for example a user can update only his own posts and comments.

The only case where I check roles is to prevent access to some routes really reserved for admins for example.

I suggest you to learn the needed roles and permissions with your client before coding, so that you know exactly which way you have to follow to implement the right permissions in your application.

Level 80

@sekiro It seems pretty pointless to pass the ID in the form as well as the URI. If someone changes the ID in the form action, then update the record (so long as they have permission to). If they want to mess about with identifiers, then they shouldn’t be surprised the identified record is updated. If you drop this check, then you can use route–model binding, instead of just passing an $id which could be anything since you have absolutely zero type-hinting on that route parameter now.

Wrapping the update in a try/catch statement is also redundant. If an exception does happen, you‘re catching it but not doing anything with it. So you’re never going to know if an exception has happened because it’s never going to get logged anywhere. Laravel has an exception handler. You don’t need to catch them.

You also don’t need to manually list the columns you want from the request if you’re using a form request. You can just retrieve the validated fields using $request->validated().

I also don’t know why you‘re redirecting to a “create” form when updating a product. You should either be redirecting back to the product’s edit form with a success message to say it’s been updated, or to something like the product index or details view.

So, taking the above into account, your update method could have looked like this:

public function update(StoreProductRequest $request, Product $product)
{
    $product->update($request->validated());

    return redirect()->route('products.show', compact('product'))->with('success', 'تم تعديل المنتج بنجاح');
}

…but you over-complicated things and ended up with far more lines of code than you needed.

1 like

OP

Level 2

@martinbean That sounds very reasonable, I’ll work with your advice. Thanks a lot for the guidance.

2 likes

Please or to participate in this conversation.