[RFC] On-Demand Name Mangling

8 years ago by Nikita Popov — view source

unread

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

8 years ago by Bishop Bettini — view source

unread

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.

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!

8 years ago by Rowan Collins — view source

unread

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]

8 years ago by Bishop Bettini — view source

unread

On Sat, Jan 2, 2016 at 11:45 AM, Rowan Collins rowan.collins@gmail.com
wrote:

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:

I've updated the on-demand name mangling RFC
https://wiki.php.net/rfc/on_demand_name_mangling 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!

8 years ago by Nikita Popov — view source

unread

On Sat, Jan 2, 2016 at 11:45 AM, Rowan Collins rowan.collins@gmail.com
wrote:

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:

I've updated the on-demand name mangling RFC
https://wiki.php.net/rfc/on_demand_name_mangling 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

8 years ago by Bishop Bettini — view source

unread

On Fri, Jan 8, 2016 at 9:39 AM, Nikita Popov nikita.ppv@gmail.com
wrote: 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.

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

8 years ago by Stanislav Malyshev — view source

unread

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

8 years ago by Bishop Bettini — view source

unread

On Fri, Jan 1, 2016 at 10:23 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

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.

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!

8 years ago by Yasuo Ohgaki — view source

unread

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

8 years ago by Rowan Collins — view source

unread

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

[RFC] On-Demand Name Mangling

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

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