Why require CrudRequest...?

[telkins]

Bug report

What I did:

I created a FormRequest like I would normally do with Laravel...via art make:request StoreFooRequest. I updated the CRUD controller's store() method to use this request.

What I expected to happen:

Once the validation on the form request passed, then the new data would be stored.

What happened:

Type error: Argument 1 passed to Backpack\CRUD\app\Http\Controllers\CrudController::storeCrud() must be an instance of Backpack\CRUD\app\Http\Requests\CrudRequest or null, instance of App\Http\Requests\StoreFooRequest given

What I've already tried to fix it:

Not much, other than to take a quick look at Backpack\CRUD\app\Http\Requests\CrudRequest to see what was so special about it.

Backpack, Laravel, PHP, DB version:

More or less the latest, across the board.

Extra:

Perhaps I've missed something, but Backpack\CRUD\app\Http\Requests\CrudRequest appears to add no value. It seems that Backpack\CRUD\app\Http\Controllers\CrudController::storeCrud() should require Illuminate\Foundation\Http\FormRequest (or some sufficient contract/interface).

I'd be happy to put together a quick PR, but I wanted to find out if this would be a waste of time before I did it and to perhaps figure out why this might not be a good idea.

[OliverZiegler]

You are probably right that there’s no extra in current crud request provided by backpack.

We try hard to enhance backpack without producing breaking changes. By requiring a special backpack crud request we can ensure that further updates (where we put extra value to the request class) will not break anything by currently requiring you to inject a subclass of our own request class.

The current overhead changing the superclass of your generated request class is just a small bit in opposite of changing every request class in every project when updating backpack (and requiring some extra functionality).

[telkins]

Thanks for the prompt reply. I understand what you're saying, but in 2017 it seems odd to choose such a solution that requires the "current overhead [of] changing the superclass of your generated request class". Introducing a contract that allows for any subclass of Illuminate\Foundation\Http\FormRequest would seem to be the "better" way to go.

Also, it's worth noting that if no request object is passed in, then the backpack code creates an instance of Illuminate\Foundation\Http\FormRequest rather than Backpack\CRUD\app\Http\Requests\CrudRequest:

    {
        $this->crud->hasAccessOrFail('create');

        // fallback to global request instance
        if (is_null($request)) {
            $request = \Request::instance();
        }

Interesting....

I would like to see a change, but I guess as long as there's the desire to keep open the ability to "enhance backpack without producing breaking changes", then we're stuck having to have an otherwise modular package further pervade other aspects of a solution.

I'm not entirely sure how I can utilize Backpack in this manner, however, without duplicating code, if I'm to be able to build my application as I had hoped. I have a simple Laravel-based API with a Vue-based SPA front-end. I would like to have the API as its own project, the SPA as its own, and the admin as its own. To do this would require having my models and certain "basic/fundamental" functionality in their own repository/package. Having FormRequests for my API that I could re-use for administrative purposes would work fine this way. But, now I have to introduce CrudRequest into my "basic/fundamental" functionality for no other reason that because it's required (for some potential future changes) by Backpack....or I have to repeat the same validation code between projects.

Not fun.

Alright...I guess that's enough rambling for me this morning. I'll accept the overhead/cost/imposition for now and maybe spend some time/thought to come up with a solution that still allows for the we-might-add-something-at-some-time mentality.

Thanks again. :-)

[OliverZiegler]

@tabacitu any opinions/suggestions on that?

[lloy0076]

I can't see any particular justification for having our own request class; in looking at support requests I'd say this (un)feature probably causes us more pain.

[telkins]

@lloy0076 I took a (quick) look through the issues and PRs before I submitted this issue and I've again looked through the issues since your post. You seem to imply that "this (un)feature probably causes us more pain" because you're seeing people encounter some sort of confusion or difficulty or something in the support requests.

May I ask, where are you seeing these support requests...?

[telkins]

@OliverZiegler Thanks for allowing for further discussion. Much appreciated. :-)

[lloy0076]

Oh, I tend to listen in on the Gitter conversations and fairly much anyone who tries to override the request classes gets gnarled by this. Whilst many people open support-like issues on Github (they really should be on StackOverflow), this one seems to not land that often here.

[OliverZiegler]

Well hearing this, I think we should remove this...

@tabacitu ?? :)

@telkins wanna create a PR for this? But Remember all the stubs that need to be changed accordingly ;)

[AurelDragut]

@telkins, pardon me, but i have a curiosity: why would you create an additional/other request file, i am assuming you used 'php artisan backpack:crud object' which generates the controller, model and request. what does the CRUDRequest is missing or not allowing?

