[RFC] Deprecate the extract function in PHP 7.3 View as Markdown

The extract function takes an associative array and
puts it into the local symbol table.
http://php.net/manual/en/function.extract.php

I seriously doubt the usefulness of this function,
especially looking at the potential risks. The fact
that overwriting the local variables is the default
behaviour doesn’t make it any better. I suggest
deprecating it in PHP 7.3 and removing it in 8.

Preface: I despise extract() as well. It's breaks assumptions for
both the developer and the runtime. I save some of my frowniest of
faces for extract().

That said...

I can see it’s usefulness in this case.
But wouldn’t it be better to implement this by hand
in these rare cases (it’s 3 lines of code) instead of
encouraging the pollution of the symbol table by
unknown input? It’s also clearer since people who
don’t know the extract function probably don’t
expect it to mutate the local symbol table.

Let's be clear on what that looks like: foreach ($data as $key =>
$value) { $$key = $value; }

This is SO MUCH WORSE for several reasons, no least of all what
happens when $data contains keys named 'data', 'key', or 'value'.

This is a very good point. I hadn't thought about keys being named that
way, but obviously they could! grumble

I'd like to kill extract(), but it does have a reason for being, and I
couldn't in any good conscience support removing it without a
replacement that's at least marginally better.

-Sara

Hi Sara

That is indeed a very good point. Haven't thought of that one.

Regards

The extract function takes an associative array and
puts it into the local symbol table.
http://php.net/manual/en/function.extract.php

I seriously doubt the usefulness of this function,
especially looking at the potential risks. The fact
that overwriting the local variables is the default
behaviour doesn’t make it any better. I suggest
deprecating it in PHP 7.3 and removing it in 8.

Preface: I despise extract() as well. It's breaks assumptions for
both the developer and the runtime. I save some of my frowniest of
faces for extract().

That said...

I can see it’s usefulness in this case.
But wouldn’t it be better to implement this by hand
in these rare cases (it’s 3 lines of code) instead of
encouraging the pollution of the symbol table by
unknown input? It’s also clearer since people who
don’t know the extract function probably don’t
expect it to mutate the local symbol table.

Let's be clear on what that looks like: foreach ($data as $key =>
$value) { $$key = $value; }

This is SO MUCH WORSE for several reasons, no least of all what
happens when $data contains keys named 'data', 'key', or 'value'.

I'd like to kill extract(), but it does have a reason for being, and I
couldn't in any good conscience support removing it without a
replacement that's at least marginally better.

-Sara

Hi all,

The extract function takes an associative array and
puts it into the local symbol table.
http://php.net/manual/en/function.extract.php

I seriously doubt the usefulness of this function,
especially looking at the potential risks. The fact
that overwriting the local variables is the default
behaviour doesn’t make it any better. I suggest
deprecating it in PHP 7.3 and removing it in 8.

Preface: I despise extract() as well. It's breaks assumptions for
both the developer and the runtime. I save some of my frowniest of
faces for extract().

That said...

I can see it’s usefulness in this case.
But wouldn’t it be better to implement this by hand
in these rare cases (it’s 3 lines of code) instead of
encouraging the pollution of the symbol table by
unknown input? It’s also clearer since people who
don’t know the extract function probably don’t
expect it to mutate the local symbol table.

Let's be clear on what that looks like: foreach ($data as $key =>
$value) { $$key = $value; }

This is SO MUCH WORSE for several reasons, no least of all what
happens when $data contains keys named 'data', 'key', or 'value'.

I'd like to kill extract(), but it does have a reason for being, and I
couldn't in any good conscience support removing it without a
replacement that's at least marginally better.

The security issue regarding extract() are:

• Unintended variable creation. e.g. $admin_flag = ture
• Unintended variable modification. e.g. $admin_flag = ture

Instead of removing/deprecating extract(),

• Add EXTR_EXCEPTION flag that throws exception for overwriting.
  (There are many flags, but no error/exception flag. This isn't good)
• Require prefix. (User may still set '' as prefix)

We may do:

• Add EXTR_EXCEPTION flag in 7.3
• Make all three parameters required parameter set EXTR_ECEPTION a default
  flag in 8.0

With this way, we'll have better compatibility with older PHP and better
security with extract().

https://github.com/php/php-src/blob/master/ext/standard/array.c#L2459
Since it checks valid flags, we wouldn't have compatibility for current
versions unless
released versions code is modified for it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

2017-09-15 20:52 GMT+03:00 Ryan Pallas derokorian@gmail.com:

Hi Ryan

I can see your argument. The reasoning behind it is that a function in
the
standard library should not encourage unsafe code. Admittedly, since this
function is rarely used except for templating systems one could call
this a
non-issue. I just wanted to bring it up.

That makes sense. What about performance difference? On a reasonable sized
array (not 10k items and not 2) which is faster?

extract($array);

• or -

foreach ($array as $key => $value)
$$key = $value;

Honestly, I don't know (ps 2 lines without braces instead of 3!)

Regards

Hi!

The extract function takes an associative array and puts it into the
local symbol table.
http://php.net/manual/en/function.extract.php

$array = [
    ‘foo’ => ‘foo’,
    ‘bar’ => ‘bar’,
];

extract($array);

print $foo; // "foo"

As a second parameter the extract function takes some options to make
this function less dangerous, like EXTR_SKIP that prevents an existing
local variable of being overwritten. There’s a few more options, go ahead
and take a look at the documentation. EXTR_OVERWRITE is the default one
though. You can also pass a prefix for the variable names as a third
argument.

I seriously doubt the usefulness of this function, especially looking at
the potential risks. The fact that overwriting the local variables is the
default behaviour doesn’t make it any better. I suggest deprecating it in
PHP 7.3 and removing it in 8.

In a whole Symfony-Stack (3.4) with all of it’s dependencies I could only
find two usages of this function, both of which could be easily rewritten
in vanilla PHP:
https://github.com/symfony/symfony/blob/master/src/Symfony/
Component/Templating/PhpEngine.php#L148
https://github.com/symfony/symfony/blob/master/src/Symfony/
Component/Templating/PhpEngine.php#L158

Only downside: A polyfill is probably impossible since you cannot mutate
the local symbol table of the callee (as far as I’m aware).

Any thoughts?

I see no gain by removing this function. I've also seen it used for
templating quite often. Yes the functionality could be changed not to use
extract and end up working the same to the consumer but why make people
rewrite these things for no apparent gain (and likely a small performance
hit)?

Regards

Hi Ryan,
well, basically, none. Results are from a Q6600 machine and under windows,
so your mileage probably gonna be quite better :)

C:\Users\psihius\Documents\web>php -v
PHP 7.1.5 (cli) (built: May 9 2017 19:48:36) ( NTS MSVC14 (Visual C++

2015. x64 )
      Copyright (c) 1997-2017 The PHP Group
      Zend Engine v3.1.0, Copyright (c) 1998-2017 Zend Technologies
      C:\Users\psihius\Documents\web>php -d memory_limit=-1 test.php
      6.884626150131226 - Creating arrays
      2.035606861114502 - foreach
      2.128609180450439 - extract

The code:

define('ITERATIONS', 10000000);

$__time = microtime(true);

$__array = $__array2 = [];
for ($__i = 0; $__i < ITERATIONS; ++$__i) {
$__array['a'.$__i] = $__i;
$__array2['b'.$__i] = $__i;
}
echo number_format(microtime(true) - $__time, 15, '.', ''), PHP_EOL;

$__time = microtime(true);
foreach ($__array as $__key => $__variable) {
$$__key = $__variable;
}
echo number_format(microtime(true) - $__time, 15, '.', ''), PHP_EOL;

$__time = microtime(true);
foreach ($__array2 as $__key => $__variable) {
$$__key = $__variable;
}
echo number_format(microtime(true) - $__time, 15, '.', ''), PHP_EOL;

--
Arvīds Godjuks

+371 26 851 664
arvids.godjuks@gmail.com
Skype: psihius
Telegram: @psihius https://t.me/psihius

no, as there is no special risks

There certainly is. No other function (as far as I’m aware) mutates your local symbol table. This means you need to know exactly what symbols are defined and what kind of data you’ll receive when calling extract. So basically this is only safe right at the beginning of your function, and even then it can override your other parameters. Even with trusted data this can hardly be considered safe.

function foo(array $data, $bar) {
    extract($data);
    print($bar);
}

$this->foo(['bar' => 'baz'], 'bar’); // “baz"

Regards

Hi!

Dangerous meaning that if given untrusted input someone could mess with
the behaviour of your code. There are risks and benefits to every

Same as many other functions. Given untrusted input, unlink() could
delete files on your hard drive, and file_put_contents() could overwrite
your data or send it to unauthorized party. That's not the reason to
remove these functions.

solution. Certainly you’d agree that in some cases the risks outweigh
the benefits.

In some cases, yes. In this case, no, as there is no special risks not
existing in many other functions. Any function that has side effects
could do something unexpected when you give it unexpected input. Since
we're not converting PHP to be purely functional language just yet, the
solution is to use functions correctly, not remove them.

Stas Malyshev
smalyshev@gmail.com