Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121856
MIME-Version: 1.0
References: <6566989F.7010305@adviesenzo.nl> <34dada8e-7f2a-4d94-b7df-d9d3c7b2f3ce@app.fastmail.com>
 <F200E6CF-5116-4361-AB5C-CDD67F2E2021@koalephant.com> <CAPzBOBPLK3rQwqYsfgF3nS6=nT1qTdz-pzVpgNFRmXd7Jh9oSA@mail.gmail.com>
 <CANS-=pfpzNwzO2b+rfD+wHk-3wDBtZ1vDOafuZRR5wwX1d-tFA@mail.gmail.com>
In-Reply-To: <CANS-=pfpzNwzO2b+rfD+wHk-3wDBtZ1vDOafuZRR5wwX1d-tFA@mail.gmail.com>
Date: Wed, 29 Nov 2023 10:08:18 +0100
Message-ID: <CAPzBOBOGRLGi3+VdGG_z=LgTvFa0LUdQAp6oWtY_eb5_KED4Wg@mail.gmail.com>
To: Lynn <kjarli@gmail.com>
Cc: Stephen Reay <php-lists@koalephant.com>, php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] What is the prevailing sentiment about extract() and
 compact() ?
From: landers.robert@gmail.com (Robert Landers)

On Wed, Nov 29, 2023 at 9:56=E2=80=AFAM Lynn <kjarli@gmail.com> wrote:
>
>
>
> On Wed, Nov 29, 2023 at 9:20=E2=80=AFAM Robert Landers <landers.robert@gm=
ail.com> wrote:
>>
>> On Wed, Nov 29, 2023 at 8:19=E2=80=AFAM Stephen Reay <php-lists@koalepha=
nt.com> wrote:
>> >
>> >
>> >
>> > > On 29 Nov 2023, at 09:58, Larry Garfield <larry@garfieldtech.com> wr=
ote:
>> > >
>> > > On Tue, Nov 28, 2023, at 7:49 PM, Juliette Reinders Folmer wrote:
>> > >> L.S.,
>> > >>
>> > >> What with all the drives towards cleaner code, how do people feel
>> > >> nowadays about `extract()` and `compact()` still being supported ?
>> > >>
>> > >> Both have alternatives. The alternatives may be a little more cumbe=
rsome
>> > >> to type, but also make the code more descriptive, lessens the risk =
of
>> > >> variable name collisions (though this can be handled via the $flags=
 in
>> > >> extract), prevents surprises when a non-associative key would be
>> > >> included in an array and lessens security risks when used on untrus=
ted data
>> > >
>> > > *snip*
>> > >
>> > >> I can imagine these could be candidates for deprecation ? Or limite=
d
>> > >> deprecation - only when used in the global namespace ?
>> > >>
>> > >> For now, I'm just wondering how people feel about these functions.
>> > >>
>> > >> Smile,
>> > >> Juliette
>> > >
>> > > extract() has very limited use in some kinds of template engine, whi=
ch use PHP require() as a template mechanism.  I don't think compact() has =
any uses.
>> > >
>> > > I very recently was just reminded that these even exist, as i had to=
 tell one of my developers to not use them.  I think it was compact() he wa=
s trying to use.  I vetoed it.
>> > >
>> > > I would not mind if they were removed, but I don't know how large th=
e BC impact would be.  They'd probably need a long deprecation period, just=
 to be safe.
>> > >
>> > > --Larry Garfield
>> > >
>> > > --
>> > > PHP Internals - PHP Runtime Development Mailing List
>> > > To unsubscribe, visit: https://www.php.net/unsub.php
>> > >
>> >
>> > Hi,
>> >
>> > While I think I understand the goal behind this, I think you're missin=
g some factors here.
>> >
>> > Regarding use-cases for compact: the most common one I can think of fr=
om my work, is for passing multiple local variables as context to a logging=
 function, but I'd be surprised if its not also used to build faux hash str=
uctures too.
>> >
>> > If your goal is to achieve an associative array (i.e a poor mans hash)=
 of known variable names, using compact in php8+ has *less* risk of uncaugh=
t/unexpected errors than building it manually. Passing an undefined name (i=
.e. due a typo, or it just not being defined) produces a warning regardless=
 of whether you build the array manually or pass the name(s) to compact(). =
Providing an array key name that doesn't match the variable name (e.g. due =
to a typo, or a variable being renamed) will produce no error when building=
 the array manually, but will produce a warning with compact().