[OliverZiegler]

@AurelDragut just suppose you have the backend (with backpack) and some kind of API. You could use the same request object for both. but as we currently need to subclass the CrudRequest class you either duplicate your code or need to use the backpack class for your API. As people sometimes try to split backend and API in different submodules (or even projects) and use packages this leads to bad atomicity of code. See also this part of @telkins comment:

I'm not entirely sure how I can utilize Backpack in this manner, however, without duplicating code, if I'm to be able to build my application as I had hoped. I have a simple Laravel-based API with a Vue-based SPA front-end. I would like to have the API as its own project, the SPA as its own, and the admin as its own. To do this would require having my models and certain "basic/fundamental" functionality in their own repository/package. Having FormRequests for my API that I could re-use for administrative purposes would work fine this way. But, now I have to introduce CrudRequest into my "basic/fundamental" functionality for no other reason that because it's required (for some potential future changes) by Backpack....or I have to repeat the same validation code between projects.

[AurelDragut]

@OliverZiegler, i keep on reading your comment and i am trying to figure out what i am missing (i haven't worked yet on so advanced projects). So we would need two different requests: one for administrative stuff and one for normal stuff. Never tried it yet, but couldn't we condition the use of a request or another based on something?

if (\Auth::user()->hasRole('admin') use App\Http\Requests\AdminRequest as StoreRequest
else use App\Http\Requests\NormalRequest as StoreRequest

both requests should have the instance of the same parent request.

I think it all started from the fact that @telkins tried to create a request using basic laravel way instead of the backpack's one (artisan make:request StoreFooRequest instead of artisan backpack:request/crud--request (i just saw we have two commands for creating requests))

[OliverZiegler]

@AurelDragut the issue is that currently we rely on the Backpack\CRUD\app\Http\Requests\CrudRequest without any benefit and we could just type hint the store and upgrade methods in the Base CrudController with the Laravel Illuminate\Foundation\Http\FormRequest class so no one needs the CrudRequest class here.

About your code: I'm not sure if switching use statements depending on some other code is good practice (and im even not sure if this even works...)

[AurelDragut]

@OliverZiegler, so it comes down to using Backpack\CRUD\app\Http\Requests\CrudRequest as we do now (which would allow you to add future updates as you said before) or using Illuminate\Foundation\Http\FormRequest (in which we would have full liberty).

[telkins]

@AurelDragut Hi. Good questions, but I think @OliverZiegler did a better job of answering than I could have. Still, I'll take a swing combining some of his words with my own. :-)

There's nothing inherently wrong with using or requiring Backpack\CRUD\app\Http\Requests\CrudRequest, but to do so without providing any benefit is questionable...at best.

Why would people have Illuminate\Foundation\Http\FormRequest instead of Backpack\CRUD\app\Http\Requests\CrudRequest...? A couple of reasons jump to mind, including the one I mentioned earlier which @OliverZiegler also noted again:

• First, one may have simply had a project and decided to add Backpack to it in order to enhance it. In this case, any existing form request classes would have to be changed..."without any benefit". It's all cost and no joy. :-)
• Second, one might have a project like the one I described, which would be composed of three different repositories, basically: an API, an admin tool (Backpack), and the "common bits" that each would use. The API and the admin tool would use the same models for those models that were common between them. Likewise, the API and the admin tool would use the same form requests for those form requests that were common between them. If an API allows for a consumer to add a user, then one might validate using a StoreUserRequest form request class. If the admin tool is allowed to do the same, then there's a good chance it would use the exact same logic. DRY dictates that one would try to reuse the same form request class. One can either make the common one Backpack\CRUD\app\Http\Requests\CrudRequest, which would be weird since the API and common repo/package wouldn't need to know anything about Backpack....or one would have multiple copies of the same functionality.

It's not the end of the world to have to make all form requests Backpack\CRUD\app\Http\Requests\CrudRequest classes....but without any benefit, it's just extra work.

So...it seems like the consensus is that if it's not providing any value/benefit, then let's not incur any costs on the Backpack users.

Hopefully that helps clarify a bit and/or give you a bit of a different angle/perspective. :-)

[OliverZiegler]

@AurelDragut that's exactly the point ;) @telkins good "sum up" ;)

Beside the suggestion of @telkins providing some kind of contract we require you to implement in your Illuminate\Foundation\Http\FormRequest subclass. But this contract would be empty at the moment...

In conclusion we have the following three options:

1. leave everything like it is (seems like the worst option)
2. switch to a contract users need to implement (this would make update-features easier but currently also has no benefit, also it seems like a breaking change)
3. switch to type hinting Illuminate\Foundation\Http\FormRequest(and edit stubs accordingly)

I would prefer number 3.

@tabacitu @lloy0076 should we do this?

[telkins]

@AurelDragut I'll jump in and try to answer since I'm already here. :-)

