Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Deprecate the extract function in PHP 7.3 »

[RFC] Deprecate the extract function in PHP 7.3

php.internals

Ilija Tovilo

8 years ago

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

Marco Pivetta

8 years ago

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/

On Fri, Sep 15, 2017 at 7:20 PM, <ilija.tovilo@me.com> wrote:

Ilija Tovilo

8 years ago

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

On 15 Sep 2017, 19:26 +0200, Marco Pivetta <ocramius@gmail.com>, wrote:

Marco Pivetta

8 years ago

Hi,

On 15 Sep 2017 19:37, <ilija.tovilo@me.com> wrote:

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


On 15 Sep 2017, 19:26 +0200, Marco Pivetta <ocramius@gmail.com>, wrote:

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/

On Fri, Sep 15, 2017 at 7:20 PM, <ilija.tovilo@me.com> wrote:

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.

Sara Golemon

8 years ago

On Fri, Sep 15, 2017 at 1:35 PM,  <ilija.tovilo@me.com> wrote:

> 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

Ryan Pallas

8 years ago

On Fri, Sep 15, 2017 at 11:50 AM, Sara Golemon <pollita@php.net> wrote:

> On Fri, Sep 15, 2017 at 1:35 PM,  <ilija.tovilo@me.com> wrote:
> > 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*

Dustin Wheeler

8 years ago

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

> On Sep 15, 2017, at 1:35 PM, ilija.tovilo@me.com wrote:
>
> 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

Ilija Tovilo

8 years ago

Hi Sara

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

Regards

Yasuo Ohgaki

8 years ago

Hi all,

On Sat, Sep 16, 2017 at 2:50 AM, Sara Golemon <pollita@php.net> wrote:

> On Fri, Sep 15, 2017 at 1:35 PM,  <ilija.tovilo@me.com> wrote:
> > 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

Ilija Tovilo

8 years ago

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


On 15 Sep 2017, 23:17 +0200, Yasuo Ohgaki <yohgaki@ohgaki.net>, wrote:

=?utf-8?B?5rGf5rmW5aSn6Jm+5LuB?=

8 years ago

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

On Fri, Sep 15, 2017 at 7:20 PM, <ilija.tovilo@me.com> wrote:

Ryan Pallas

8 years ago

On Sep 15, 2017 11:22 AM, <ilija.tovilo@me.com> wrote:

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

Ilija Tovilo

8 years ago

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

On 15 Sep 2017, 19:30 +0200, Ryan Pallas <derokorian@gmail.com>, wrote:

Ryan Pallas

8 years ago

On Fri, Sep 15, 2017 at 11:38 AM, <ilija.tovilo@me.com> wrote:

> 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!)

Arvids Godjuks

8 years ago

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

> On Fri, Sep 15, 2017 at 11:38 AM, <ilija.tovilo@me.com> wrote:
>
> > 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
> >
> >
> > On 15 Sep 2017, 19:30 +0200, Ryan Pallas <derokorian@gmail.com>, wrote:
> >
> >
> >
> > On Sep 15, 2017 11:22 AM, <ilija.tovilo@me.com> wrote:
> >
> > 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

Stephen Reay

8 years ago

> On 16 Sep 2017, at 01:27, Arvids Godjuks <arvids.godjuks@gmail.com> wrote:
>
> 2017-09-15 20:52 GMT+03:00 Ryan Pallas <derokorian@gmail.com>:
>
>>> On Fri, Sep 15, 2017 at 11:38 AM, <ilija.tovilo@me.com> wrote:
>>>
>>> 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
>>>
>>>
>>> On 15 Sep 2017, 19:30 +0200, Ryan Pallas <derokorian@gmail.com>, wrote:
>>>
>>>
>>>
>>> On Sep 15, 2017 11:22 AM, <ilija.tovilo@me.com> wrote:
>>>
>>> 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

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

lists@rhsoft.net

8 years ago

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

Ilija Tovilo

8 years ago

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.

On 16 Sep 2017, 11:42 +0200, lists@rhsoft.net <lists@rhsoft.net>, wrote:

lists@rhsoft.net

8 years ago

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?

Ilija Tovilo

8 years ago

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.

On 16 Sep 2017, 11:49 +0200, lists@rhsoft.net <lists@rhsoft.net>, wrote:

Rick Widmer

8 years ago

On 9/16/2017 3:50 AM, ilija.tovilo@me.com wrote:

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

Stas Malyshev

8 years ago

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

lists@rhsoft.net

8 years ago

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 s*itlab/s*ithub world!

Stas Malyshev

8 years ago

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

Ilija Tovilo

8 years ago

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 `extract`s behaviour without introducing at least one local variable that could be overwritten.

Thanks for the feedback everybody!

Regards

On 15 Sep 2017, 22:10 +0200, Stanislav Malyshev <smalyshev@gmail.com>, wrote:

Stas Malyshev

8 years ago

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

Ilija Tovilo

8 years ago

> 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


On 15 Sep 2017, 23:05 +0200, Stanislav Malyshev <smalyshev@gmail.com>, wrote:

Stas Malyshev

8 years ago

Hi!

On 9/15/17 2:20 PM, ilija.tovilo@me.com wrote:

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

lists@rhsoft.net

8 years ago

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"