Make requests modifiable

[promatik]

Related with #3783.

Backpack doesn't allow to modify the request on the Request class, because after the validation, the request is discarded.
This PR sends the request to the getStrippedSaveRequest function so it doesn't get discarded.

Since developers may want to modify the request, in order to add "static" fields like the author of a post, with author_id field, this field in not in the crud field list, so $this->getAllFieldNames() will not return it and getStrippedSaveRequest() will strip off that field.

To overcome this, I made getStrippedSaveRequest() not strip crud fields AND model fillable. This way, if author_id is fillable, even if it's not in the crud fields, it

[scrutinizer-notifier]

The inspection completed: No new issues

[tabacitu]

I think we should do this NOW, in 4.2. I especially like the fact that this adds the ability to modify the request in two different but important moments:

A) Before the validation

/**
 * Modify the input values
 *
 * @return void
 */
protected function prepareForValidation() {

    // get the input
    $input = array_map('trim', $this->all());

    // check newsletter
    if (!isset($input['newsletter'])) {
        $input['newsletter'] = false;
    }

    // replace old input with new input
    $this->replace($input);
}

B) After the validation

    /**
     * Get data to be validated from the request.
     *
     * @return array
     */
    public function validationData() {
        return array_merge(
            $this->all(),
            [
                'number' => preg_replace("/[^0-9]/", "", $this->number)
            ]
        );
    }

In addition to that:

• it's all functionality that Laravel offers and maintains, not Backpack (we just have to teach people to use it);
• it will replace the need to overwrite CrudController::store() and CrudController::update() for 90% of all use cases (I think people that overwrite them only do it to add inputs or remove inputs);
• it allows the dev to do the same thing as [4.2] add beforeSaving callback for fields #3928 , but define that custom logic not inside the Model (as a mutator), not inside the CrudController, but inside the FormRequest; it's not as convenient as in the CrudController... but it'll do the same thing (do the processing without modifying the Model);

@pxpm you should take a look at this too, I think this PR is much bigger than it seems. Small change, but with HUGE positive implications.

[pxpm]

@tabacitu I carefully read your and @promatik explanations on this.

I think we are opening strippedSaveRequest too much by allowing fillable fields that are not in the request.

Fillable fields means only that the field can be mass assigned in the Model, not that the developer wants that field to be processed in that particular form. Allowing all the fillable fields to be stored when not present in the form would allow malicious injection outside of admin control anyway.

• We can add a crud property shadow_fields or something like that, that would specifically allow determined fields to be stored when not present in the fields array.

public function setupCreateOperation() {
    $this->setOperationSetting('shadow_fields', ['user_id']);
}

This would allow the developer to then set the user_id in the request without exposing the field in the form pages as an hidden field, and backpack would not strip it ensuring that is developer intention to allow it to be stored, and not just a model definition (mass assignable).

Apart from that the prepareForValidation part is a big improvement and I agree with you here, 90% of the use cases are to add or remove some simple thing from request.

Let me know,
Pedro

[tabacitu]

Hey @pxpm

I think we are opening strippedSaveRequest too much by allowing fillable fields that are not in the request.

Sorry... I don't understand. How does this PR do that? From what I can tell it does one thing - run getStrippedSaveRequest($request) instead of the normal getStrippedSaveRequest() which basically did getStrippedSaveRequest(request()). The way I see it, if an input is added in FormRequest that does NOT have a field, it will still be taken out by getStrippedSaveRequest($request).

You CAN bypass that, by telling Backpack to not do getStrippedSaveRequest any more, but then again it you could do that before too. If you go out of your way to do your own stripping, we assume you know what you're doing.

Please explain @pxpm - something's off here.

---

That being said, I did think of a way this could be a breaking change, though. What if the dev has already changed his request, in the controller, in store() or update(), so something like:

public function store() {
    // change the request()
    return $this->traitStore();
}

Wouldn't this PR make Backpack ignore their changes? And prevent people from doing that? What do you think @promatik ?

[pxpm]

@tabacitu my concern is about return $request->only(array_merge($this->getAllFieldNames(), $this->model->getFillable()));. Previously it would only use getAllFieldNames because those were specifically added by developer, but now, since it also allow the fillables, if I don't add the field slug to the crud fields, but some malicious actor add the slug in the form via inspector, it would go through and is totally not developer desire. The slug is on fillable to be mass assignable in model, not to allow that specific crud page to interact with it.

That's why I proposed the "shadow fields" that are fields that developer don't want exposed in the form page, but want backpack requests to allow them to go through the saving process. I get that it's not a great idea, but is more to mark a standpoint than a solution for the problem itself.

Let me know if I didn't make myself clear.

[tabacitu]

OOooohhhhhh. NOW I get it! Thank you @pxpm . I agree with you 100% - I did NOT notice we're using getFillable(), I do NOT agree with allowing those by default. Good catch! 👏👏👏

[tabacitu]

Regarding my concern about the breaking change this entail, I've tested this and it's true. It IS a breaking change for the people who have changed the request in the Controller - in setup() or store() or update() or wherever else. For example people who have followed these instructions.

For those people, what we'll need to say is:

// if you've done anything like this in your CrudController to manipulate the request
//
use \Backpack\CRUD\app\Http\Controllers\Operations\CreateOperation { store as traitStore; }

public function store()
{
    $this->crud->setOperationSetting('saveAllInputsExcept', ['save_action']);
    $this->crud->getRequest()->request->add(['updated_by' => backpack_user()->id]);     
        
    return $this->traitStore();
}
// that will no longer work, because Backpack no longer saves the Illuminate\Http\Request
// but instead saves the FormRequest you've defined for validation;

// you can do THIS instead, to achieve the same thing from the CrudController
public function store()
{
    $this->crud->set('strippedRequest', function($request) {
        CRUD::getRequest()->request->add([
            'updated_by' => backpack_user()->id,
        ]);
        return CRUD::getRequest()->except(['_save_action', '_token', '_http_referrer']);
    });
        
    return $this->traitStore();
}

// but what we recommend you do is move that logic to the FormRequest that holds your
// validation too, that way everything that concerns the Request is in one place
// for example in your ProductCrudRequest you could do:

    protected function prepareForValidation()
    {
        // tell Backpack to save the validated inputs (instead of all inputs that have fields)
        \CRUD::set('create.strippedRequest', function ($request) {
            return $request->validated();
        });

        // add something to the request
        $this->request->add(['updated_by' => backpack_user()->id]);
    }

// then make sure inside rules() you have ALL the inputs you want saved, including updated_by;
// for those you want saved but can be null you can use the `nullable` validation rule;

Do you guys think this is too much of a breaking change? Or worth it?

[tabacitu]

Please reply in #3988 - that PR there is the same as this one, but without the fillable thing, and pointing to the refactored branch that has strippedRequest that accepts a closure.