laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.45k stars 11k forks source link

Validating Multiple Files if failed will reset all sessions on php7 #14143

Closed 3alampro closed 8 years ago

3alampro commented 8 years ago

Hello,

i got this issue on laravel 5.2 and php7 i'm not sure about other versions since we got array validation on 5.2.

if you created form to upload multi-files and created request rules to validate the input if validations fails you will redirected back to the form with all sessions deleted.

by looking into laravel.log i got

[2016-06-27 04:23:13] local.ERROR: Exception: Serialization of 'Illuminate\Http\UploadedFile' is not allowed in /Applications/MAMP/htdocs/upload-multiple-files/bootstrap/cache/compiled.php:12115
Stack trace:
#0 /Applications/MAMP/htdocs/upload-multiple-files/bootstrap/cache/compiled.php(12115): serialize(Array)
#1 /Applications/MAMP/htdocs/upload-multiple-files/bootstrap/cache/compiled.php(11936): Illuminate\Session\Store->save()
#2 /Applications/MAMP/htdocs/upload-multiple-files/bootstrap/cache/compiled.php(2375): Illuminate\Session\Middleware\StartSession->terminate(Object(Illuminate\Http\Request), Object(Illuminate\Http\RedirectResponse))
#3 /Applications/MAMP/htdocs/upload-multiple-files/public/index.php(58): Illuminate\Foundation\Http\Kernel->terminate(Object(Illuminate\Http\Request), Object(Illuminate\Http\RedirectResponse))
#4 {main} 

i tried tracking the issue to withInput method on Illuminate\Http\RedirectResponse and it seem for some reason this code

return ! $value instanceof SymfonyUploadedFile;

is not working on php7 but work on php5.6 so i change it to this

if(!$value instanceof SymfonyUploadedFile)
{
    return $value;
}

this did fix it for me and all sessions will not be deleted because of trying to serialize resource 'files' but we also lost all errors related to this specific input field i hope this can improved or fixed in the next release

i created repo to demonstrate the issue https://github.com/3alampro/upload-multiple-files all you have to do is install it and run it on php7 and try to fail the validations by uploading non image files you can see that you will redirected back with reset sessions (old input,errors and auth if you used them) and you no longer have access to them

GrahamCampbell commented 8 years ago

Looks like you have an old version runnning. Make sure you only use laravel's uploaded file class.

GrahamCampbell commented 8 years ago

That repo isn't too useful as it is atm. Can you commit laravel first, then commit your changes so we can see what they are please?

3alampro commented 8 years ago

Hello @GrahamCampbell thank you for your fast response as you can see in the repo i'm on laravel v5.2.39 https://github.com/3alampro/upload-multiple-files/blob/master/composer.lock the repo it's just fresh laravel install and few basic codes to reproduce the issue and by searching here i just found many have it with extra info on it #12118 #12335 #13779 #13217 also i have the issue on all my laravel projects that allow multi-files upload and hosted with DigitalOcean and provisioned with forge with latest php7

thank you again for your effort.

3alampro commented 8 years ago

@GrahamCampbell @taylorotwell i'v been looking into this issue and why it's happening and it's really strange i even tried asking on StackOverFlow about why the code behave like this on php7 http://stackoverflow.com/questions/38082997/array-filter-on-php7

example to demonstrate the issue in raw php

<?php

$array = ['one', 'two', 'three', new DateTime(), [new DateTime(), new DateTime(), new DateTime()]];

$data = array_filter($array, $callback = function (&$value) use (&$callback) {
    if (is_array($value)) {
        $value = array_filter($value, $callback);
    }

    return ! $value instanceof DateTime;
});

echo "<pre>";

var_dump($data);

$data = array_filter($array, $callback = function (&$value) use (&$callback) {
    if (is_array($value)) {
        $value = array_filter($value, $callback);
    }

    if (! $value instanceof DateTime) {
        return $value;
    }
});

var_dump($data);

if we run this code on php5.6 the result is

array(4) {
  [0]=>
  string(3) "one"
  [1]=>
  string(3) "two"
  [2]=>
  string(5) "three"
  [4]=>
  array(0) {
  }
}
array(3) {
  [0]=>
  string(3) "one"
  [1]=>
  string(3) "two"
  [2]=>
  string(5) "three"
}

and if we run it on php7 the result is

