Multiple files upload #173

Open
bitbucket-import opened this Issue Aug 19, 2011 · 18 comments

Comments

Projects
None yet

bitbucket-import commented Aug 19, 2011

Hi!

For one of projects I needed multiple files upload so I extended CI_Upload with two functions: do_multiple_upload() and get_data() (that are equivalents of do_upload() and data() respectively but for multiple files). My extension uses dirty hack with substituting $_FILES contents and calling do_upload(), but that's the best way I can think of (duplicating whole class or just do_upload() is much worse).

Maybe that feature is important enough to add to CI, or maybe someone would just find my code useful, so here it is:

# !php

<?php 
class MY_Upload extends CI_Upload {

private $multiple_upload_data = array();

/**
 * Perform multiple file upload
 *
 * @return  bool
 */
public function do_multiple_upload($field = 'userfile[]')
{
    $files = $_FILES; // we would change $_FILES so we need a copy
    $flag = false; // flag indicates if at least one upload was successful
    $fld = substr($field, 0, strlen($field)-2); // cutting out '[]' at the end
    for($i = 0; $i < count($files[$fld]['name']); $i++) { // for each file
        // setting up $_FILES so it looks like we uploaded single file
        foreach($files[$fld] as $attr => $values) {
            $_FILES[$fld][$attr] = $values[$i];
        }
        // performing upload and updating flag
        $res = $this->do_upload($fld);
        $flag = $flag || $res;
        // saving upload's data (it can later be fetched using get_data()
        array_push($this->multiple_upload_data, $this->data());
    }
    return $flag;
}

// --------------------------------------------------------------------

/**
 * Returns data about all uploaded files as data() method does
 *
 * @return  array
 */
public function get_data()
{
    return $this->multiple_upload_data;
}

// --------------------------------------------------------------------


}

?>

Thank you for developing Code Igniter!

Jajajaa , Hello friend, you save my work...after two hours i research for php.net view the functionality of $_FILE
and i remake all C_IUpload class to tray multiple file upload, thanks a lot
I think codeigniter must have a multiple upload in the next release. don't you?

purdy commented Jun 11, 2012

In the latest HTML file input spec, you have a multiple parameter, which you can set to true to select more than one file to upload at a time. I took this code and it almost worked, but I had to call initialize every time as it couldn't use the upload path, which meant I also needed to pass in the configuration (though there's probably a slick way to pull it in as a subclass).

  public function do_multiple_upload($field = 'userfile', $config = array() ) {
    $files = $_FILES; // we would change $_FILES so we need a copy
    $flag = false; // flag indicates if at least one upload was successful
    for($i = 0; $i < count($files[$field]['name']); $i++) { // for each file
      // setting up $_FILES so it looks like we uploaded single file
      foreach($files[$field] as $attr => $values) {
        $_FILES[$field][$attr] = $values[$i];
      }
      // performing upload and updating flag
      $this->initialize($config);
      $res = $this->do_upload($field);
      $flag = $flag || $res;
      // saving upload's data (it can later be fetched using get_data()
      array_push($this->multiple_upload_data, $this->data());
    }
    return $flag;
  }

Dentxinho pushed a commit to Dentxinho/CodeIgniter that referenced this issue Sep 28, 2012

Merge pull request #173 from nihaopaul/2
documentation updates only

sviande pushed a commit to sviande/CodeIgniter that referenced this issue Jan 3, 2014

Contributor

jim-parry commented Nov 11, 2014

It looks like this could be profitably added to the file uploading class (libraries/upload).
This would need an update to the documentation and changelog too.
Any takers?

This is an unnecessary change,It's very easy to handle multiple uploads, default is better(brings more control)
It's just my opinion
Thanks

I think codeigniter must have a multiple upload in the next release.

Te class CI_Upload should have do_upload() method and do_multiple_upload() method.

Once uploaded files $this->upload->data() method should return an associative array with the information of each file.

Why has this been made "won't fix"? It still needs fixing. I can't believe I am still having to extend the upload class...

If people are struggling to write this here is a nice example: https://github.com/avenirer/MY_Upload

in my opinion it is shame in 2015 and with good framework like codeigniter to discuss if we accept or not accept to add multiupload... so i support this request with @pleaseremove

Contributor

narfbg commented Dec 3, 2015

