Framework: [5.1] [5.2] Required file rules dont work when file size greater than max_file_upload size

[laurencei]

This issue has come up before: https://github.com/laravel/framework/issues/4467 & https://github.com/laravel/framework/issues/4226

@GrahamCampbell said in #4467 that there is an existing pull that deals with this - but it is not fixed - it is still broken - and I cant find any PR that seems to deal with this?

The problem is simple:

1. Upload file greater than max_file_upload (but less than post_max_size)
2. PHP will send the request to Laravel, but has cleared the file because it is larger than max_file_upload
3. Laravel does a required check - sees the file is not there, and reports "The file is required".

Obvious this is very confusing to users - because they did attach a file - but the website is telling them they didnt.

The general solution would be to add a simple check to see if there is an error on the $_FILES global - which would indicate there was a file attached. But then you run into a new problem - if the file is required, but it is not actually there - then you cant let required pass - it must fail.

So I think the solution is to return a custom message for file validations - with some checks as to why the required failed?

I'm happy to do the PR myself - but I'm looking for some guidance/direction on how we can solve this problem...

[GrahamCampbell]

Duplicate of #8202?

[laurencei]

@GrahamCampbell - no - separate issue.

8202 is when post > post_max_size

This is when file > max_file_upload - which is much more common

[laurencei]

I've done a picture to try to illustrate the problem

[laurencei]

I've been digging around the validation rules - and I have an idea how we might solve this, plus improve the validation rules at the same time.

The validation rule required for files is actually a bit strange:

elseif ($value instanceof File)
{
    return (string) $value->getPath() != '';
}

The _only_ way a $value can be an instanceOf File is if the user originally sent a file. So by definition - this means the required rule has been meet, and should always pass.

The problem is that if the file upload had an error, then the ->getPath() returns blank, and the required rule is now false - giving an incorrect error "File is required" - when it should be more like "There was an error uploading".

I propose we remove that elseif statement from the required rules.

And we add a file validation rule. We have string and array validation rules. We also have an image rule. But we dont have a general file validation rule? If you want to allow any file, it is difficult without setting mimes - but what if you want to allow any mime?

The file rule could be something like this (needs refactoring - but you get the idea):

protected function validateFile($attribute, $value, $parameters)
{
     if ( ! $this->isAValidFileInstance($value)) {
             return false;
     }

     $minFileSize = isset($parameters[0]) ? $parameters[0] : 1;

     if ($this->getSize($attribute, $value) < $minFileSize) {
             return false;
     }

     $maxFileSize = isset($parameters[1]) ? $parameters[1] : ini_get('max_file_upload');

     if ($this->getSize($attribute, $value) > $maxFileSize) {
              return false;
     }

     return true;
}

Then in your validation rules you just have to do

'file' => 'required|file:1,10000'

or you can do

'file' => 'required|file'

Which defaults to min size of 1 (because you shouldnt accept filesizes of 0 bytes), and max file size of whatever your PHP max_file_upload config is (which is your limit no matter what).

and you can easily combine it with image and mime rules

'file' => 'required|file:1,10000|mime:pdf,doc,txt'
'image' => 'required|file:1,10000|image'

or like this:

'file' => 'required|file|mime:pdf,doc,txt'
'image' => 'required|file|image'

[arrilot]

Pretty decent imo, good job, but i see some problems too
1) it can conflict with min and max rules and will be a bc break if they are ignored.
2) users will end up writting this rule on every validated file field because all problems you wrote about start again otherwise.

What about executing all those check that you described in the previous message automatically if the field is file and has ANY rule? Neither new rule is needed, nor min and max broken then.

It looks hard but not impossible

[laurencei]

How is this a breaking change?

You can just continue to use your current validation rules using min and max and nothing has changed.

But in the 5.1 upgrade docs - I am assuming we would _suggest_ people update their file validations to use the new rule, as it provides better file and error handling in some situations - but there is nothing forcing them to do this.

People can also use the rule like this if they _really_ want to - it would still work:

'file' => 'required|file|min:1|max:10000'

[laurencei]

@taylorotwell - if I do this a PR to the 5.1 branch - is this something your likely to accept?

I'm thinking I can switch PR https://github.com/laravel/framework/pull/8215 to the 5.1 branch as well - then as part of the overall 5.1 upgrade docs we can mention there is an improvement to the file upload validation handling and error catching.

Although there shouldnt be any BC from either PR - at least people would be aware of a change during the upgrade process.

Thoughts?

