Critique Request: PHP Request-Method Class

I'm working on a general Requestmethod class which sanitizes and recasts the input of users in an automatic fashion. I've also tried to do array-access in cookies.

My biggest question are:

• Is this too much for one class alone?

• Is the readability/thinking right on this?

Code:

```
Example: ()
$a = new Request('POST');
$b = $a->getArray('checkboxes');
$b->get(0);
$b->get(1);
$b->get(2);

class REQUESTCONF {
const XSS = false;
const SALT = 'anysalt';

const c_expire = 24;
const c_path = '/';
const c_domain = '';
const c_secure = false;
const c_httponly = false;
}

/**
* Class to get Secure Request Data
*
* XSS Levels:
* 0 / false = off
* 1 = htmlentities
* 2 = strip tags
*
* @package Request
*/
class Request {

private $DATA;
private $CURSOR = false;
private $XSS = false;
private $METHOD;

/**
* Constructor
*
* @package Request
* @param string $METHOD POST GET SESSION or COOKIE
* @param boolean $XSS XSS Prevent Level
* @uses sanitize(

);
* @return object Request
* @access public
*/
function __construct($METHOD,$XSS=false) {
$this->METHOD = strtoupper($METHOD);
$this->XSS = $XSS;

switch ($this->METHOD) {
case 'FILE':
$this->DATA = $_FILES;
break;

case 'GET':
$this->DATA = $_GET;
break;

case 'POST':
$this->DATA = $_POST;
break;

case 'COOKIE':
foreach($_COOKIE as $k => $v) {
// hiding Notice - but no other way :'

Right, let me be clear, I mean this in the nicest of ways, but I don't like this code one bit. Let me start off a couple of simple things:

Please, follow the coding standards as described by PHP-FIG

I've noticed you're (ab)using the error supressing operator (eg @unserialize). Don't. Errors and Warnings are there to help you improve on your code. If there's a warning issued, don't cover it up, fix it. Even more worrying is where I saw this being used:

foreach($_COOKIE as $k => $v)
{
    $array = @unserialize($v);
    if ($array && is_array($array))
    {
        $this->DATA[$k] = $array;
    }
    else
    {
        $this->DATA[$k] = $v;
    }
}

Now this implies you're actually setting cookies to hold serialized objects or arrays. First off, it gives me a solid clue on what your stack looks like (I know for a fact you're using PHP). Serialized arrays are easily changed client side, so there's no guarantee on data integrity. If they're serialized objects, you're in trouble... big time. That would mean you're actually sending an object to the client, containing its state, and possibly all sorts of information on your server. If I were to be a 16 year old would-be hacker, I'd feel like I just struck gold. With some trial and error, I could easily manipulate that objects state, to gain access to those parts of your app you probably rather I didn't know about.

Cookies should cosist of little slivers of, on its own, meaningless data, Sessions are where you could store serialized objects, but still: I wouldn't.

Your Request class has a public function __construct function defined. Why then, do you also define a function Request? It looks as though you're trying to catch errors if somebody omitted the new keyword, or you're trying to mimic the factory pattern. Either implement a factory, or drop the function. If it's there to catch code that doesn't construct its instances using new, then that code contains bugs: fix them, don't work around them.

Next: The REQUESTCONF class, which only contains constants (ignoring the naming issues here). These constants obviously belong to the Request object: the Request object's state is defined by it. Drop that class, which is used as a constant array anyway, and define the constants as Request constants.

You don't need to call all those unset's in the destructor. Dealing with the session is fine, anything else is just overhead (the values will be unset when the object goes out of scope anyway), which is right after the destructor returns.

Moving on to some actual code:

public function getArray($key)
{
    if (!isset($this->DATA[$key]) || !is_array($this->DATA[$key])) {
        $this->CURSOR = false;
        return false;
    }
    $this->CURSOR = $key;
    return $this;
}

This doesn't make sense, to me at least. I'd expect the return type of getArray to be either an array or null, you're returning false, or the object itself. I see what you're doing here, but the name is just begging for accidents to happen.

either implement a child of the Traversable interface (which is what you're trying to do anyway) or change the method-name.

Your first question: Is this too much for one class?
Yes, it is. You might want to take a look at existing frameworks and how they manage the Request, what objects they use and how they're tied in.

Your Request object deals with everything, from the basic GET params to sessions. They're not the same thing, and should be treated as seperate entities. You're using a class as a module, which violates the single responsability principle.

I'd suggest you write a class for each request that requires its own treatment. Take, for example, a Session class. That class should indeed implement a destructor, but a Get object shouldn't. Their constructors should be different, too, but they can both implement the same basic getter and setter.

Start off by writing an abstract RequestType class, that holds the shared methods/properties, and extend it with the various request-type classes:

abstract class RequestType
{
    protected $data = null;
    protected $writeable = true;
    abstract public function __construct();//ensure all children implement their own constructor
    public function __get($name)
    {
        if (!is_array($this->data) || !isset($this->data[$name]))
        {//or throw exception
            return null;
        }
        return $this->data[$name];
    }
    public function __set($name, $val)
    {
        if ($this->writeable === false)
        {
            throw new RuntimeException(sprintf('%s instance is Read-Only', get_class($this)));
        }
        $this->data[$name] = $value;
        return $this;
    }
}
//then
class Session extends RequestType
{
    public function __construct($id = null, $readOnly = false)
    {
        $this->writeable = !!$readOnly;
        //start session, assign to $this->data
    }
}

You get the idea... You can use the abstract clas

StackExchange Code Review Q#29469, answer score: 13