CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

Upload multiple -validation.

Open pxpm opened this issue 3 years ago β€’ 3 comments

SO ... I think it's time to put this behind us .. πŸ“†

First thing first, I usually use a custom validator for this field depending on my needs, that I proposed to be used as a general solution for it in: https://github.com/Laravel-Backpack/CRUD/pull/3951/files

I get it does not make sense adding such an amount of code for that "small" problem, but without it, how are we supposed to validate upload_multiple field ?

To put us on the same page, here is how I think IT SHOULD WORK when the field is required:

  • If you try to leave it empty - it will error
  • If you add one or more files they should be run through the validation rules for mimes etc.

Here goes my findings and proposal:

using 'upload_multiple.*' => 'required|file|mimes:pdf',
  • when creating: (correct)

image

  • when editing without messing with any files: (incorrect) image

  • adding the sometimes rule didn't help: 'upload_multiple.*' => 'sometimes|required|file|mimes:pdf' get the same error when editing without changing the files.

This happens when you don't change nothing in the request, an "empty" field will be submitted and raises the error that is required: image

I tried something to overcome this but I am blocked in some part that only you could unlock @tabacitu πŸ¦Έβ€β™‚οΈ !

  • I only setup the hidden field when ALL previous files are cleared, that way it 100% works, on create and update (the validation part).

With this solution the ability to delete the selected files is lost, because the mutator don't run if you don't send the field.

In this regard, I DON'T THINK a model mutator is the appropriate place to handle this file deletions. In 4.2 we introduced the field callbacks, that would fix this if developer setup the delete process in the field callback instead of in a mutator, because the callback would be run even if mutator don't, and we just want the cleared_files request input to know what files to delete.

I think it's time for a change here, let's do it!

I'v created https://github.com/Laravel-Backpack/CRUD/tree/browse-multiple-check with my proposed change. That and a doc. fix would make this field 100% work and validate as expected.

Did I made myself clear on the problem @tabacitu or you need me to rephrase something ?

Pedro

pxpm avatar Jan 11 '22 18:01 pxpm

I think I understand, thanks for the clear explanation @pxpm .

I only setup the hidden field when ALL previous files are cleared, that way it 100% works, on create and update (the validation part).

Hmm... how about if the rule is file|mimes:pdf? (no required). I believe in that case:

  • on create you can save the form without uploading anything (the input won't be sent).;
  • on update you can't submit the form at all (fails because we're sending an empty input);

I guess it's a bit clunky, but I've always thought that validation for upload_multiple needs to be done with two different FormRequest files (validations):

// on create
'upload_multiple.*' => 'required|file|mimes:pdf',
// on update
'upload_multiple.*' => 'file|mimes:pdf',

Am I correct that the above would solve the problem @pxpm ? If so, that can even be done in one Request, by doing:

$operation = \CRUD::getCurrentOperation();

if ($operation == 'create') {
    $upload_multiple_validation_rules = 'required|file|mimes:pdf';
} else {
    $upload_multiple_validation_rules = 'file|mimes:pdf';
}

If my concern above is true (that the hidden input is not an actual solution), perhaps what we can do if we want to make it easier for people, is to code two custom validation rules: required_on_create and required_on_update?

tabacitu avatar Jan 13 '22 12:01 tabacitu

PS. Created a PR from your branch here - https://github.com/Laravel-Backpack/CRUD/pull/4080 so that we can see the diff.

tabacitu avatar Jan 13 '22 12:01 tabacitu

Update from Pedro:

// on update
'upload_multiple.*' => 'file|mimes:pdf',

This won't work. On UPDATE if you delete all uploads, it still needs to be required. You're not allowed to delete all uploads and save with zero, that's the whole point.

So we need a different solution. Food for thought.

tabacitu avatar Jan 13 '22 15:01 tabacitu

Fixed by #5038 in v6

tabacitu avatar Apr 29 '23 10:04 tabacitu