One can continue to use Backpack\CRUD\app\Http\Requests\CrudRequest. It's easy to do and for many/most, it will make zero difference going forward. If/when some value/benefit is added to the class, you'll get it for free without any bumps.

If, however, you choose/need to use Illuminate\Foundation\Http\FormRequest, then everything should go just as nicely as with the other option (if the proposed changes are made, of course). However, as soon as Backpack\CRUD\app\Http\Requests\CrudRequest is required (again), there will be bumps. The idea would be that there will now be value/benefit to such a requirement, however, which would presumably be worth the trouble. :-)

[telkins]

@OliverZiegler Thx. :-)

Btw, I am trying to pack up and get out of here. It's a bit late where I am and I need to get home. :-)

I agree with you and prefer number 3. I think number 2 might be good, but I haven't been able to find a good solution in my head. I also wouldn't mind doing it. I just need to find time. :-)

For now, though...I gotta run. I'll check back in later....

[AurelDragut]

First, one may have simply had a project and decided to add Backpack to it in order to enhance it. In this case, any existing form request classes would have to be changed..."without any benefit". It's all cost and no joy. :-)

Which is kinda obvious/normal (it's called adaptation), also we would do the same thing in the opposite way when switching from backpack requests to laravel requests.

Anyway I guess you know better which is the best choice for long term and I also think option 3 is the best despite the consequences on current projects.

[OliverZiegler]

@AurelDragut switching to Illuminate\Foundation\Http\FormRequest is no issue for any current project. As Backpack\CRUD\app\Http\Requests\CrudRequest extends the former, switching the type hints will not break anything.

[telkins]

Thx, @OliverZiegler. I logged in on my phone to say the exact same thing. :-)

[AurelDragut]

Yeah, sorry, i forgot about that.

[lloy0076]

Some people just don't want to use form requests, though....and one could equally argue that Illuminate\Http\Request is the one to aim at.

[OliverZiegler]

Well... as we build it currently like this:

if (is_null($request)) {
    $request = \Request::instance();
}

i would really just use Illuminate\Http\Request

see the linked PR #1129. this are the only changes required. No need to change docs, tutorials or any other projects but leaving space for people not wanting to use subclasses of Backpack\CRUD\app\Http\Requests\CrudRequest.

[telkins]

@lloy0076 It looks like the controller has the request already (from the constructor) and it doesn't appear to use anything from the FormRequest, either. So, going with Illuminate\Http\Request seems even better. And, relying on $this->request when no request is passed in to the store/update Crud methods might be a better fallback for when no request is passed in...? I'm not sure I understand the meaning/need/value of passing in any request (Request, FormRequest, or CrudRequest) to the store/update Crud methods...? Anyone know what the thought is behind that?

[OliverZiegler]

@telkins as you could manipulate the request data before calling parent:storeCrud($request) (in a subclass of CrudController) we need it as a parameter.

Default Laravel has the request injection in the route functions and we should stick to that and not introduce (bother) devs to use $this->request instead. (That also doesn't feel very laravely)

As I understand $this->request and the middleware closure in the __construct is only needed for the setup function to have session data and stuff like Auth available.

[telkins]

@OliverZiegler Thx for the explanation. Makes sense to me, more or less. :-)

[tabacitu]

Hi guys,

Long thread - took me a while to catch up.

I think you guys are right - we've had this for a few years and nothing new came up to be included in CrudRequest, I think it's safe to say nothing will probably come up in the near future. So I agree with what @telkins said, CrudRequest might not be necessary.

Another reason CrudRequest existed is because I noticed that (without it) people forgot to include this in their custom Requests:

    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        // only allow creates if the user is logged in
        return \Auth::check();
    }

I previously thought that without it, theoretically anyone could send a POST request to the /admin/entity/create URL, without being authenticated, and they would insert an item. update would work similarly. And I think that's actually how things worked back then. I usually have good reasons before doing stuff. I think this was before the admin middleware and automatic CSRF fields, in Backpack's father, Dick, and back when Laravel didn't check for the existence of the authorize() method. I don't know, doesn't really matter.

Please correct me if I'm wrong - I don't think this authentication verification is necessary anymore:

