[RFC] Deprecate the extract function in PHP 7.3

6 years ago by Marco Pivetta — view source

unread

Heya,

This is typically used in templating engines.
The one I worked on is
https://github.com/zendframework/zend-view/blob/5523511b6771cb6c060a77f6777426526a8db5ab/src/Renderer/PhpRenderer.php#L491-L492

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

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?

Regards

6 years ago by ilija.tovilo@me.com — view source

unread

Hi Marco

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.

Cheers

Heya,

This is typically used in templating engines.
The one I worked on is
https://github.com/zendframework/zend-view/blob/5523511b6771cb6c060a77f6777426526a8db5ab/src/Renderer/PhpRenderer.php#L491-L492

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/
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?

Regards

6 years ago by Marco Pivetta — view source

unread

Hi,

Hi Marco

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.

Cheers

Heya,

This is typically used in templating engines.
The one I worked on is
https://github.com/zendframework/zend-view/blob/
5523511b6771cb6c060a77f6777426526a8db5ab/src/Renderer/
PhpRenderer.php#L491-L492

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

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?

Regards

Absolutely, can be replaced with a loop indeed.

6 years ago by Sara Golemon — view source

unread

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

6 years ago by Ryan Pallas — view source

unread

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

6 years ago by Dustin Wheeler — view source

unread

Hi, (accidentally replied to only OP)...

Hi Marco

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.

This argument is predicated on the assumption that my input is unknown. This assumption is not universal to all codebases as implied.

There are many "foot guns" in every language and framework. I don't think it is necessary (or wise) for the language to assume responsibility for abuse.