>> >
>> > IDEs (e.g. PHPStorm/IDEA+PHP plugin) can already understand that the n=
ames passed to compact are a variable name, and make changes when a variabl=
e is renamed via the IDE. They simply cannot do the same for plain array ke=
ys.
>> >
>> > Due to how variable scope works, the only way to re-implement compact(=
) with the same key-typo-catching behaviour as a function in userland would=
 be something that requires the user to pass the result of get_defined_vars=
() to every call.
>> >
>> > So no, I don't think compact() should be deprecated, what I think *sho=
uld* happen, is to promote the current warning on undefined variables, to a=
n error, as per https://wiki.php.net/rfc/undefined_variable_error_promotion=
. Whether this is a foregone conclusion or not, I don't know because that R=
FC doesn't mention compact() specifically.
>> >
>> >
>> > extract(), as Larry points out has historically been used by 'pure php=
' style template systems, in a manner that's generally "safe". Personally I=
'm less inclined to use this behaviour now (i.e. I'd prefer to access named=
 & typed properties from a template than arbitrary local variable names) bu=
t I don't think that's enough of a case to remove it, because just like wit=
h compact, by nature of how variable scope works, it's very difficult/impos=
sible to re-implement this in userland, in a way that's reusable and doesn'=
t involve using worse constructs (e.g. eval'ing the result of a function)
>> >
>> > I think there's possibly an argument to be made for improvements, such=
 as changing the default mode of extract to something besides EXTR_OVERWRIT=
E, or to have checks in place preventing the overwrite of superglobals.
>> >
>> >
>> > Cheers
>> >
>> >
>> > Stephen
>>
>> FWIW, I use compact all the time, usually like this:
>>
>> try {
>>   // do stuff
>> } catch(Throwable $exception) {
>>   $this->logger->error("failed to do stuff", compact('exception'));
>>   throw $exception;
>> }
>>
>> But thanks for the reminder to finish the nameof RFC, I was waiting
>> until after 8.3 to avoid the "trying to rush it to get into 8.3"
>> shenanigans that happened to another RFC around the same time. If
>> nameof passes, then it could make this more obvious when refactoring:
>>
>> try {
>>   // do stuff
>> } catch(Throwable $exception) {
>>   $this->logger->error("failed to do stuff", compact(nameof($exception))=
);
>> }
>>
>> Robert Landers
>> Software Engineer
>> Utrecht NL
>>
>> --
>> PHP Internals - PHP Runtime Development Mailing List
>> To unsubscribe, visit: https://www.php.net/unsub.php
>>
>
> My main concern with compact and extract is the counterintuitive usage of=
 variable names, which is also why I've personally broken production after =
changing a variable name. If there is another way of using compact in a way=
 where it's used without string and with dollar sign, I won't have a proble=
m with it. I still find `compact(nameof($exception))` a little confusing pe=
rsonally though. I have the feeling that the wish is to have a nice syntact=
ic sugar for compact, not sure about extract though. Extract for me is some=
what of a security concern as it will make it easy to implicitly overwrite =
variables from a local scope if the array being used is filled elsewhere. S=
omeone adding an array key in another layer of code in an application can c=
ause different behavior on a totally unrelated place when there's a variabl=
e collision, and there's no way to detect this when adding (or removing) a =
key from the array.
>
> I'm all for getting rid of extract. For compact I'd like to explore alter=
native solutions before outright deprecating. I'm still in favor of seeing =
it gone in its current form though.

It would be neat to combine something like nameof into something more intui=
tive:

$name =3D "Robert";
$title =3D "Software Engineer";

$arr =3D [ $name =3D> ..., $title =3D> ... ];

// $arr =3D=3D ['name' =3D> 'Robert', 'title' =3D> 'Software Engineer']

would basically mean take the name of the variable as the key and the
value as the value of the array. If we took the same semantics as
nameof, then even this would work:

$arr =3D [ $this->name =3D> ..., $this->title =3D> ... ];

// $arr =3D=3D ['name' =3D> 'Robert', 'title' =3D> 'Software Engineer']

I'm sure there is probably a prettier way to express it, but I don't
dislike it. Then we just deprecate compact. There's already
destructuring to handle extract's case:

[ 'name' =3D> $name, 'title' =3D> $title ] =3D $arr;

Robert Landers
Software Engineer
Utrecht NL