Everything

php

[RFC] On-Demand Name Mangling

php.internals

Bishop Bettini

10 years ago

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

Nikita Popov

10 years ago

On Fri, Jan 1, 2016 at 10:47 PM, Bishop Bettini <bishop@php.net> wrote:

> 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

Bishop Bettini

10 years ago

On Fri, Jan 1, 2016 at 5:11 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Fri, Jan 1, 2016 at 10:47 PM, Bishop Bettini <bishop@php.net> wrote:
>
>> 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!

Rowan Collins

10 years ago

On 02/01/2016 03:09, Bishop Bettini 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:

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

Bishop Bettini

10 years ago

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

> On 02/01/2016 03:09, Bishop Bettini 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!

Nikita Popov

10 years ago

On Mon, Jan 4, 2016 at 5:16 PM, Bishop Bettini <bishop@php.net> wrote:

> On Sat, Jan 2, 2016 at 11:45 AM, Rowan Collins <rowan.collins@gmail.com>
> wrote:
>
>> On 02/01/2016 03:09, Bishop Bettini 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

Bishop Bettini

10 years ago

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

Stas Malyshev

10 years ago

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

Bishop Bettini

10 years ago

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!

Yasuo Ohgaki

10 years ago

Hi Bishop,

On Sat, Jan 2, 2016 at 6:47 AM, Bishop Bettini <bishop@php.net> wrote:

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

Rowan Collins

10 years ago

On 10/01/2016 08:58, Yasuo Ohgaki wrote:

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