validation of superglobal

Question

I am developing a class to assist multiple upload using the codeigniter and I decided to analyze my code using some online tools.

One of them was the code Climate; I noticed that when checking reports for code improvement it informs that it is not recommended to use superglobals such as $_FILES even in specific functions for validation such as;

public function hasFile($key)
{
   if (!isset($_FILES[$key]) {
       throw new Exception("Não existe o indice $key", 1);
   }
   return TRUE;
}

I tried to modify this by creating an attribute called key

public $key = 'userFile';

And call you within the duties

if (!isset($_FILES[$this->key]) {.....

But I keep getting the report that this is not a good practice! I also looked in the PHP documentation if there is any type of filter for superglobal $_FILES using the function filter_input and I haven’t found.

What would be the best way to validate the superglobal $_FILES?

print of the report;

In the function in question I do not attribute the super-global to any variable, I only perform the validation as shown above.

Answer 1

Looking closely at the question, it seems you want to use a class to apply upload operations.

I would recommend you in such cases really not use the $_FILES within the class. Also because, to that end, there would be no need for the methods to be dynamic, but rather static.

A good example I always like to use is the Symfony, that in their source codes, the class Request, Response or UploadedFile, are only entities that aim to process the data, regardless of the way they come - which in your case would be through the variable $_FILES.

What Symfony actually does is to create a static method that initializes a new instance, based on global elements. So yes, I think there’s something more organized, and not dependent on just having uploads coming from $_FILES - you could add file_get_contents('php://input') for example.

So in this context, I suggest creating the following static method for creating the instance:

class Upload
{
    protected $files = [];

    public function __construct(array $files)
    {
        $this->files = $files;
    }

    public function hasFile($key)
    {
        return isset($this->files[$key]);
    }

    public function getFile($key)
    {
        if ($this->hasFile($key)) {
            return $this->files[$key];
        }

        throw new Exception("Não existe o arquivo $key");
    }


    public static function createFromFilesGlobal()
    {
        return new static($_FILES);
    }
}

So the use would be:

 $files = Upload::createFromFilesGlobals();


 $files->getFile('key');

Note that, in this case, the createFromFilesGlobals is a static method - the opposite of dynamic. This method, aims to only return new static, passing as parameter the global variable $_FILES. This will cause an instance of Upload is returned with the property $files filled with the values of $_FILES.

Now, imagine if you want to use that same class to capture some sort of request that didn’t perhaps go through $_FILES PHP. What would be the solution?

Fictitious example:

 // essa função não existe no PHP, é só um exemplo
 $raw_files = parse_uploaded_files_from_php_raw_input('php://input'); 

 $files = new Upload($raw_files);

So, this way, you have a class that is not limited by a super global PHP variable, but only uses its values (or not), if necessary.

Answer 2

In programming, in part of time (ideally) you should not treat or change external elements within a function, that is to say function or method should not be able to be passed as argument or that belongs to the class.

This 'rule' is more or less as your mother said it does not accept anything from strangers, it includes variables or objects :P.

Remember that this rule can change as the language can be more or less rigid, take for example javascript, a function does not access N external elements?

I believe the solution to remove this Warning is to pass $_FILES in your class constructor and always change/return that copy.

Answer 3

In the code you presented there is no bad practice.

A bad practice would be to sanitize or filter the data of the superglobal.

Example of what would be a bad practice:

$_POST['foo'] = mysql_real_escape_string($_POST['foo']);

In the case of isset(), there is no change in value. There is nothing wrong with using it this way.

if (isset($POST['foo'])) {
    // bla bla bla
}

In the examples I used $_POST, however, the same applies $_GET, $_FILES, $_SERVER, finally, the superglobal: http://php.net/manual/en/language.variables.superglobals.php