This is the same line of reasoning that could be used to justify removing serialize/deserialize because a developer could "accidentally a tent" or we could build safer options into the feature (as you've already described)

Just my 2c

6 years ago by Ilija Tovilo — view source

unread

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

6 years ago by Yasuo Ohgaki — view source

unread

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

6 years ago by ilija.tovilo@me.com — view source

unread

Hi Yasuo

These sound good to me!
There’s still a smaller vulnerability of defining variables beforehand:

$data = ['hasAccess' => true];

export($data);

if ($user->isAdmin()) {
    $hasAccess = true;
}

if (isset($hasAccess) && $hasAccess === true) {
    print 'Bingo';
}

but code like this should be rather rare.

Regards

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

6 years ago by me@jhdxr.com — view source

unread

I think there is some idea we can borrow from the discussion of unserialize security policy. extract should not be used on untrusted input, and that's it.

Best regrads,

CHU Zhaowei

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?

Regards

6 years ago by Ryan Pallas — view source

unread

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

6 years ago by ilija.tovilo@me.com — view source

unread

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.

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

6 years ago by Ryan Pallas — view source

unread

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

6 years ago by Arvids Godjuks — view source

unread

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++

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

6 years ago by Stephen Reay — view source

unread

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++

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

Hi all,

If you want to reduce unintended consequences while retaining functionality, the default flag could be changed to EXTR_SKIP. Yes it affects BC but it's also much simpler to fix than if the functionality is gone completely, and in the case of templating it's probably already used that way in most cases anyway.

Cheers

Stephen

6 years ago by lists@rhsoft.net — view source

unread

Am 15.09.2017 um 20:27 schrieb Arvids Godjuks:

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

well, and now implement the EXTR_SKIP in PHP code - that becomes a ugly
piece of code and that only because someone likes to remove a already
existing function nobody is forced to use?

sadly i am not allowed to say what i think in public...

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

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;

6 years ago by ilija.tovilo@me.com — view source

unread

Dude, calm down.

This was a request for feedback. As other people have pointed out before it may not be the best idea to remove it since there’s no good way to write the equivalent code in PHP. No reason to be rude.

Am 15.09.2017 um 20:27 schrieb Arvids Godjuks:

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

well, and now implement the EXTR_SKIP in PHP code - that becomes a ugly
piece of code and that only because someone likes to remove a already
existing function nobody is forced to use?

sadly i am not allowed to say what i think in public...

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

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;

6 years ago by lists@rhsoft.net — view source

unread

Am 16.09.2017 um 11:44 schrieb ilija.tovilo@me.com:

Dude, calm down.

dude when people have no other hobbies than think about removing
functionality for no gain

This was a request for feedback

then you should withstand the feedback!
why don't you spend your time for add features instead cripple things?

As other people have pointed out before
it may not be the best idea to remove it since there’s no good way to
write the equivalent code in PHP. No reason to be rude.

Am 15.09.2017 um 20:27 schrieb Arvids Godjuks:

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

well, and now implement the EXTR_SKIP in PHP code - that becomes a ugly
piece of code and that only because someone likes to remove a already
existing function nobody is forced to use?

sadly i am not allowed to say what i think in public...

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

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;

6 years ago by ilija.tovilo@me.com — view source

unread

That’s fine, I have no problem with feedback. I changed my mind once other people pointed out the flaws in my thinking. Politely.

Sorry for spending my free time thinking about how to make PHP better. I’m not claiming to be an expert in any means, hence the RFC.

Am 16.09.2017 um 11:44 schrieb ilija.tovilo@me.com:

Dude, calm down.

dude when people have no other hobbies than think about removing
functionality for no gain

This was a request for feedback

then you should withstand the feedback!
why don't you spend your time for add features instead cripple things?

As other people have pointed out before
it may not be the best idea to remove it since there’s no good way to
write the equivalent code in PHP. No reason to be rude.

Am 15.09.2017 um 20:27 schrieb Arvids Godjuks:

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

well, and now implement the EXTR_SKIP in PHP code - that becomes a ugly
piece of code and that only because someone likes to remove a already
existing function nobody is forced to use?

sadly i am not allowed to say what i think in public...

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

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;

6 years ago by Rick Widmer — view source

unread

That’s fine, I have no problem with feedback. I changed my mind once other people pointed out the flaws in my thinking. Politely.

Sorry for spending my free time thinking about how to make PHP better. I’m not claiming to be an expert in any means, hence the RFC.

If only it were making PHP better. It is only breaking existing code
for no good reason. There have been way too many such suggestions
recently. That is why you are getting such a strong reaction!

6 years ago by Ilija Tovilo — view source

unread

Again, tell me I'm wrong. I have no problem with that. After all I'd still need 50%+ of people with voting privileges to agree with me which is rather unlikely if it has major flaws.

I don't claim to know it all, you don't need to get rude for me to listen. As soon as you do that the discussion goes to shit.

With that I'd like to end this discussion. Thanks to everybody who responded!

If someone is interested in creating an RFC for a new option to throw on variable re-declaration, as it has been suggested, feel free.

Regards

That’s fine, I have no problem with feedback. I changed my mind once other people pointed out the flaws in my thinking. Politely.
Sorry for spending my free time thinking about how to make PHP better. I’m not claiming to be an expert in any means, hence the RFC.

If only it were making PHP better. It is only breaking existing code for no good reason. There have been way too many such suggestions recently. That is why you are getting such a strong reaction!

6 years ago by Stanislav Malyshev — view source

unread

Hi!

This was a request for feedback

then you should withstand the feedback!

Request for feedback does not mean consent for abuse. You can criticize
without being offensive.

--
Stas Malyshev
smalyshev@gmail.com

6 years ago by lists@rhsoft.net — view source

unread

Am 15.09.2017 um 19:38 schrieb ilija.tovilo@me.com:

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

who do you think you are that you can qualify this?
breaking news: there is code outside the sitlab/sithub world!

6 years ago by Stanislav Malyshev — view source

unread

Hi!

As a second parameter the extract function takes some options to
make this function less dangerous, like EXTR_SKIP that

I'd start with specifying what exactly is "dangerous" in this function.
So far I don't see any specific danger. You can shoot yourself in the
foot, so you can with many other tools in the language.

I seriously doubt the usefulness of this function, especially looking
at the potential risks. The fact that overwriting the local variables

Which risks? This function is used by real-life code, and unless you do
something like extract($_GET) in global scope I don't see any problem.
With extract($_GET) we could then also propose to remove all file
functions because fopen($_GET['filename']) or unlink($_GET['filename'])
are also dangerous. But if you use it properly, I don't see what "risks"
are there.

Any thoughts?

-1 so far, I don't see what problem you are trying to solve.

--
Stas Malyshev
smalyshev@gmail.com

6 years ago by ilija.tovilo@me.com — view source

unread

Hi Stas

Dangerous meaning that if given untrusted input someone could mess with the behaviour of your code. There are risks and benefits to every solution. Certainly you’d agree that in some cases the risks outweigh the benefits.

As Sara pointed out, this might not be the case here as there’s no obvious way of mimicking extracts behaviour without introducing at least one local variable that could be overwritten.

Thanks for the feedback everybody!

Regards

Hi!

As a second parameter the extract function takes some options to
make this function less dangerous, like EXTR_SKIP that

I'd start with specifying what exactly is "dangerous" in this function.
So far I don't see any specific danger. You can shoot yourself in the
foot, so you can with many other tools in the language.

I seriously doubt the usefulness of this function, especially looking
at the potential risks. The fact that overwriting the local variables

Which risks? This function is used by real-life code, and unless you do
something like extract($_GET) in global scope I don't see any problem.
With extract($_GET) we could then also propose to remove all file
functions because fopen($_GET['filename']) or unlink($_GET['filename'])
are also dangerous. But if you use it properly, I don't see what "risks"
are there.

Any thoughts?

-1 so far, I don't see what problem you are trying to solve.

--
Stas Malyshev
smalyshev@gmail.com

6 years ago by Stanislav Malyshev — view source

unread

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

6 years ago by ilija.tovilo@me.com — view source

unread

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

6 years ago by Stanislav Malyshev — view source

unread

Hi!

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

Sure, because this is the function to mutate your local symbol table!
Why would we need more of them? It's like saying unlink() should be
removed because it deletes files and no other function does it, so it's
super-dangerous!

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.

It does exactly what you tell it to do. Extracts array into local symbol
table. If you don't need that, don't use that function.

--
Stas Malyshev
smalyshev@gmail.com

6 years ago by lists@rhsoft.net — view source

unread

Am 15.09.2017 um 19:20 schrieb ilija.tovilo@me.com:

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

then don't use it - who are you proposing remove functionality because
you don't use it - fine, nobody forces you to do so - a foreach()
replacement would miss EXTR_OVERWRITE - this whole thread is a "have
solution, looking for a problem"