[shssoichiro]

Although I may not hold much weight as I am new here, I would agree with @Arrilot that it would be better to modify the existing "required" rule than to force users to add a new "file" rule to any file upload validations.

A bugfix should not require end users to take an action, it should be invisible to them. Although adding a "file" validation rule is something that can be discussed as a new feature, it's separate from the bug immediately at hand.

[laurencei]

I disagree. Using the "required" rule creates two problems;

1. What if you dont want it to be _required_? You might have an _optional_ file upload. So you need a rule that allows for this.
2. Required validation returns either _true_ or _false_. So performing additional validation checks causes the original problem - that the required rule fails, leading to incorrect error messages that the file is missing, rather than there was an issue with the file itself.

And because this will go to the 5.1 branch (if Taylor accepts it) - then it just forms part of the upgrade docs. There is no forcing people to change - they can keep their current code as is. But if you want to improve the validation and file error handling, then you can update your code to use these rules.

[laurencei]

@taylorotwell - can you please provide some guidance on your thoughts on this please?

Are you happy if I do a PR to the 5.1 branch with some code similar to the above idea? (And I'll change https://github.com/laravel/framework/pull/8215 to also be to the 5.1 branch).

[GrahamCampbell]

Ping @taylorotwell.

[taylorotwell]

Sure send a PR.

[GrahamCampbell]

Ping @TheShiftExchange.

[tuanpt216]

Hi,
This issue hasn't been fixed.

Tested in Laravel 5.2.39, use case:

$file = Request::file('file');
var_dump($file->isValid());   // => false
$this->validate(request(), [
    'file' => 'file|max:300', // => Not check
    // 'file' => 'required'   // But this is checked, and failed: The file field is required.
]);

[tuanpt216]

@amonger I think we need better solution, because we need the validation message, not an exception...
Ping @GrahamCampbell

The problem here is all validation rules related to file (file, max, min, size, mimetypes, ..) will not run if the file is not successfully uploaded. The reason is that they are not implicit rules, so they are not validatable.

So should we make file-related rules to be implicit rules? Or do we have any better approaches?

[vigneshmuthukrishnan]

Hi
I use laravel 5.2 and i upload 5mb image so i have error
Error : Image source not readable
My code is here

     public function resizeImagePost(Request $request)
      {
     $this->validate($request, [
        'title' => 'required',
        'image' => 'mimes:jpeg,png,bmp,jpg,gif,svg,mp4,qt|file:1,10000',
          //'image' => 'required|image|mimes:jpeg,png,jpg,gif,svg',
       ]); 
           $image = $request->file('image');
          $input['imagename'] = time().'.'.$image->getClientOriginalExtension();


    $destinationPath = public_path('/thumbnail');
    $img = Image::make($image->getRealPath());
    $img->resize(200, 200, function ($constraint) {
        $constraint->aspectRatio();
    })->save($destinationPath.'/'.$input['imagename']);

    $destinationPath = public_path('/images');
    $image->move($destinationPath, $input['imagename']);

    //$this->postImage->add($input);

    return back()
        ->with('success','Image Upload successful')
        ->with('imageName',$input['imagename']);
}

[znck]

@vigneshmuthukrishnan Verify that client_max_body_size in nginx configuration is more than 5mb.

[vigneshmuthukrishnan]

sorry @znck im new in laravel i don't file path in client_max_body_size pls help me

[znck]

Try adding client_max_body_size 100m; to server { ... } block in /etc/nginx/nginx.conf.

Also check <form> has enctype="multipart/form-data".

[vigneshmuthukrishnan]

I'm work in local and I'm windows user then use in XAMPP

[vigneshmuthukrishnan]

{!! Form::open(array('route' => 'resizeImagePost','enctype' => 'multipart/form-data')) !!}

{!! Form::close() !!}
Correct

[znck]

{!! Form::open(array('route' => 'resizeImagePost', 'files' => true)) !!}

[vigneshmuthukrishnan]

Error :
NotReadableException in AbstractDecoder.php line 302:
Image source not readable
@

[znck]

@vigneshmuthukrishnan Better; take it to forum

[vigneshmuthukrishnan]

Hi @znck Now i change php.ini 'upload_max_filesize' and 'post_max_size'
But Now I have new Error

The localhost page isn’t working

localhost is currently unable to handle this request.
HTTP ERROR 500

[vigneshmuthukrishnan]

This my new errors:

MethodNotAllowedHttpException in RouteCollection.php line 218: