Everything

php

Evaluate arguments of new for classes without constructors

php.internals

Nikita Popov

10 years ago

Hi internals,

Currently

class Foo {}
new Foo(print 'xyz');

will not print "xyz", because the arguments to "new" are not evaluated if
the class has no constructor. Conversely

class Foo { function __construct() {} }
new Foo(print 'xyz');

*will* print "xyz". This behavior is confusing, especially if the code is
new $class(...) where $class may or may not have a constructor and thus the
side effects of "..." may or may not occur, leading to a hard to identify
failure mode.

I do not believe this behavior is intentional, it is simply an artifact of
the precise way in which "new" was implemented. HHVM does not implement
"new" in this way, they always evaluate the arguments.

We regularly get bug reports about this behavior. A sample of recent ones I
was able to track down: #54162, #54170, #65930, #67829 and #70698.

I'd like to fix this behavior, so that new arguments are always evaluated.
Here's a patch: https://github.com/php/php-src/pull/1802

As this is technically a BC affecting change (even if of the lowest
impact), I'm running it past the list first.

Thanks,
Nikita

Sara Golemon

10 years ago

On Sat, Mar 12, 2016 at 8:01 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

>     class Foo {}
>     new Foo(print 'xyz');
>
> will not print "xyz", because the arguments to "new" are not evaluated if
> the class has no constructor. Conversely
>

I recall someone once dubbing this "The Rasmus Optimization".  I don't
know if that means it was intentional, or just a lazy jibe, but yeah.
It's a long known issue, so it's probably good that you posted about
it first, as someone may have a legit reason for not wanting to fix
it. (I don't, I regard this as a php sadness)

> HHVM does not implement "new" in this way, they always evaluate the arguments.
>

Which, if it helps, means that we already know a lot of frameworks
/don't/ break as a result of fixing this behavior.

> As this is technically a BC affecting change (even if of the lowest
> impact), I'm running it past the list first.
>

Technically a BC break that justifies waiting till a major version.
Given the extremely specific requirements to hit this edge case
however, passing side-effect args to a non-existent constructor, I
personally think a minor version is fine.

+1 (without having looked at the patch)

-Sara

Paul M Jones

10 years ago

> On Mar 12, 2016, at 11:27, Sara Golemon <pollita@php.net> wrote:
>
>> HHVM does not implement "new" in this way, they always evaluate the arguments.
>>
> Which, if it helps, means that we already know a lot of frameworks
> /don't/ break as a result of fixing this behavior.
>
>> As this is technically a BC affecting change (even if of the lowest
>> impact), I'm running it past the list first.
>>
> Technically a BC break that justifies waiting till a major version.
> Given the extremely specific requirements to hit this edge case
> however, passing side-effect args to a non-existent constructor, I
> personally think a minor version is fine.

Of curiosity, what effect (if any) might this have on ReflectionClass::newInstance(), ::newInstanceArgs(), and ::newInstanceWithoutConstructor()?  (I ask because some existing DI systems, such as Aura.Di, rely on those for creating objects.)

--
Paul M. Jones
pmjones88@gmail.com
http://paul-m-jones.com

Modernizing Legacy Applications in PHP
https://leanpub.com/mlaphp

Solving the N+1 Problem in PHP
https://leanpub.com/sn1php

Nikita Popov

10 years ago

On Sat, Mar 12, 2016 at 6:54 PM, Paul M. Jones <pmjones88@gmail.com> wrote:

>
> > On Mar 12, 2016, at 11:27, Sara Golemon <pollita@php.net> wrote:
> >
> >> HHVM does not implement "new" in this way, they always evaluate the
> arguments.
> >>
> > Which, if it helps, means that we already know a lot of frameworks
> > /don't/ break as a result of fixing this behavior.
> >
> >> As this is technically a BC affecting change (even if of the lowest
> >> impact), I'm running it past the list first.
> >>
> > Technically a BC break that justifies waiting till a major version.
> > Given the extremely specific requirements to hit this edge case
> > however, passing side-effect args to a non-existent constructor, I
> > personally think a minor version is fine.
>
> Of curiosity, what effect (if any) might this have on
> ReflectionClass::newInstance(), ::newInstanceArgs(), and
> ::newInstanceWithoutConstructor()?  (I ask because some existing DI
> systems, such as Aura.Di, rely on those for creating objects.)

There is no effect on any the ReflectionClass::newInstance*() methods, only
direct use of the "new" operator is affected.

Nikita