Right, seeing as I could go on and review your code in comments, I'd thought I'd post my comments + extra review in one go instead (after all, that's what this site is for).
Starting from the top:
public function uploadFile($titulo,$file) {
A couple of niggles and suggestions
- Please try to adhere to the PHP-FIG coding standards. They're not official, but there are no official standards. PHP-FIG is subscribed to by all the main players, so it's best you do, too
- The
$file argument is central to the entire method. Why isn't it the first argument? Suppose you want $titulo (I'm assuming it means title or something) optional, and default to a string?
- What is
$file meant to be? What do you expect it to be? If I am to use your class, I'd like to be able to know what I have to pass to your methods just by looking at their signature.
So, basically, I'd recommend changing your method signature to:
public function uploadFile(array $file, $titulo = 'optional default argument')
{//brace goes on new line
}
Even that wouldn't be to my liking, though. I should know I'm expected to pass a file, so why not write:
public function uploadFile(\SplFileInfo $file, $titulo = null)
{
}
In this case, you can get rid of that if (isset($file['name'])) bit, and focus on the stuff that matters.
Next:
$accepted_types = array(
'application/zip',
'application/x-zip-compressed',
'multipart/x-zip',
'application/x-compressed',
'application/x-rar-compressed',
);
Now every time this method gets called, the array above will be constructed. After the function returns, its ref-count will (or should) be zero, and it'll get GC. If you want to call this method more than once, or if you want to re-use this class for other file manipulations, you should turn this into a property:
class Downloads
{
private $accepted_mimes = array(
'application/zip',
'application/x-zip-compressed',
'multipart/x-zip',
'application/x-compressed',
'application/x-rar-compressed',
);
public function uploadFile(\SplFileInfo $file, $titulo)
{
//code here
}
}
Better yet: define an associative array (this can be a private static, for once) that groups the mime-types into "modes", which you can then use per constant:
class Downloads
{
const MIME_MODE_FILE = 1;
const MIME_MODE_TEXT = 2;
const MIME_MODE_FOO = 4;//powers of 2, for easy bitwise operations
private static $mime_modes = array(
self::MIME_MODE_FILE => array(
'application/zip',
'application/x-zip-compressed',
'multipart/x-zip',
'application/x-compressed',
'application/x-rar-compressed',
),
self::MIME_MODE_TEXT => array(
//mime-types for text content
)
);
private $mime_mode = null;
private $accepted = null;
public function __construct($mode = self::MIME_MODE_FILE)
{
if (!isset(static::$mime_modes[$mode])) throw new InvalidArgumentException('Invalid mime-mode for '.__CLASS__);
$this->mime_mode = $mode;
$this->accepted = static::$mime_modes[$mode];
}
}
And take it from there. The reason why bitwise operators can come in handy here is that, if I wanted to use your Download class to process text files, but then call a method that was intended for use on non-text files, like archive decompression, you could check the $this->mime_mode property.
However MS Office documents are, in effect zip files. So a mime-type to accept those would have to accept zip files, too. Thus, the mime-mode to set might have to be Downloads::MIME_MODE_FILE | Downloads::MIME_MODE_TEXT
Of course, you'll have to work on the constructor in that case, but that's something you can do yourself. It's not that hard.
As for the actual checking of the mime-type: I've previously linked this answer and discussed various ways to do so. You've decided to play it safe and go for the checking of the first n bytes. That's fine, but never trust the result of a single check, though: always check both the given mime-type, and the bytes, always go "Double Dutch".
Lastly: mysqli_* usage where prepared statements shine in absence.
Sure, you say this code will only be called deep in the bowels of your back-end system, and no user input will be used. Call me stupid, but an uploaded file is user input in my book. I haven't got the faintest idea as to what $titulo is, nor do I know what the filenames might look like. Personally, I'd still play it safe and use a prepared statement.
If you feel as if prepared statements will only cause overhead then don't, because prepared statements can be used more than once. You could lazy-load a statement:
class Downloads
{
//all of the consts, methods and properties I've previously listed +:
private $mysqli = null;
private $stmts = array();
//then add:
private function getStatement($queryString)
{
if (!isset($stmts[$queryString]))
{
$this->stmts[$queryString] = $this->mysqli->prepare($queryString);
}
return $this->stmts[$queryString];
}
}
The prepared statement won't exist until, from the uploadFile method you call:
$stmt = $this->getStatement(
'INSERT INTO descargas (titulo,size,src,filename) VALUES
(?,?,?,?)'
);
The first time this call is executed, you'll create a prepared statement, the following times, you'll just get the same prepared statement returned to you, ready to be used again. So the overhead isn't as big as you might expect it to be.
Security-wise, however, you'll be better of. And in a way, in terms of traffic between PHP and MySQL, you'll bode well, too: the query string from which the statement is prepared is sent once, for all subsequent execute calls, only the parameters are sent over to the server, via a different protocol, so if you execute this query 20 times or more, you'll possibly end up sending less data to the DB.
mysqli, but you're not using prepared statements. I'd play it safe, and use a prep. stmt. I'd also not use adie: you're usingmysqli::query() or die, so theelsewill never get executed. Besides,or dieshouldn't be used, except when debugging... \$\endgroup\$or die()since you told me not to in another question. But, i haven't removed them from the res of my code. Thank you very much again. \$\endgroup\$