Everything

php

BC break: ReflectionMethod::invoke() expects parameter 1 to be object, string given

php.internals

Nicolas Grekas

10 years ago

Hello,

now that the BC break on ReflectionType has been reverted, another one
remains in ReflectionMethod::invoke():

the method doesn't accept a string as first argument anymore, see e.g.:

https://3v4l.org/pImmv

As you can see, this worked since 5.0 and even in HHVM.

It would be great to fix this BC break please.

Regards,
Nicolas

Levi Morrison

10 years ago

On Mon, Aug 22, 2016 at 5:17 AM, Nicolas Grekas <
nicolas.grekas+php@gmail.com> wrote:

> Hello,
>
> now that the BC break on ReflectionType has been reverted, another one
> remains in ReflectionMethod::invoke():
>
> the method doesn't accept a string as first argument anymore, see e.g.:
>
> https://3v4l.org/pImmv
>
> As you can see, this worked since 5.0 and even in HHVM.
>
> It would be great to fix this BC break please.
>
> Regards,
> Nicolas
>

According to the [documentation][1] it requires an object. If the
documentation has not been altered recently to make it this way then I'm
inclined to keep the backward compatibility break. Your example uses a
static method - you should be passing null and not the name of the class
(this is also in the documentation).

[1]: http://php.net/manual/en/reflectionmethod.invoke.php

Pierre Joye

10 years ago

On Aug 22, 2016 9:01 PM, "Levi Morrison" <levim@php.net> wrote:

>
> On Mon, Aug 22, 2016 at 5:17 AM, Nicolas Grekas <
> nicolas.grekas+php@gmail.com> wrote:
>
> > Hello,
> >
> > now that the BC break on ReflectionType has been reverted, another one
> > remains in ReflectionMethod::invoke():
> >
> > the method doesn't accept a string as first argument anymore, see e.g.:
> >
> > https://3v4l.org/pImmv
> >
> > As you can see, this worked since 5.0 and even in HHVM.
> >
> > It would be great to fix this BC break please.
> >
> > Regards,
> > Nicolas
> >
>
> According to the [documentation][1] it requires an object. If the
> documentation has not been altered recently to make it this way then I'm
> inclined to keep the backward compatibility break. Your example uses a
> static method - you should be passing null and not the name of the class
> (this is also in the documentation).
>
>   [1]: http://php.net/manual/en/reflectionmethod.invoke.php

I have to disagree here.

Many codes out there uses string. What is the appealing reason to break
these codes in 7.1?

I think it should restore the precious behavior and if the docs need a
fix,  let fix it, not the other way.

Cheers
Pierre

Ferenc Kovacs

10 years ago

On Mon, Aug 22, 2016 at 4:16 PM, Pierre Joye <pierre.php@gmail.com> wrote:

> On Aug 22, 2016 9:01 PM, "Levi Morrison" <levim@php.net> wrote:
> >
> > On Mon, Aug 22, 2016 at 5:17 AM, Nicolas Grekas <
> > nicolas.grekas+php@gmail.com> wrote:
> >
> > > Hello,
> > >
> > > now that the BC break on ReflectionType has been reverted, another one
> > > remains in ReflectionMethod::invoke():
> > >
> > > the method doesn't accept a string as first argument anymore, see e.g.:
> > >
> > > https://3v4l.org/pImmv
> > >
> > > As you can see, this worked since 5.0 and even in HHVM.
> > >
> > > It would be great to fix this BC break please.
> > >
> > > Regards,
> > > Nicolas
> > >
> >
> > According to the [documentation][1] it requires an object. If the
> > documentation has not been altered recently to make it this way then I'm
> > inclined to keep the backward compatibility break. Your example uses a
> > static method - you should be passing null and not the name of the class
> > (this is also in the documentation).
> >
> >   [1]: http://php.net/manual/en/reflectionmethod.invoke.php
>
> I have to disagree here.
>
> Many codes out there uses string. What is the appealing reason to break
> these codes in 7.1?
>
> I think it should restore the precious behavior and if the docs need a
> fix,  let fix it, not the other way.
>
> Cheers
> Pierre
>

Hi,

If it was explicitly documented to be expecting an object, then altering
the code to match the documented behavior can be considered a bugfix, but I
agree that if it is moderately/widely used we should consider keeping the
old behavior instead of removing it in a minor version.

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

Alexander Lissachenko

10 years ago

Hello, there is one more issue with ReflectionMethod::invoke() for static
methods: it's impossible to call an overridden parent static method and
preserve a scope information:

// https://3v4l.org/ASntq

class foo
{
static function bar()
{
echo self::class, ' vs ', static::class, PHP_EOL;
}

static function bar1()
{
echo self::class, ' vs ', static::class, PHP_EOL;
}
}

class baz extends foo
{
static function bar()
{
echo self::class, ' vs ', static::class, PHP_EOL;
}
}

$r = new ReflectionClass('baz');
$m = $r->getMethod('bar1'); // without overriden method - it's ok, our
static scope is preserved
$m->invoke(null); // foo vs baz

$r = new ReflectionClass('baz');
$m = $r->getMethod('bar'); // with overriden method it's impossible to call
parent static method, no way to specify a scope.
$m->invoke(null); // baz vs baz

// how to statically call the parent static method bar() with scope == baz??

2016-08-22 17:00 GMT+03:00 Levi Morrison <levim@php.net>:

Alexander Lissachenko

10 years ago

