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

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