# [RFC] On-Demand Name Mangling - Thread: https://externals.io/message/89957 - Original archive: https://news-web.php.net/php.internals/89957 - Started by Bishop Bettini on 2016-01-01T21:47:09+00:00 - Messages: 11 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #89957 — Bishop Bettini — 2016-01-01T21:47:09+00:00 URL: https://externals.io/message/89957#89957 Hello Internals, Ringing in the new year with a proposal to retool name mangling: Mangling has the undesirable consequence that many external variables may map to one PHP variable. This leads to user confusion and user-land workarounds, not to mention bug reports. Since register_globals has been removed, we can replace automatic name mangling with on-demand mangling via `extract()`. This allows a one-to-one between external and PHP super-global variables, while fixing a few bug reports on extract as well. RFC: https://wiki.php.net/rfc/on_demand_name_mangling Thanks in advance for your feedback! bishop --- ## #89960 — Nikita Popov — 2016-01-01T22:11:07+00:00 URL: https://externals.io/message/89957#89960 · In reply to: #89957 > Hello Internals, > > Ringing in the new year with a proposal to retool name mangling: > > Mangling has the undesirable consequence that many external variables may > map to one PHP variable. This leads to user confusion and user-land > workarounds, not to mention bug reports. Since register_globals has been > removed, we can replace automatic name mangling with on-demand mangling via > `extract()`. This allows a one-to-one between external and PHP super-global > variables, while fixing a few bug reports on extract as well. > > RFC: https://wiki.php.net/rfc/on_demand_name_mangling > > Thanks in advance for your feedback! > bishop > I like the idea of getting rid of name mangling, but don't like the particular approach you propose here. In particular anything that will result in an arbitrary number of deprecation notices *controlled by user input* is a no go for me. I think this is one of the rare cases where I would favor the introduction of a new ini setting. E.g. mangle_names=0 disables name mangling, while mangle_names=1 throws a deprecation warning on startup and enables name mangling. mangle_names=0 should be the default. That is essentially disable name mangling, but leave an escape hatch for those people who rely on it (for whatever reason). Furthermore I don't like the suggestion that `extract()` should start mangling names by default. Currently "invalid" variable names will be ignored by `extract()`. We can introduce a EXTR_MANGLE_INVALID option for those who want it. Additionally we can also provide an EXTR_ALLOW_INVALID option, which simply extracts "invalid" names normally. (Imho that should be the default behavior. "Invalid" in this context is a misnomer, these variable names are perfectly fine and easily accessible.) Nikita --- ## #89961 — Bishop Bettini — 2016-01-02T03:09:42+00:00 URL: https://externals.io/message/89957#89961 · In reply to: #89960 > > > > […] > > I like the idea of getting rid of name mangling, but don't like the > particular approach you propose here. In particular anything that will > result in an arbitrary number of deprecation notices *controlled by user > input* is a no go for me. > Agreed. The message intends to provide some warning to application developers when there is known use of name mangling. As such, a single warning when the mangler runs is sufficient to meet this intent. I think this is one of the rare cases where I would favor the introduction > of a new ini setting. E.g. mangle_names=0 disables name mangling, while > mangle_names=1 throws a deprecation warning on startup and enables name > mangling. mangle_names=0 should be the default. That is essentially disable > name mangling, but leave an escape hatch for those people who rely on it > (for whatever reason). > An INI setting to disable mangling must be engine-wide. In a hosted environment where many unrelated sites share the same engine configuration, it's possible that one site might require mangling while another site requires no-mangling. These two sites could not co-exist with an engine setting. But, even without a setting, there's an escape hatch: userland can polyfill the mangling behavior using extract. The updated RFC demonstrates an algorithm. > Furthermore I don't like the suggestion that `extract()` should start > mangling names by default. Currently "invalid" variable names will be > ignored by `extract()`. We can introduce a EXTR_MANGLE_INVALID option for > those who want it. Additionally we can also provide an EXTR_ALLOW_INVALID > option, which simply extracts "invalid" names normally. (Imho that should > be the default behavior. "Invalid" in this context is a misnomer, these > variable names are perfectly fine and easily accessible.) > Agreed, that would unnecessarily introduce a BC break on extract. A new option makes the most sense. I've updated the RFC in light of all your comments/suggestions. Thanks for them! --- ## #89962 — Stanislav Malyshev — 2016-01-02T03:23:18+00:00 URL: https://externals.io/message/89957#89962 · In reply to: #89957 Hi! > Ringing in the new year with a proposal to retool name mangling: While the variable name changing probably outlived its usefulness with the demise of register_globals, changing it would produce serious BC issues with which we should be very careful. Emitting `E_DEPRECATED` based on user data certainly does not sound like a good idea. I like Nikita's proposal on both counts - making it an config switch (as much as I hate those) and have `extract()` flag for this (I would make the current behavior default to minimize BC disruption). I don't see why it says in the RFC it should be PHP_INI_ALL - I think it should be PHP_INI_SYSTEM or PHP_INI_PERDIR, as changing it at runtime won't do anything useful - the values should already be loaded. I would propose default for the INI to be current behavior for now and 0 for next version where we can break BC. BTW, for `extract()` option we don't need 2/3 majority, it's a simple function fix (which can be merged even in 7.0). -- Stas Malyshev smalyshev@gmail.com --- ## #89963 — Bishop Bettini — 2016-01-02T03:51:03+00:00 URL: https://externals.io/message/89957#89963 · In reply to: #89962 On Fri, Jan 1, 2016 at 10:23 PM, Stanislav Malyshev wrote: > Hi! > > > […] > > While the variable name changing probably outlived its usefulness with > the demise of register_globals, changing it would produce serious BC > issues with which we should be very careful. Agreed! > Emitting `E_DEPRECATED` based > on user data certainly does not sound like a good idea. > Agreed. I updated the RFC to throw a notice only once (per startup) and only if the mangler fires up. This is similar in spirit to the datetime.timezone setting. > I like Nikita's proposal on both counts - making it an config switch (as > much as I hate those) and have `extract()` flag for this (I would make the > current behavior default to minimize BC disruption). > In my 1.1 update to the RFC, I contend that a config switch will cause problems in some cases. Specifically, the logic goes like this. The config must be PHP_INI_SYSTEM. An engine wide config affects all sites in a shared hosting environment. If one of these sites requires mangling, while another site requires no mangling, we have an operational conflict. The only way to solve that is by giving each site its own engine, which isn't tenable in a shared environment. My RFC update further contends that a userland polyfill solves the BC issue without a config switch. Here's how: The engine is updated to not mangle. The polyfill provides a function that uses `extract()` to mangle the superglobals. Userland can maintain BC by composing in (or writing their own) polyfill. This approach improves the engine while giving existing sites an escape hatch. > I don't see why it says in the RFC it should be PHP_INI_ALL - I think it > should be PHP_INI_SYSTEM or PHP_INI_PERDIR, as changing it at runtime > won't do anything useful - the values should already be loaded. > That was a typo. Fixed. Thanks! --- ## #89966 — Rowan Collins — 2016-01-02T16:45:52+00:00 URL: https://externals.io/message/89957#89966 · In reply to: #89961 > But, even without a setting, there's an escape hatch: userland can polyfill > the mangling behavior using extract. The updated RFC demonstrates an > algorithm. I'm not that comfortable with requiring / encouraging the use of `extract()` - a rather awkward function which clobbers your local variable space in unpredictable ways - to write the polyfill. The fact that the clobbering is necessary / useful for `extract()` is wholly unrelated to the need for BC in super-global arrays. It would be more useful to expose a method or regex constant to userland so that the mangling can actually be done in place on appropriate arrays: $old_get = $_GET; $_GET = []; foreach ( $old_get as $old_key => $value ) { $new_key = preg_replace(PHP_ILLEGAL_NAME_REGEX, '_', $old_key); $_GET[$new_key] = $value; } Regards, -- Rowan Collins [IMSoP] --- ## #89990 — Bishop Bettini — 2016-01-04T16:16:49+00:00 URL: https://externals.io/message/89957#89990 · In reply to: #89966 On Sat, Jan 2, 2016 at 11:45 AM, Rowan Collins wrote: > > > > […] > > I'm not that comfortable with requiring / encouraging the use of `extract()` > - a rather awkward function which clobbers your local variable space in > unpredictable ways - to write the polyfill. The fact that the clobbering is > necessary / useful for `extract()` is wholly unrelated to the need for BC in > super-global arrays. > > It would be more useful to expose a method or regex constant to userland > so that the mangling can actually be done in place on appropriate arrays: > I've updated the on-demand name mangling RFC in light of all feedback so far. The crux being: - If an app requires name mangling: - As of 7.1, the app will get one `E_NOTICE` per startup warning of the impending removal of name mangling - As of 8.0, the app will need to include a polyfill package and add one line of code to its bootstrap. (Safe to include the polyfill prior to the release of 8.0.) - If an app does not need name mangling, there's no effect, no notices, nothing needed Thanks! --- ## #90351 — Nikita Popov — 2016-01-08T14:39:25+00:00 URL: https://externals.io/message/89957#90351 · In reply to: #89990 > On Sat, Jan 2, 2016 at 11:45 AM, Rowan Collins > wrote: > > > […] > > I've updated the on-demand name mangling RFC > in light of all > feedback so far. The crux being: > > - If an app requires name mangling: > - As of 7.1, the app will get one `E_NOTICE` per startup warning of > the impending removal of name mangling > - As of 8.0, the app will need to include a polyfill package and > add one line of code to its bootstrap. (Safe to include the polyfill prior > to the release of 8.0.) > - If an app does not need name mangling, there's no effect, no > notices, nothing needed > > Thanks! > I still don't like this part: > As written, one can't: the engine emits the error as soon as it mangles a variable, at most one time per startup. While that's annoying, it's also informative: someone's hitting your application in a way that may not expect. Even if it's only a single deprecation warning instead of multiple, it's still a deprecation warning that I, as the application author, have absolutely no control over. For me, a deprecation warning indicates that there is some code I must change to make that warning *go away*. Sure, it's informative. But it's enough to be informative about this *once*, rather than every time a user makes an odd-ish request. If we don't want to introduce an ini setting (against which there are good reasons, as you have outlined) I'd just mark this as deprecated with a nice red box in the documentation and upgrading guide, and commit to dropping it in 8.0, but not throw actual deprecation notices. Nikita --- ## #90403 — Bishop Bettini — 2016-01-09T02:21:12+00:00 URL: https://externals.io/message/89957#90403 · In reply to: #90351 On Fri, Jan 8, 2016 at 9:39 AM, Nikita Popov wrote: I still don't like this part: > > > > […] > a variable, at most one time per startup. While that's annoying, it's also > informative: someone's hitting your application in a way that may not > expect. > > Even if it's only a single deprecation warning instead of multiple, it's > still a deprecation warning that I, as the application author, have > absolutely no control over. For me, a deprecation warning indicates that > there is some code I must change to make that warning *go away*. > > Sure, it's informative. But it's enough to be informative about this > *once*, rather than every time a user makes an odd-ish request. > > If we don't want to introduce an ini setting (against which there are good > reasons, as you have outlined) I'd just mark this as deprecated with a nice > red box in the documentation and upgrading guide, and commit to dropping it > in 8.0, but not throw actual deprecation notices. > Agreed. Updated RFC. In summary: - The docs for 7.1 will say mangling is deprecated in 8.0 and that a polyfill to avoid a BC break will be available for download - The 8.0 code will completely remove all superglobal name mangling I feel we're pretty close on this, but of course comments appreciated: https://wiki.php.net/rfc/on_demand_name_mangling --- ## #90446 — Yasuo Ohgaki — 2016-01-10T08:58:18+00:00 URL: https://externals.io/message/89957#90446 · In reply to: #89957 Hi Bishop, > > RFC: https://wiki.php.net/rfc/on_demand_name_mangling I like the idea overall. mangle_superglobals()/name() could be php_mangle_super_global()/name() - "php_" prefix for being explicit it's for PHP, especially mangle_name(). - "super_global" rather than "superglobal" to obey CODING_STANDARD. BTW, you seems to have mistake in phpt sample. mangle_superglobals(); print_r(get_defined_vars()); This `print_r()` should print mangled names, but it's not. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #90454 — Rowan Collins — 2016-01-10T14:17:22+00:00 URL: https://externals.io/message/89957#90454 · In reply to: #90446 > - "super_global" rather than "superglobal" to obey CODING_STANDARD. AFAIK, "superglobal" is consistently spelled as one word throughout the manual, so that underscore looks very out of place to me. -- Rowan Collins [IMSoP]