array(4) {
  [0]=>
  string(3) "one"
  [1]=>
  string(3) "two"
  [2]=>
  string(5) "three"
  [4]=>
  array(3) {
    [0]=>
    object(DateTime)#2 (3) {
      ["date"]=>
      string(26) "2016-06-29 02:10:05.000000"
      ["timezone_type"]=>
      int(3)
      ["timezone"]=>
      string(11) "Asia/Riyadh"
    }
    [1]=>
    object(DateTime)#3 (3) {
      ["date"]=>
      string(26) "2016-06-29 02:10:05.000000"
      ["timezone_type"]=>
      int(3)
      ["timezone"]=>
      string(11) "Asia/Riyadh"
    }
    [2]=>
    object(DateTime)#4 (3) {
      ["date"]=>
      string(26) "2016-06-29 02:10:05.000000"
      ["timezone_type"]=>
      int(3)
      ["timezone"]=>
      string(11) "Asia/Riyadh"
    }
  }
}
array(3) {
  [0]=>
  string(3) "one"
  [1]=>
  string(3) "two"
  [2]=>
  string(5) "three"
}

as you can see the array filter function only remove the object from the first level array and ignore the second array

how this affect Laravel

in laravel we have array validations which is the best but when you validate multiple files that stored in single input as array if validations fails you end up with this error if you run your project on php7 local.ERROR: Exception: Serialization of 'Illuminate\Http\UploadedFile' is not allowed since the files object will never be removed in withInput method on Illuminate\Http\RedirectResponse

To demonistrate the error in laravel 5.2.* on php7

  1. install fresh copy of laravel 5.2
  2. add this route Route::post('upload', ['as' => 'store', 'uses' => 'HomeController@store']);
  3. add this form to the welcome.blade.php
<form action="{{ route('store') }}" method="post" enctype="multipart/form-data">
                    {!! csrf_field() !!}

    <label class="control-label" for="name">name</label>
    <input type="text" id="name" name="name" value="{{ old('name') }}">
    <div>{{$errors->has('name') ? $errors->first('name') : 'insert name here'}}</div>

    <label class="control-label" for="content">content</label>
    <input type="text" id="content" name="content" value="{{ old('content') }}">
    <div>{{$errors->has('content') ? $errors->first('content') : 'insert content here'}}</div>

    <label class="control-label" for="images">upload images</label>
    <input type="file" id="images" name="images[]" multiple="">
    <div>{{$errors->has('images') ? $errors->first('images') : 'select images to upload'}}</div>

    <button type="submit">upload</button>
</form>
  1. create form request and add these rules
return [
    'name'    => 'required|string',
    'content'    => 'required|string',
    'images.*' => 'image'
];
  1. create store method on HomeController and use the form request to validate the Request data
  2. finally try to fail the rules by uploading non image files you will redirect back with deleted sessions and you can see the error in your laravel.log file

i think laravel need improvement when validating files if they fails delete all file object and keep the errors only.

3alampro commented 8 years ago

interesting also if we use return $value instanceof SymfonyUploadedFile ? false : $value; in php7 and php5.6 we get the same result which remove all files object without issues

alwarren commented 8 years ago

This started happening to me yesterday after doing composer update. I did notice a lot of Symfony updates when I did that. Before the update, file validation on mimetype was failing but no messages were displayed from the error bag. After the update, I would get a brief error page followed by a redirect. I managed to get this screen grab before the redirect -

capture

I might also add that if files passed validation no error page was displayed. It happened with multiple files where one failed mimetype validation.

I'm using a FormRequest for validation and solved the issue by adding $dontFlash=['file_input_name'] to the request properties. Validation works as expected now and there is no error page displayed before the redirect.

The project is only a couple weeks old and was using Laravel 5.2.* in composer.json. I don't "think" I saw Laravel download anything when I did the update but I could be mistaken.

alwarren commented 8 years ago

@GrahamCampbell I've had a look at the docs for array_filter. There is a note that says:

Caution. If the array is changed from the callback function (e.g. element added, deleted or unset) the behavior of this function is undefined.

I think what's happening is subsequent calls to array_filter in the if() block remove children of array elements in $value causing the first callback to become undefined. At that point, the original call to array_filter defaults to testing for boolean. Since an array with elements doesn't cast to boolean false it returns the array of UploadedFile objects.

themsaid commented 8 years ago

Closing since it appears to be fixed in 5.3, please feel free to ping me if you still face the problem.