• We already make sure that the user is authenticated in the admin middleware. Laravel Form Request's authorize() method is now just to go deeper, check if the user is an admin, check if the user is not trying to edit himself, stuff like that.
• Laravel now checks that the authorize() method exists in the Request file. Otherwise it won't run.
• All forms have a CSRF field, so if the user logs out but submits the form afterwards, they get a "page expired" screen (just tested it).

So I think:

• it's super-safe now to allow people to use their own Form Request files, instead of extending CrudRequest;
• we don't actually need CrudRequest anymore; but we need to keep it in the package for backwards-compatibility;

So I'd say "option 3" is the winner: change CrudRequest with vanilla Request in CrudController. So exactly what @OliverZiegler 's PR #1129 does (thanks a lot).

[lloy0076]

I don't think we MUST enforce authentication at all, to be honest; it might behoove us to make it EASY to enforce authentication but that is different from enforcing it.

[tabacitu]

That being said, I wonder how you got to this error, @telkins : Type error: Argument 1 passed to Backpack\CRUD\app\Http\Controllers\CrudController::storeCrud() must be an instance of Backpack\CRUD\app\Http\Requests\CrudRequest or null, instance of App\Http\Requests\StoreFooRequest given

For the life of me, I couldn't get it, with zero changes in Backpack, before merging @OliverZiegler 's PR. Here's what I did:

• created a new Form Request using php artisan make:request TestTagRequest
• checked that that request extends Illuminate\Foundation\Http\FormRequest instead of \Backpack\CRUD\app\Http\Requests\CrudRequest; it does;
• placed my rules() in the request file;
• in TagCrudController I used the new TestTagRequest instead of the TagRequest generated by the generator:

-use App\Http\Requests\TagRequest as StoreRequest;
+use App\Http\Requests\TestTagRequest as StoreRequest;
-use App\Http\Requests\TagRequest as UpdateRequest;
+use App\Http\Requests\TestTagRequest as UpdateRequest;

My result - it works. No error...

Did you do anything differently?

[AurelDragut]

I think that is a bit weird, @tabacitu, CrudController uses backpack request and so should your TagCrudController which extends the CrudController, maybe there is a different setting about errors your config ignores.

[telkins]

@tabacitu Sorry for taking so long to reply. I was quite sick when you posted your last comment and I think it just got lost in the rubble, so to speak. :-)

Anyway, I have to agree with @AurelDragut ...."that is a bit weird". Something's not right.

When Backpack\CRUD\app\Http\Controllers\CrudController::storeCrud() is called, it requires . an instance of Backpack\CRUD\app\Http\Requests\CrudRequest. So, if you pass an instance of Illuminate\Foundation\Http\FormRequest, then PHP should react as described here: http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration

The method, storeCrud() has the following declaration:

use Backpack\CRUD\app\Http\Requests\CrudRequest as StoreRequest;

...

    public function storeCrud(StoreRequest $request = null)
    {
        ...

So, if storeCrud() receives an instance of Illuminate\Foundation\Http\FormRequest "then an error is generated: in PHP 5, this will be a recoverable fatal error, while PHP 7 will throw a TypeError exception."

I can redo everything, but I'm 99.9% certain that this is simply how it works, so if you're not seeing it, then my guess is that the test is flawed somehow.

P.S. I hope I'm not coming across rudely here in "black and white". I said, "99.9% certain" because I've been proven wrong so many times in my life that I know it's always possible. I'm confident and certain, but this wouldn't be the last time I've been wrong about something. ;-) Anyway, the tone of this post is meant to be positive and friendly. I hope you've taken it that way. :-)

[lloy0076]

@telkins @tabacitu I'm tempted to hit this with a big hammer and refer to #1129 but I'm not sure if the conversation hasn't stopped.

Nor am I sure what conclusion has been come to 😿

[tabacitu]

@telkins thank you for the kind reply. Don't worry about coming across rude, you certainly didn't. Even if you had, tough skin, been doing this for a while :-)

What I was trying to explain (and failed) is that switching to Illuminate\Foundation\Http\FormRequest should not be a breaking change for any current project. As Backpack\CRUD\app\Http\Requests\CrudRequest extends Illuminate\Foundation\Http\FormRequest, switching the type hints will not break anything. Just tried it again. It does not. So I've just merged #1129. If I made a bubu, I'm very very sorry. But I'm pretty certain about this - tried it over and over again, on multiple systems.

Cheers! Thanks for the great collaboration here.

[telkins]

@lloy0076 Looks like the conversation's come to an end...finally. :-)

@tabacitu Thanks. Glad I didn't come across rude. And thanks for the great effort...! :-)