You know what's a shame? That even in 2016 people like you will come here complaining that the framework isn't adding tens of lines of code for something that you can do by simply wrapping do_upload() in a foreach loop.

We're not discussing this; it's rejected and that's why it has the "Won't Fix" label. The issue has been kept open just so you don't open new ones about the same thing.

people like me don't waste their times to write basics and understand the DRY concept and if framework like codeigniter doesn't support this basics it will be shame :)

Contributor

narfbg commented Dec 3, 2015

I hate going off-topic on stupid shit, but I also hate it when people talk nonsense and I especially hate it when every refusal to implement a shorthand for something is met with "blah blah blah DRY blah blah".

If you truly understood the DRY concept, you would know that it stands for "Don't Repeat Yourself". So please explain to me, how is implementing a do_multiple_uploads() method is not in fact repetition of all the do_upload() logic? Because that would be, by definition, a violation of the DRY principle.
And furthermore, how would it make your code more in tact with DRY? The way I see it, there's nothing WET about the following piece of code:

foreach (['file1', 'file2', 'file3'] as $field)
{
    $this->upload->initialize($config);
    if ($this->upload->do_upload($field))
    {
        // success
    }
    else
    {
        // error
    }
}

And last but not least, DRY, KISS and whatever other catchy-name principles you can think of, are not the holy grail of programming and must not be followed blindly. There's always context to have in mind and you must always, always know why you're doing something.

Contributor

narfbg commented Dec 3, 2015

@davidgv88 Your last comment didn't accidentally disappear, I deleted it. This isn't StackOverflow ... we're not looking for a 3 pages long snippet of how to do something.

I do take your point @narfbg and clearly whatever you decide is what will happen so that is fair enough.
I do just want to make clear that I do not support a "do_multiple_upload" option. That to me is not very elegant.

The way I normally extend what is there (and as such what I would be after) is support for the "multiple" attribute. Aka, the do upload would perform as normal, but if there were multiple files under that field name, spit out an array of results.

I accept there are complications like what to do if there is a single failure in a mix of 3 or 4 files etc, but personally I find most of my apps need to support multiple files.

Thanks

@pleaseremove Codeigniter by default not support upload files with "multiple" attribute.

The Solution with "multiple" attribute is this:

<removed>

@narfbg narfbg locked and limited conversation to collaborators Dec 3, 2015

@narfbg narfbg unlocked this conversation Dec 8, 2015

Has this been implemented yet? This looks just like the thing I need for our system. Looping is such a hassle and strain on resources.

Contributor

renedekat commented Apr 22, 2016

According to the DRY principle we shouldn't have to write for loops every time we want to implement multiple file uploads. Besides that it keeps code clean if it's centralised.

Waiting for this to implemented.

Contributor

narfbg commented Apr 22, 2016

TL;DR:

There is nothing left to be contributed to this thread and it has already been decided that this is not going to be implemented.

Here's a few reasons why (the list is not exhaustive):

  • Implementing the feature means making assumptions about:
    • Configuration being always the same
    • Error handling and the desired behavior
    • The desired behavior at large
  • The above-listed assumptions will result in numerous bug reports, regardless of how those would be treated. It won't make everybody happy.
  • Writing a loop is simple, easy, trivial.
  • Few applications need to use this feature on more than one screen and a well-designed application has to abstract that; this is not a DRY violation (and DRY is not the "law" to begin with).

The decision won't change.

I'm permanently locking the thread to avoid more pointless messages.


Has this been implemented yet?

Yes, but we like to troll people by keeping issues open and with the "Won't Fix" label applied.

Looping is such a hassle and strain on resources.

I agree, looping does take hours to implement. And it takes up so much resources, which would magically go away if the framework did the looping instead of you.

According to the DRY principle we shouldn't have to write for loops every time we want to implement multiple file uploads.

Indeed, the DRY principle does explicitly say that for loops equal repetitive code writing ... if only somebody else pointed this out earlier so we could avoid having such a long thread.

</sarcasm>

To the possibly offended by this comment:

If you can't be bothered to read the thread you are replying to (not even the last few comments, nor even its very visible labels), then you cannot reasonably expect a nice answer either.

No offense meant; that's just how it works and I like sarcasm.

@narfbg narfbg locked and limited conversation to collaborators Apr 22, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.