If the first argument for invoke() and invokeArgs() for static methods will
be a string with compatible class name for the scope, then it can be used
as a scope modifier (like second argument for Closure::bind($instance,
$scopeClassName))

2016-08-22 17:29 GMT+03:00 Alexander Lisachenko <lisachenko.it@gmail.com>:

Rowan Collins

10 years ago

On 22/08/2016 15:32, Alexander Lisachenko wrote:

> If the first argument for invoke() and invokeArgs() for static methods will
> be a string with compatible class name for the scope, then it can be used
> as a scope modifier (like second argument for Closure::bind($instance,
> $scopeClassName))

The problem with this is that you will only be able to use this with a
version check - in prior versions, the argument will be silently
accepted but ignored. That would make it extremely easy for any code
relying on this to be subtly broken on older builds.

Regards,

--
Rowan Collins
[IMSoP]

Rowan Collins

10 years ago

On 22/08/2016 12:17, Nicolas Grekas wrote:

> in ReflectionMethod::invoke():
>
> the method doesn't accept a string as first argument anymore, see e.g.:
>
> https://3v4l.org/pImmv

To be precise, in prior versions, the first argument was completely
ignored for static methods: https://3v4l.org/8o4Hm

$m->invoke('some_other_class_name', 'baz'); // Warns in 7.1.0beta3,
ignores everywhere else
$m->invoke(42, 'baz'); // Warns in 7.1.0beta3, ignores everywhere else
$m->invoke(new DateTime, 'baz'); // OK in all versions
$m->invoke($m, 'baz'); // OK in all versions

It is still ignored if it is a valid object, so the current beta's
behaviour doesn't actually make a lot of sense. The most logical would
be for it to fail on any value other than null, since for a non-static
method any other object will give "Given object is not an instance of
the class this method was declared in" (except in HHVM, which just
invokes on whatever object you give it): https://3v4l.org/sCFcA

However, simply documenting that "the first argument is ignored for
static methods", and removing the parameter validation, would probably
be sensible for BC if there is likely to be real-world code passing
something other than null.

Regards,

--
Rowan Collins
[IMSoP]

Christoph Becker

10 years ago

On 22.08.2016 at 16:38, Rowan Collins wrote:

> On 22/08/2016 12:17, Nicolas Grekas wrote:
>> in ReflectionMethod::invoke():
>>
>> the method doesn't accept a string as first argument anymore, see e.g.:
>>
>> https://3v4l.org/pImmv
>
> To be precise, in prior versions, the first argument was completely
> ignored for static methods: https://3v4l.org/8o4Hm
>
> $m->invoke('some_other_class_name', 'baz'); // Warns in 7.1.0beta3,
> ignores everywhere else
> $m->invoke(42, 'baz'); // Warns in 7.1.0beta3, ignores everywhere else
> $m->invoke(new DateTime, 'baz'); // OK in all versions
> $m->invoke($m, 'baz'); // OK in all versions
>
> It is still ignored if it is a valid object, so the current beta's
> behaviour doesn't actually make a lot of sense.

Well, see <https://3v4l.org/n8cad>.  Only as of 7.1.0beta3 ::invoke()
matches the behavior of ::invokeArgs().

--
Christoph M. Becker

Rowan Collins

10 years ago

On 22/08/2016 16:40, Christoph M. Becker wrote:

>> It is still ignored if it is a valid object, so the current beta's
>> > behaviour doesn't actually make a lot of sense.
> Well, see <https://3v4l.org/n8cad>.  Only as of 7.1.0beta3 ::invoke()
> matches the behavior of ::invokeArgs().

OK, so that's a further inconsistency. The current version still doesn't
actually make sense, though. If you want to validate that the correct
argument is being passed, then the only value allowed should be null.

> If so desired, I can revert that commit, but I wouldn't be happy with
> sticking with a completely unused parameter, which obviously has been
> and still is misunderstood.

The parameter is still unused, and can be any object. The only sane
validation would be to check explicitly for null, which is documented as
the correct argument for both invoke() and invokeArgs().

Regards,

--
Rowan Collins
[IMSoP]

Christoph Becker

10 years ago

On 22.08.2016 at 13:17, Nicolas Grekas wrote:

> now that the BC break on ReflectionType has been reverted, another one
> remains in ReflectionMethod::invoke():
>
> the method doesn't accept a string as first argument anymore, see e.g.:
>
> https://3v4l.org/pImmv
>
> As you can see, this worked since 5.0 and even in HHVM.
>
> It would be great to fix this BC break please.

The BC break is caused by implementing feature requrest #38992
(<https://bugs.php.net/bug.php?id=38992>), which has been posted nearly
10 years ago.

My analysis as posted in the bug report:

| Actually, the first parameter to ReflectionMethod::invoke() is
| completely ignored, if the method is a static method, see
| <https://3v4l.org/e9h1S>. A comment in the sources[1] also
| explains the reasoning.
|
| This doesn't make any sense, however, because for static methods
| to be invoked, NULL is supposed to be passed as first parameter to
| ::invoke() as well as ::invokeArgs(). Allowing an arbitrary
| argument is confusing at best.
|
| Changing the behavior would introduce a BC break, though, so it
| appears to be reasonable to do that for PHP 7.1 only.
|
| [1]
<https://github.com/php/php-src/blob/PHP-7.0.10/ext/reflection/php_reflection.c#L3197-L3202>

If so desired, I can revert that commit, but I wouldn't be happy with
sticking with a completely unused parameter, which obviously has been
and still is misunderstood.

--
Christoph M. Becker