What is the prevailing sentiment about extract() and compact() ?

> > > > > try { > > // do stuff > > } catch(Throwable $exception) { > > $this->logger->error("failed to do stuff", compact('exception')); > > throw $exception; > > } > > > > I wonder why not just create an array with the key... > > ```php > try { > // do stuff > } catch(Throwable $exception) { > $this->logger->error("failed to do stuff", ['exception' => $exception])); > throw $exception; > } > ``` > > It's a few more characters of course, but I would pick the readability > and simplicity over the potential typos (which the IDE should point > out) and a few key presses it saves.I

> > > > On Wed, Nov 29, 2023 at 9:20 AM Robert Landers <landers.robert@gmail.com> wrote: >> >> On Wed, Nov 29, 2023 at 8:19 AM Stephen Reay <php-lists@koalephant.com> wrote: >> > >> > >> > >> > > On 29 Nov 2023, at 09:58, Larry Garfield <larry@garfieldtech.com> wrote: >> > > >> > > 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 cumbersome >> > >> 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 untrusted data >> > > >> > > *snip* >> > > >> > >> I can imagine these could be candidates for deprecation ? Or limited >> > >> 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, which 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 was trying to use. I vetoed it. >> > > >> > > I would not mind if they were removed, but I don't know how large the 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 missing some factors here. >> > >> > Regarding use-cases for compact: the most common one I can think of from 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 structures 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 uncaught/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 names passed to compact are a variable name, and make changes when a variable is renamed via the IDE. They simply cannot do the same for plain array keys. >> > >> > 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 *should* happen, is to promote the current warning on undefined variables, to an 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 RFC 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) but I don't think that's enough of a case to remove it, because just like with compact, by nature of how variable scope works, it's very difficult/impossible 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_OVERWRITE, 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 problem with it. I still find `compact(nameof($exception))` a little confusing personally though. I have the feeling that the wish is to have a nice syntactic sugar for compact, not sure about extract though. Extract for me is somewhat 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. Someone adding an array key in another layer of code in an application can cause different behavior on a totally unrelated place when there's a variable 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 alternative solutions before outright deprecating. I'm still in favor of seeing it gone in its current form though.