Updating register_globals code for importing $_GET and $_POST data

Question

I'm updating a bunch of PHP code that relies on register_globals and uses request data globally. In the process of fixing/updating I spend a lot of time writing code that looks like:

<?php
$x = isset($_REQUEST['x']) ? $_REQUEST['x'] : $xdefault; // other times loaded from $_GET['x'] or $_POST['x']
$abc = isset($_REQUEST['abc']) ? $_REQUEST['abc'] : $abcdefault;
?>

I've been toying with the idea of using something like:

<?php
function _export(&$source, &$defaults) {
  return array_merge($defaults, array_intersect_key($source, $defaults));
}

// old code essentially uses $_REQUEST, but I could point this at $_POST
// $_GET or a db query in the future

$defaults = array (
  'x'   => $xdefault,
  'abc' => $abcdefault,
);


$options = _export($_GET, $defaults);
extract($options); //export if needed until global code can be fixed
?>

The benefits being:

• It's easier (I feel) to read array syntax compared to a list of ternary issets() assignments
• The defaults array acts as a whitelist
• The defaults array can be used to document the expected or required inputs

Are there any disadvantages or possible improvements to this approach? I would name _export() something different.

Answer 1

This seems a bit complex for relatively little benefit. Personally, from these two choices, I would go with the isset approach for simplicity and slightly improved performance.

But I do see your point about the default value, and the readability of the array structure. If you actually do have a lot of parameters which have a default value, your approach might have merit.

But since you are going through the trouble of updating legacy code anyways, you could also think about choosing a different approach and create an Input class. It would also result in readable code, and you could add input filters, increasing your security. The structure is really a matter of preference, but it might look something like this:

class Input {

    function getRaw($value, $default) { ... }
    function getInt($value, $default) { ... }
    function getSafeHTML($value, $default) { ... }
    function getFiltered($value, $default, $regex) { ... }
    ...

    function postRaw($value, $default) { ... }
    function postInt($value, $default) { ... }
    function postSafeHTML($value, $default) { ... }
    function postFiltered($value, $default, $regex) { ... }
    ...

    ...
}

It's then used like this:

$x  = Input::getInt('x', $xdefault);
$ab = Input::postCleanHTML('ab', $abdefault);

It's a bit more work, but it's readable, well structured, and provides additional security; filtering input should never be your main line of defense, but it is highly recommended as defense in depth (depending on the input of course; some input you want unaltered, which is why the getRaw method is there).

Misc

• There's no need to pass the arguments by reference to _export. It gives off the impression that the function will change the values, which it does not (and should not, especially for $_GET).
• if you are updating legacy code anyways, I would check if REQUEST is really needed. If it is not, change it to GET or POST to increase the security of the code even further.