Skip to content

Commit

Permalink
Fix $_FILES parsing
Browse files Browse the repository at this point in the history 
The old tests were just straight up wrong. I built them wrong when
converting the code from 2.x to now. In 2.x all files were preceded by
the `data` name which made files parsing much simpler as we had already
stripped off the first name element.

The new method feels better to me as well. It doesn't use reference
arguments and I think is a tiny bit simpler.

Refs #3999
  • Loading branch information
@markstory
markstory committed Jul 18, 2014
1 parent 96a1f36 commit 45725b8
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 243 deletions.
39 changes: 20 additions & 19 deletions src/Network/Request.php
View file
Expand Up @@ -402,10 +402,10 @@ protected static function _base() {
protected function _processFiles($post, $files) {
if (is_array($files)) {
foreach ($files as $key => $data) {
if (!is_numeric($key)) {
$this->_processFileData($post, '', $data, $key);
} else {
if (isset($data['tmp_name']) && is_string($data['tmp_name'])) {
$post[$key] = $data;
} else {
$post[$key] = $this->_processFileData([], $data);
}
}
}
Expand All @@ -414,31 +414,32 @@ protected function _processFiles($post, $files) {

/**
* Recursively walks the FILES array restructuring the data
* into something sane and useable.
* into something sane and usable.
*
* @param array &$post The post data having files inserted into
* @param array $data The data being built
* @param array $post The post data being traversed
* @param string $path The dot separated path to insert $data into.
* @param array $data The data to traverse/insert.
* @param string $field The terminal field name, which is the top level key in $_FILES.
* @param string $field The terminal field in the path. This is one of the
* $_FILES properties e.g. name, tmp_name, size, error
* @return void
*/
protected function _processFileData(&$post, $path, $data, $field) {
foreach ($data as $key => $fields) {
$newPath = $key;
if (!empty($path)) {
$newPath = $path . '.' . $key;
protected function _processFileData($data, $post, $path = '', $field = '') {
foreach ($post as $key => $fields) {
$newField = $field;
if ($path === '' && $newField === '') {
$newField = $key;
}
if ($field === $newField) {
$path .= '.' . $key;
}
if (is_array($fields)) {
$this->_processFileData($post, $newPath, $fields, $field);
$data = $this->_processFileData($data, $fields, $path, $newField);
} else {
if (strpos($newPath, '.') === false) {
$newPath = $field . '.' . $key;
} else {
$newPath .= '.' . $field;
}
$post = Hash::insert($post, $newPath, $fields);
$path = trim($path . '.' . $field, '.');
$data = Hash::insert($data, $path, $fields);
}
}
return $data;
}

/**
Expand Down
289 changes: 65 additions & 224 deletions tests/TestCase/Network/RequestTest.php
View file
Expand Up @@ -259,235 +259,76 @@ public function testPutParsingJSON() {
}

/**
* Test parsing of FILES array
* Test processing files with `file` field names.
*
* @return void
*/
public function testFilesParsing() {
$files = array(
'name' => array(
'File' => array(
array('data' => 'cake_sqlserver_patch.patch'),
array('data' => 'controller.diff'),
array('data' => ''),
array('data' => ''),
),
'Post' => array('attachment' => 'jquery-1.2.1.js'),
),
'type' => array(
'File' => array(
array('data' => ''),
array('data' => ''),
array('data' => ''),
array('data' => ''),
),
'Post' => array('attachment' => 'application/x-javascript'),
),
'tmp_name' => array(
'File' => array(
array('data' => '/private/var/tmp/phpy05Ywj'),
array('data' => '/private/var/tmp/php7MBztY'),
array('data' => ''),
array('data' => ''),
),
'Post' => array('attachment' => '/private/var/tmp/phpEwlrIo'),
),
'error' => array(
'File' => array(
array('data' => 0),
array('data' => 0),
array('data' => 4),
array('data' => 4)
),
'Post' => array('attachment' => 0)
),
'size' => array(
'File' => array(
array('data' => 6271),
array('data' => 350),
array('data' => 0),
array('data' => 0),
),
'Post' => array('attachment' => 80469)
),
);

public function testProcessFilesNested() {
$files = [
'image_main' => [
'name' => ['file' => 'born on.txt'],
'type' => ['file' => 'text/plain'],
'tmp_name' => ['file' => '/private/var/tmp/php'],
'error' => ['file' => 0],
'size' => ['file' => 17178]
],
0 => [
'name' => ['image' => 'scratch.text'],
'type' => ['image' => 'text/plain'],
'tmp_name' => ['image' => '/private/var/tmp/phpChIZPb'],
'error' => ['image' => 0],
'size' => ['image' => 1490]
],
'pictures' => [
'name' => [
0 => ['file' => 'a-file.png']
],
'type' => [
0 => ['file' => 'image/png']
],
'tmp_name' => [
0 => ['file' => '/tmp/file123']
],
'error' => [
0 => ['file' => '0']
],
'size' => [
0 => ['file' => 17188]
],
]
];
$request = new Request(compact('files'));
$expected = array(
'File' => array(
array(
'data' => array(
'name' => 'cake_sqlserver_patch.patch',
'type' => '',
'tmp_name' => '/private/var/tmp/phpy05Ywj',
'error' => 0,
'size' => 6271,
)
),
array(
'data' => array(
'name' => 'controller.diff',
'type' => '',
'tmp_name' => '/private/var/tmp/php7MBztY',
'error' => 0,
'size' => 350,
)
),
array(
'data' => array(
'name' => '',
'type' => '',
'tmp_name' => '',
'error' => 4,
'size' => 0,
)
),
array(
'data' => array(
'name' => '',
'type' => '',
'tmp_name' => '',
'error' => 4,
'size' => 0,
)
),
),
'Post' => array(
'attachment' => array(
'name' => 'jquery-1.2.1.js',
'type' => 'application/x-javascript',
'tmp_name' => '/private/var/tmp/phpEwlrIo',
$expected = [
'image_main' => [
'file' => [
'name' => 'born on.txt',
'type' => 'text/plain',
'tmp_name' => '/private/var/tmp/php',
'error' => 0,
'size' => 80469,
)
)
);
$this->assertEquals($expected, $request->data);

$files = array(
'name' => array(
'Document' => array(
1 => array(
'birth_cert' => 'born on.txt',
'passport' => 'passport.txt',
'drivers_license' => 'ugly pic.jpg'
),
2 => array(
'birth_cert' => 'aunt betty.txt',
'passport' => 'betty-passport.txt',
'drivers_license' => 'betty-photo.jpg'
),
),
),
'type' => array(
'Document' => array(
1 => array(
'birth_cert' => 'application/octet-stream',
'passport' => 'application/octet-stream',
'drivers_license' => 'application/octet-stream',
),
2 => array(
'birth_cert' => 'application/octet-stream',
'passport' => 'application/octet-stream',
'drivers_license' => 'application/octet-stream',
)
)
),
'tmp_name' => array(
'Document' => array(
1 => array(
'birth_cert' => '/private/var/tmp/phpbsUWfH',
'passport' => '/private/var/tmp/php7f5zLt',
'drivers_license' => '/private/var/tmp/phpMXpZgT',
),
2 => array(
'birth_cert' => '/private/var/tmp/php5kHZt0',
'passport' => '/private/var/tmp/phpnYkOuM',
'drivers_license' => '/private/var/tmp/php9Rq0P3',
)
)
),
'error' => array(
'Document' => array(
1 => array(
'birth_cert' => 0,
'passport' => 0,
'drivers_license' => 0,
),
2 => array(
'birth_cert' => 0,
'passport' => 0,
'drivers_license' => 0,
)
)
),
'size' => array(
'Document' => array(
1 => array(
'birth_cert' => 123,
'passport' => 458,
'drivers_license' => 875,
),
2 => array(
'birth_cert' => 876,
'passport' => 976,
'drivers_license' => 9783,
)
)
)
);

$request = new Request(compact('files'));
$expected = array(
'Document' => array(
1 => array(
'birth_cert' => array(
'name' => 'born on.txt',
'tmp_name' => '/private/var/tmp/phpbsUWfH',
'error' => 0,
'size' => 123,
'type' => 'application/octet-stream',
),
'passport' => array(
'name' => 'passport.txt',
'tmp_name' => '/private/var/tmp/php7f5zLt',
'error' => 0,
'size' => 458,
'type' => 'application/octet-stream',
),
'drivers_license' => array(
'name' => 'ugly pic.jpg',
'tmp_name' => '/private/var/tmp/phpMXpZgT',
'error' => 0,
'size' => 875,
'type' => 'application/octet-stream',
),
),
2 => array(
'birth_cert' => array(
'name' => 'aunt betty.txt',
'tmp_name' => '/private/var/tmp/php5kHZt0',
'error' => 0,
'size' => 876,
'type' => 'application/octet-stream',
),
'passport' => array(
'name' => 'betty-passport.txt',
'tmp_name' => '/private/var/tmp/phpnYkOuM',
'error' => 0,
'size' => 976,
'type' => 'application/octet-stream',
),
'drivers_license' => array(
'name' => 'betty-photo.jpg',
'tmp_name' => '/private/var/tmp/php9Rq0P3',
'error' => 0,
'size' => 9783,
'type' => 'application/octet-stream',
),
),
)
);
'size' => 17178,
]
],
'pictures' => [
0 => [
'file' => [
'name' => 'a-file.png',
'type' => 'image/png',
'tmp_name' => '/tmp/file123',
'error' => '0',
'size' => 17188,
]
]
],
0 => [
'image' => [
'name' => 'scratch.text',
'type' => 'text/plain',
'tmp_name' => '/private/var/tmp/phpChIZPb',
'error' => 0,
'size' => 1490
]
]
];
$this->assertEquals($expected, $request->data);
}

Expand Down

13 comments on commit 45725b8

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nice fix!
I encountered this issue a few days ago in 2.x.
Do you have a plan to backport these changes to 2.x?

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think this issue existed in 2.x as the files are generally always prefixed with data which should mitigate this issue.

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, it doesn't occur.
I have created a kind of bulk update action.
So I created a view like this.

echo $this->Form->input('0.Photo.file', array('type' => 'file'));
echo $this->Form->input('1.Photo.file', array('type' => 'file'));

In this case, $this->request->data will be like this.

Array
(
    [Photo] => Array
        (
            [file] => Array
                (
                    [name] => 1.jpg
                    [type] => image/jpeg
                    [tmp_name] => C:\xampp\tmp\phpD795.tmp
                    [error] => 0
                    [size] => 21391
                )

        )

    [1] => Array
        (
            [Photo] => Array
                (
                    [file] => Array
                        (
                            [name] => 2.jpg
                            [type] => image/jpeg
                            [tmp_name] => C:\xampp\tmp\phpD796.tmp
                            [error] => 0
                            [size] => 8096
                        )

                )

        )
)

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, and does backporting this fix to 2.x solve the issue?

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't look closely into this issue.
It may be only related this check.

$newPath = $key;
if (!empty($path)) {
    $newPath = $path . '.' . $key;
}

If $path is 0, it doesn't work.

So, now I think that we have only to fix it.

There might be no need to backport to 2.x.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, If you could share with me the $_FILES array that is troublesome, I can work on a fix.

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wanted to tell you that we didn't need all changes in this commit.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can change the path check to not use empty.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cd7438d

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very sorry ...
Now I understand what you mean.
But it's too late 😢

$_FILES array is such as the following .

array(
  'data' => array(
    'name' => array(
      'Photo' => array(
        0 => array(
          'file' => '1.jpg',
        ),
        1 => array(
          'file' => '2.jpg',
        ),
      ),
    ),
    'type' => array(
      'Photo' => array(
        0 => array(
          'file' => 'image/jpeg',
        ),
        1 => array(
          'file' => 'image/jpeg',
        ),
      ),
    ),
    'tmp_name' => array(
      'Photo' => array(
        0 => array(
          'file' => 'C:\\xampp\\tmp\\php5603.tmp',
        ),
        1 => array(
          'file' => 'C:\\xampp\\tmp\\php5604.tmp',
        ),
      ),
    ),
    'error' => array(
      'Photo' => array(
        0 => array(
          'file' => 0,
        ),
        1 => array(
          'file' => 0,
        ),
      ),
    ),
    'size' => array(
      'Photo' => array(
        0 => array(
          'file' => 21391,
        ),
        1 => array(
          'file' => 8096,
        ),
      ),
    ),
  ),
)

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think I've gotten the issue fixed - I figured it out by reading your earlier comments more carefully.

@chinpei215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works now. Thanks.

Please sign in to comment.