Hacker News Reader

Comment by Udo | Hacker News Reader

In all my years of writing PHP code I have yet to find a legitimate use case for extract() - PHP should have gotten rid of it generations ago.

View on HN

Frameworks tend to use it, to create variables in a view that were sent from a controller. For similar usecases it also makes sense (you want a bunch of variables created from an associative array).

It's not terribly bad if you use it inside a function or method. Creating global variables is what causes all the issues, but then again, if you're handling global variables, you're asking for trouble anyway.

What's really bad about extract() is that the default behavior is to overwrite existing variables. This is just a recipe for disaster, no less than the infamous register_globals.

I once wrote a framework where the controller would pass variables to the view as $view->currentUser and the template would access it as $currentUser instead of the more verbose $this->currentUser. This was implemented with extract(), but I went to great lengths to make sure that the scope was clean.

PHP probably ain't gonna get rid of extract() any time soon, but at least they should change the default to EXTR_SKIP.

You can use it to pass a bunch of variables between functions, without having to constantly get them from an array.

    function qux() {
      return compact('foo', 'bar', 'baz');
    }

And in the other function:

    extract(qux());

You can have one "preparer" function that works on the data and sets up the variables, then several others that work on it, and all call the same preparer. Or the reverse.

Obviously you can do the same thing by just passing arrays but sometimes a simple variable is easier and less cumbersome.

... and to think that my contempt for PHP couldn't be increased.

Wow. This is the anti-Scheme. It's like they took everything good in language design and decided to do the exact opposite. "Let's make a function that has the side effect of introducing variables into scope. That's a great idea."

Oh, Bog....

> "Let's make a function that has the side effect of introducing variables into scope. That's a great idea."

And ``let'' is? ;-)

Doing a (let [request] (check-auth ...)) would be equally dumb. Let's not blame the tools.

That's not not how 'let' works, you can't do that with 'let'. You most definitely should blame the tools here.

You're right, let is a macro so you can't pass binding forms from a variable, my mistake. But surely Scheme has some function for binding variables to the local scope...

My point is that trusting user input is the error, not having the ability to play with the scope.

Scheme has forms that add bindings to the local scope -- local defines -- but they're similar in spirit to let. There aren't any such functions in standard Scheme, and I don't know of any major implementation that has any. It's not clear how they would interact with macros or macro hygiene, and they would do unpleasant things to lexical scope which is Scheme's original schtick.

> This is the anti-Scheme.

Since when is Scheme the arbiter of what is good or bad?

PHP lets you do this, or not do this, your choice. Like C, PHP doesn't tell you what to do, it's up to you.

That's why people actually use it.

Exactly, what's wrong with a double clawed hammer?

Use it or not, if you don't like it you can smash stuff with the triblade screw driver. Or the 7 point socket wrench.

PHP provides a wealth of very useful tools that you can choose to use or not. I don't know why people think my 7 point socket wrench is dumb, it works quite well to round the edges of 6 sided bolts and saves a lot of money on buying those special bolts that can't be removed once tightened.

If this is the quality of the argument then I have nothing to concern myself with.

Your post might be [slightly] humorous, but as an argument against PHP language constructs it fails miserably.

It really doesn't. Especially when you see people pulling out the double claw hammers at your company.

Features that have limited real benefits with lots of risk get abused all the time. They are the retarded tools of the world creating technical debt for everybody else and they should be retired.

Extract is one of the stupidist language feature conceived precisely because it puts something so ripe for abuse into the hands of idiots. It even has a simple name that practically encourages its abuse.

Yup, then you see people building sine wave roads for the square wheels they build last week.

Or making critical parts of infrastructure work by a cron job that calls wget.

For some reason PHP needs extract, because a dictionary just won't do.

  foo["var_x"]

  // for some reason needs to be:

  $var_x

I think you mean catenaries, not sine waves.

Just because something is being used widely does not make it right, nor should it be held up as a fine example of design.

Language hacks like the one we're discussing are fine IF:

- You work with people who avoid bad features.

