Uploader Class about codeigniter4

[davidgv88]

+1 "Must be able to work with multiple files at once"

[lonnieezell]

You're absolutely correct. My last line there implied this, but wasn't very specific:

Most of the image features should be handled through a new Image class.

An issue exists for this one already, actually: https://github.com/lonnieezell/CodeIgniter4/issues/114

Using hooks/events (which is a pub/sub pattern basically) is an interesting idea, and I can see it being potentially very handy. Definitely something to consider more as we get closer to start working on it.

Thanks!

[orionstar]

I think the thumbnail creation should be decoupled from the Uploader class. Maybe the uploader should implement some kind of pubsub pattern or should use the events what you mentioned in the ticket about the queue interface.

[davidgv88]

@lonnieezell I'm trying to make the Uploader Class

https://github.com/davidgv88/CodeIgniter4/blob/upload_class/system/Uploader/Uploader.php

$options = array(
            'allowed_types' => array('jpg','png','gif'),
            'max_size' => 100
);
$uploader = new \CodeIgniter\Uploader\Uploader(\Config\Services::request(), $options);


$do_upload = $uploader->doUpload('file');

$uploadData = $uploader->uploadData();

[lonnieezell]

Looks like a good start. A couple of comments and/or thoughts:

1. You'll need to make sure that Ll comments are in English, please.
2. Error strings will need to be localized
3. Does it make sense to just return the file objects, instead of keeping a separate record of success and path in $dataUploads?

[lonnieezell]

The more I think about this the more I'm thinking this library isn't needed anymore. The FileCollection class that can be retrieved from the current Request object fulfills all of the standard requirements of being able to upload the files, I believe. Any image-related features would be part of the image library, anyway.

Does this make sense to everyone?

[lonnieezell]

Actually realized, also, that I started to add some file-related validation rules last night, and that pretty much fills out the basics of the uploader class, when combined with the existing UpoadedFile class.

I really think the upload class is unnecessary now, but I'll wait a few days to get other's comments before closing this.

[orionstar]

An upload class can provide some convience feature such as file type restriction, auto encrypt filenames, transform filename to lowercase etc. I use these in my own applications. I don't think these belongs to the HTTP "package".

[lonnieezell]

To be fair, though, uploaded files are part of the HTTP request so it makes sense to deal with them there, like any other form input.

Currently, you can grab all of the uploaded files from the request, and they are represented by an UploadedFile class that uses best practice and most secure methods I could find for correctly determining type, etc. It also has a move() method to move the file from it's temp location to the location of your choice. It also takes care of generating a secure random name if you want.

The Validation library now has elements to validate if a file is an image, restrict to certain mime types or extensions, set a maximum file size or, if an image, maximum dimensions. These all make sense as you are validating an HTTP input, just like you would validate everything else on the form.

So, the only things left from the current Upload class are some of the file naming/move options (like the lowercase option, or removing spaces, or overwriting/appending version numbers to file name. Setting things to lowercase or removing spaces can be done manually when you specify the name, though we could provide options in the File class, though I think that's perhaps stretching it, but would be handy. The overwrite options would make sense to go into the UploadedFile class since the move method is there already.

At that point, I believe all of the current Uploader library functions are taken care of in a way that's actually a little more true to the HTTP process than we have been.

At least, it seems to all make sense to me....

[orionstar]

I was thinkg about it with your message in mind, and you're right! The approach is new but the result is the same as used to...

[mwhitneysdsu]

So, the only things left from the current Upload class are some of the file naming/move options (like the lowercase option, or removing spaces, or overwriting/appending version numbers to file name. Setting things to lowercase or removing spaces can be done manually when you specify the name, though we could provide options in the File class, though I think that's perhaps stretching it, but would be handy. The overwrite options would make sense to go into the UploadedFile class since the move method is there already.

File naming rules and whether to allow overwrite (or what to do if naming conflicts arise) sound like either validation or lower-level business rules. While CI traditionally could do all of this stuff in the form_validation library, I don't know whether that's where you actually want it to reside in this case.

After looking up a couple of file move commands/functions/methods, it looks like it's fairly common to pass the source filename (obviously not needed in cases like the UploadedFile class, where this is already known), the destination path/filename, and, in complex methods, an options field, which usually includes a flag to allow overwrite. PHP's rename() accepts a context, which can include options like overwrite when it makes sense for the type of stream in use.

[lonnieezell]

Sorry if that was confusing. The naming rules wouldn't be part of validation. They'd be part of the UploadedFile class, which already has a move() method. Sending the overwrite/append a number/etc rules as part of an options array would work great for that, I think, and definitely should be looked into a little more.