1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your code seems a bit broken. array_interset_key doesn't exist (do you mean array_intersect_key?), I'm not aware of any export function, and $result doesn't exist (do you mean $options?). It's also not completely clear to me what you mean by $xdefault; // or $_GET, $_POST. Can you add a bit more context? \$\endgroup\$ – tim Jan 15 '16 at 20:19
  • \$\begingroup\$ Sorry! You are right, extract() and array_intersect_key. I've updated the question. The comment was to indicate current code I am updating uses $_REQUEST, so that's usually what I do my isset and assignment off of. Sometimes I use $_GET or $_POST instead. \$\endgroup\$ – Pete Jan 15 '16 at 20:24
  • \$\begingroup\$ And your _export function should return the result of the array merge, right? \$\endgroup\$ – tim Jan 15 '16 at 20:31
  • \$\begingroup\$ Yes. Sorry again. I should have copy/pasted from my poc. \$\endgroup\$ – Pete Jan 15 '16 at 20:34
3
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ I like this class approach. I'd rather not roll my own if one already exists... do you know of any? \$\endgroup\$ – Pete Jan 15 '16 at 21:48
  • \$\begingroup\$ @Pete not really. Most such classes I saw contain quite a bit of additional logic, making them hard to reuse. I would just look at existing PHP projects and see how they handle it and then write it myself (it shouldn't be that much work). One example would eg be mediawiki (includes/WebRequest.php, starting around line 362), most frameworks should have a similar class. But for something like clean HTML I would definitely use a library such as html purifier, as it's too much work and quite complex. If you do end up writing your own feel free to post it here for review, I'd be interested in it. \$\endgroup\$ – tim Jan 15 '16 at 22:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.