- You can avoid using code that uses bad features (less of a problem, until you have to debug things and then you're in a world of hurt).

- The features are not short-sighted hacks that prevent the language from moving forward (e.g., eliminate the opportunity to make things faster through dynamic compilation or whatever).

Scheme is a great language that is not useful in the real world, while PHP is a terrible language that happens to be in wide use. Neither of these positions are unique, and honestly I'd much rather use a bad language with good tooling than a great language with poor support. But I will continue to point out PHP's flaws, which are many and just howling bad, and work towards improving the alternatives.

The problem is when unexpected variables are overwritten. It would be better to have a mandatory list of allowed variables that can be extracted.

    $login = '1234';
    extract(array('login'=>true, 'messsage'=>'hacker'), 'message');
    var_dump($login_, $message);
    // => $login='1234', $message='hacker'

(pardon my rusty PHP)

> variables are overwritten. It would be better to have a mandatory list of allowed variables that can be extracted.

You can set the EXTR_SKIP flag to do something like that. There are a bunch of other flags as well to control how it works.

Or you could do this:

    function qux() { return [$foo, $bar, $baz]; }

    list($foo, $bar, $baz) = qux();

The problem with this is that it leaves things without any obvious, canonical, name. You have to read a bunch of code to figure out what's going on and a change in the values returned may result in subtle errors.

Coffeescript has a really neat destructuring operator for this kind of thing:

    qux = () -> {blah: "a", blorp: "b", zoop: "c"}
    {blah, blorp, zoop} = quz()

Yah, I do that when it's just a few variables, but it gets unmanageable when the number starts increasing.

It's also error prone if you have several feeder/user functions since you have to change them in multiple places.

Or if you have two code paths that end up with two different sets of variables (probably controlled by a semaphore variable) then this won't work.

This smells like an antipattern. I can't imagine a reason for wanting to do this that isn't a result of some very poor architectural choices.

I've found exactly two:

Ghetto templating (using a PHP file as a template). Extract an object from the database into local vars to echo in the "template".

Parsing fixed width files, zip the columns with their names, then extract in the processing function.

In both cases you know exactly which vars are being replaced. The real WTF is extract on $_{REQUEST,GET,POST,SERVER,...}.

I wouldn't use the word "exactly" (high chance?): f.ex. where did the fixed width files come from? If it's from some (external) other system you're already have an attack vector.

> f.ex. where did the fixed width files come from?

Encrypted and signed files sent over the wire from a company we do business with. Additionally I was just pairing the values up with local names that I chose (the fixed width file had no column names itself, they sent us a word doc (ugh)). And the function that called extract had exactly one local, the array I built representing a row from the file (unused after the initial call to extract).

Extract also lets you prefix the extracted vars, avoid overwriting name collisions, etc. http://php.net/extract

The code from this article is unsafe because it directly operates on user input, was not explicit about what values were required (you can filter an array by key easily enough...) and doesn't isolate the environment it's extracting in. That's the unsafe behavior.

I think it is a useful construct for specifically the scenario it is mis-used in here. Overriding default values, or a kwargs polyfill

    function foo($option1=1, $option2=2, $option3=3, $option4=4) {
        //do stuff with params
    }

this is unwieldy and difficult to reason about when you see it used, especially if you are trying to use some of the initial default values. ie.

    foo(null, null, null, 10)

vs.

    function foo($options) {
        $option1 = 1;
        $option2 = 2;
        $option3 = 3;
        $option4 = 4;

        extract($options);

        //do stuff with params
    }

With this, you can just do

    foo(array('option4'=>10));

obviously its a preference thing, but I think it is a somewhat harmless use case. If there are any params which you do not want to be overriden you move them beneath the call to extract.

We typically handle that by having a $defaults array, then array_merge($defaults, $options) in the function.

Yep, totally valid, and like I said basically a preference thing.

I personally like to work with explicit variables within the function rather then array indices. With your system I could easily call extract on your resulting array, but I also like defining the variables I want overridden explicitly in code so I my IDE doesn't think they are undefined (though JetBrains does a pretty good job of determining which variables are being created by extract in most scenarios anyway).

Wordpress shortcodes, to extract variables from an array into local scope. Works great and even lets you set default values easily.

I agree with you, but here the problem is not the extract() call as everyone here seems to imply.

The security problem lies in the fact that the authentication code is conditionally called depending on user input.

    if ($login) { … }

is not less safe than

    if ($_POST['c99shcook']['login']) { … }

would have been.

I'm pretty sure $login can be set to true/false somewhere on top of the file as 'configuration'. Only it gets overwritten by the call to extract().

It was not meant to be user input

I feel stupid. Of course, you are right.

Don't feel stupid. This is difficult to reason about, hence the existence of the bug and the inadvisability of PHP's `extract` function.

Drupal uses it to implement templates. I always got the feeling that it's one of those language features where you want to take a deep breath before using it; sort of like when you type `rm -rf *` and haven't pressed enter yet.

It makes perfect sense when PHP is actually being used as a backend-isolated, controlled-environment templating language.

Imagine this setup:

1. you have Nginx at $DOMAIN, with your well-oiled API service (with no HTML rendering logic to be found within it) mounted to /, and a PHP FCGI instance sitting on /views.

2. Nginx is configured such that /views is only accessible from localhost.

3. Your webapp on / mostly sees requests with "Accept: application/json" or some such. It responds to these directly.

4. If your webapp sees "Accept: text/html", it does the same stuff it would to render a text/json response... but then takes that response body, and sends it in an HTTP subrequest back to Nginx, to, say, /views/accounts.php.

5. PHP does this:

  extract(json_decode($_POST['response']));

...and then does whatever it does, simply and elegantly, to render your template.

6. Your webapp catches the response from Nginx, and sends it back without modification to the client.

You are setting yourself up for a reflected request forgery attack.

The way PHP allows fluid, dynamic interaction with the symbol table is pretty nifty, I think, and even if there were no extract() function built into the language, you could implement it yourself out of other primitives. I'm not one that believes that any feature of a language that can be used in dangerous ways should be eliminated (anything can be dangerous). I just think extract() is one that you ought to think carefully about before using. This example ought to reinforce that.

Agreed. The sane alternative is to extract specific, expected inputs to scope once they have been filtered and escaped. Any API I've written has this sort of validation built-in.

Exactly, I'm all for getting rid of black magics such as extract() and compact().

Reply on news.ycombinator.com