What is so wrong with extract()?

Question

I was recently reading this thread, on some of the worst PHP practices. In the second answer there is a mini discussion on the use of extract(), and im just wondering what all the huff is about.

I personally use it to chop up a given array such as $_GET or $_POST where I then sanitize the variables later, as they have been conveniently named for me.

Is this bad practice? What is the risk here? What are your thoughts on the use of extract()?

Answer 1

I find that it is only bad practice in that it can lead to a number of variables which future maintainers (or yourself in a few weeks) have no idea where they're coming from. Consider this scenario:

extract($someArray); // could be $_POST or anything

/* snip a dozen or more lines */

echo $someVariable;

Where did $someVariable come from? How can anyone tell?

I don't see the problem in accessing the variables from within the array they started in, so you'd really need to present a good case for using extract() for me to think it's worth it. If you're really concerned about typing out some extra characters then just do this:

$a = $someLongNameOfTheVariableArrayIDidntWantToType;

$a['myVariable'];

I think the comments here on the security aspects of it are overblown somewhat. The function can take a second parameter that actually gives you fairly good control over the newly created variables, including not overwriting any existing variables (EXTR_SKIP), ONLY overwriting existing variables (so you can create a whitelist) (EXTR_IF_EXISTS), or adding prefixes to the variables (EXTR_PREFIX_ALL).

Answer 2

Come on now. People blame the tool instead of the user.

That's like talking against unlink() because you can delete files with it. extract() is a function like any other, use it wisely and responsibly. But don't claim it's bad per se, that's just ignorant.

Answer 3

the risk is: don't trust data from users, and extracting into the current symbol table means, your variables could be overwritten by something the user provides.

<?php
    $systemCall = 'ls -lh';
    $i = 0;

    extract($_GET);

    system($systemCall);

    do {
        print_r($data[$i];
        $i++;
    } while ($i != 3);

?>

(a nonsensical example)

but now a malicious user who guesses or knows the code calls:

yourscript.php?i=10&systemCall=rm%20-rf

instead of

yourscript.php?data[]=a&data[]=b&data[]=c

now, $systemCall and $i are overwritten, resulting in your script deleting your data first and hanging then.

Answer 4

There is nothing wrong with it. Otherwise it wouldnt be implemented. Many (MVC) frameworks use it when you pass (assign) variables to Views. You just need to use it carefully. Sanitize those arrays before passing it to extract() and make sure it does not override your variables. Dont forget that this function also accepts a few more arguments! Using the second and third arguments you can control the behavior if collision occurs. You can override, skip or add prefix. http://www.php.net/extract

Answer 5

If not used carefully it can confuse the heck out of others you work with consider:

<?php

    $array = array('huh' => 'var_dump', 'whatThe' => 'It\'s tricky!', 'iDontGetIt' => 'This Extract Function');
    extract($array);
    $huh($whatThe, $iDontGetIt);


?>

Yields:

string(12) "It's tricky!"
string(21) "This Extract Function"

Would be useful to use in an obfuscation. But I can't get over the "Where did that var come from?" problem that I run into.

Answer 6

People get all up-in-arms about extract because it has the potential to be misused. Doing something like extract($_POST) is not a good idea in any case, even if you know what you are doing. However, it does have it's uses when you are doing things like exposing variables to a view template or something similar. Basically, only use it when you are very certain that you have a good reason for doing so, and understand how to use the extract type parameter if you get the idea of passing something crazy like $_POST to it.

Answer 7

I guess the reason a lot of people don't recommend using it is that extracting $_GET and $_POST (even $_REQUEST) superglobals registers variables in the global namespace with the same name as each key within those arrays, which is basically emulating REGISTER_GLOBALS = 1.

Answer 8

If you extract in a function, the variables will only be available in that scope. This is often used in views. Simple example:

//View.php
class View {
    function render($filename = null) {
        if ($filename !== null) {
            $this->filename = $filename;
        }
        unset($filename);
        extract($this->variables);
        ob_start();
        $this->returned = include($this->dir . $this->filename);
        return ob_get_clean();
    }
}

//test.php
$view = new View;
$view->filename = 'test.phtml';
$view->dir = './';
$view->variables = array('test' => 'tset');
echo $view->render('test.phtml');
var_dump($view->returned);

//test.phtml
<p><?php echo $test; ?></p>

With some alternative directories, checks to see if the file exists and defined variables and methods - you've pretty much replicated Zend_View.

You can also add $this->outVariables = get_defined_vars(); after the include to run code with specific variabels and get the result of these for use with old php code.

Answer 9

I'll let the PHP manual do the talking for me.

Background: extract($_REQUEST) is the same as setting register_globals = On in php.ini

Answer 10

Extract is safe as long as you use it in a safe manner. What you want to do is filter the array's keys to only the ones you intend to use and maybe check that all those keys exist if your scenario requires their existence.

#Extract only the specified keys.
$extract=array_intersect_key(
    get_data()
    ,$keys=array_flip(['key1','key2','key3','key4','key5'])
);

#Make sure all the keys exist.
if ($missing=array_keys(array_diff_key($keys,$extract))) {
    throw new Exception('Missing variables: '.implode(', ',$missing));
}

#Everything is good to go, you may proceed.
extract($extract);

or

#If you don't care to check that all keys exist, you could just do this.
extract(array_intersect_key(
    get_data()
    ,array_flip(['key1','key2','key3','key4','key5'])
));

Answer 11

Never extract($_GET) in a global scope. Other than that, it has its uses, like calling a function that could (potentially) have lots of optional arguments.

This should look vaguely familiar to WordPress developers:

function widget (Array $args = NULL)
{
    extract($args);

    if($before_widget) echo $before_widget;

    // do the widget stuff

    if($after_widget) echo $after_widget;
}

widget(array(
    'before_widget' => '<div class="widget">',
    'after_widget' => '</div>'
));

Answer 12

As someone noted in a different thread, here is a safer way to use extract, by only allowing it to extract variables you specify, instead of everything the array contains.

This serves a dual purpose of documenting what variables are coming out of it so tracking back a variable wont be so difficult.

Answer 13

Every method usage can lead to some conditions where it can be a point of failure for application. I personally feel that extract() should not be used for user input(that is not predictable) and for data that is not sanitized.

Even CodeIgniter core code uses extract, so there must be no harm in using the method if data is sanitized and handled well.

I have used extract in CodeIgniter models with the EXTR_IF_EXISTS switch and limiting the number of variables, it works pretty well.

Answer 14

The risk is the same as with register_globals. You enable the attacker to set variables in your script, simply by tampering with the request.

Answer 15

Be aware that extract() is not safe if you are working with user data (like results of requests), so it is better to use this function with the flags EXTR_IF_EXISTS and EXTR_PREFIX_ALL.

If you use it right, it safe to use

Answer 16

To just expound a little on previous answers... There's nothing wrong with extract() so long as you filter the input correctly (as other's have stated); otherwise you can end up with huge security issues like this:

<?php

// http://foobar.doo?isLoggedIn=1

$isLoggedIn = (new AdminLogin())->isLoggedIn(); // Let's assume this returns FALSE

extract($_GET);

if ($isLoggedIn) {
    echo "Okay, Houston, we've had a problem here.";
} else {
    echo "This is Houston. Say again, please.";
}

Answer 17

One additional good reason to no longer use extract() is that there is a momentum in PHP to use HHVM which is claiming to make PHP about 10x faster. Facebook (who made it) is using it, Wikipedia is on it, and WordPress is rumored to to be looking at it.

HHVM doesn't allow extract()

It's still kind of alpha, so it's not the largest concern