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

Use the validated method of FormRequest for store and update #3792

Closed
wants to merge 1 commit into from
Closed

Use the validated method of FormRequest for store and update #3792

wants to merge 1 commit into from

Conversation

rodrigofs
Copy link

@rodrigofs rodrigofs commented Jul 17, 2021

My suggestion would be to use the validated method of the FormRequest class, this method makes it possible to modify the data after validation and before persisting the data in the database, in addition to giving us the possibility to use the prepareForValidation method that is executed before validation, in case need to unmask data before validating.

To make use of this approach, it would be enough to have a data validation in a form request class and use $this->crud->enableApproachFormRequest() in the controller setup.

Sorry for my English from google translator. ^^

@welcome
Copy link

welcome bot commented Jul 17, 2021

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

  • if this constitutes a breaking change, it might take quite a while for this to get merged; we try to emulate the Laravel release cycle as much as possible, so developers can upgrade both software once; this means a new big release every ~6 months;
  • even if it's a non-breaking change, it might take a few days/weeks for the PR to get merged; unless it's a no-brainer, we like to have some community feedback on new features, before we merge them; this leads to higher-quality code, in the end; we learnt this the hard way :-)
  • not all PRs get merged; sometimes we just have to hold out new features, to keep the packages lean; sometimes we don't include features that only apply to niche use cases;
  • we're not perfect; if you think we're wrong, call us out on it; but in a kind way :-) we all make mistakes, best we learn from them and build better software together;

Thank you!

--
Justin Case
The Backpack Robot

@scrutinizer-notifier
Copy link

The inspection completed: 2 updated code elements

@pxpm
Copy link
Contributor

pxpm commented Sep 20, 2021

Hello @rodrigofs !

Sorry for taking so much time to get back to this. I think I've opened this issue like 2 or 3 times already, but I cannot figure out why we should do this. What was your concrete implementation that misses this functionality?

If possible I would like you to describe what you are trying to achieve, and where did backpack failed you.

Google translate does a pretty good job, I could understand what you wrote, but I could not figure out the scenario where I would be needing it. So feel free to include some code also if you feel that would help me to understand it.

Anyway I don't think we can make changes like this in 4.1 so I am postponing it to 4.2 (it's just right about the corner).

Let's get to the endpoint here and if we can do something better to help our users we will for sure pay close attention to it.

Thanks,
Pedro

@tabacitu
Copy link
Member

tabacitu commented Nov 27, 2021

I just thought of a syntax/name for this. What if instead of saveAllInputsExcept plus this method... we let the dev define the code itself, using a closure?

// instead of defining the stripping in config/backpack/crud.php like this:

            // Before saving the entry, how would you like the request to be stripped?
            // - false - ONLY save inputs that have fields (safest)
            // - [x, y, z] - save ALL inputs, EXCEPT the ones given in this array
            'saveAllInputsExcept' => false,
            // 'saveAllInputsExcept' => ['_token', '_method', '_http_referrer', '_current_tab', '_save_action'],

// we have a default way of saving (the same as now), so we save:
$request->only(CRUD::getAllFieldNames());

// but we allow the dev to overwrite that using a closure, 
// in his setupCreateOperation() or setupUpdateOperation()

// example 1: only save the validated inputs in the FormRequest 
CRUD::set('saved_input', function($request) {
    return $request->validated();
});

// example 2: strip out all inputs that begin with underscore (equivalent to the comment out line above)
CRUD::set('saved_input', function($request) {
    return $request->filter(function ($value, $key) {
        return ! Str::of($key)->beginsWith('_');
    });
});

This would not add another option on how the request is to be processed before saving... it would allow the dev complete customization of that process. Sure they can choose to use the validated request. But they can choose to do something else too, like add another input... or remove a different input... or change one input from something to something else (make it uppercase, turn it into a kebab... whatever).

I think I'm onto something here, and I'd love to code it... Basically the only thing that will change is getStrippedSaveRequest(), to something like:

public function getStrippedSaveRequest($request) 
{
    $requestProcessingClosure = $this->getOperationSetting('saved_input');
    if ($requestProcessingClosure && is_callable($requestProcessingClosure)) {
        return $requestProcessingClosure($request);
    }
    
    return $request->only($this->getAllFieldNames());
}

My only question is... what's a good name for that? 😅

CRUD::set('saved_input', function($request) {
    return $request->validated();
});

CRUD::set('saved_request', function($request) {
    return $request->validated();
});

CRUD::set('stripped_request', function($request) {
    return $request->validated();
});

CRUD::set('final_request', function($request) {
    return $request->validated();
});

CRUD::set('final_input', function($request) {
    return $request->validated();
});

CRUD::set('before_saving', function($request) {
    return $request->validated();
});

Personally I kind of prefer saved_input (though saved_request would technically be more accurate... actually the most accurate would be request_to_be_saved but that's ugly as hell).

@pxpm
Copy link
Contributor

pxpm commented Nov 30, 2021

@tabacitu I also think you have nailed it here in regards of what inputs to save. I don't have much to say about implementation here, so I will just try to suggest some names: guarded_request, safe_request, to_save, save_fields and I also like your suggestion stripped_request.

Best,
Pedro

tabacitu added a commit that referenced this pull request Dec 4, 2021
@tabacitu
Copy link
Member

tabacitu commented Dec 4, 2021

Great to hear that @pxpm 🙏

@rodrigofs , I've just finished the PR that does the above, so after we merge that and #3789 and launch 4.2 you'll be able to do this to achieve the same thing:

    /**
     * Before saving the entry, how would you like the request to be stripped?
     * - false - fall back to Backpack's default (ONLY save inputs that have fields)
     * - closure - process your own request (example removes all inputs that begin with underscode)
     *
     * @param  \Illuminate\Http\Request  $request
     * @return  array
     */
    'strippedRequest' => (function ($request) {
        return $request->validated();
    }),

Or inside your CrudController::setupCreateOperation() and CrudController::setupUpdateOperation() using

CRUD::set('strippedRequest', (function ($request) {
    return $request->validated();
}));

🎉🎉🎉

Thanks for the feedback, the PR and for the patience.
Cheers!

PS. I'll close this PR because #3987 fixes the same thing in a different way.

@tabacitu tabacitu closed this Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants