# Strict type declarations not enforced for Reflection API invocation

- Thread: https://externals.io/message/100851
- Original archive: https://news-web.php.net/php.internals/100851
- Started by Sebastian Bergmann on 2017-10-10T13:41:03+00:00
- Messages: 12
- 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

---

## #100851 — Sebastian Bergmann — 2017-10-10T13:41:03+00:00

URL: https://externals.io/message/100851#100851

https://bugs.php.net/bug.php?id=75345 is about the fact that strict type
declarations are not enforced when a function or method is invoked via the
Reflection API.

There is a pull request that addresses this (as well as
https://bugs.php.net/bug.php?id=74750) at
https://github.com/php/php-src/pull/2837.

I consider this a serious bug that leads to unexpected, confusing problems
such as
https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273.

I understand Nikita's point of view (see
https://github.com/php/php-src/pull/2837#issuecomment-335405067) that
changing this behavior (aka. fixing this bug) can be considered a
"non-trivial backwards compatibility break". Therefore I would like to
bring this issue to the attention of this list with this mail.

---

## #100852 — lists@rhsoft.net — 2017-10-10T14:03:45+00:00

URL: https://externals.io/message/100851#100852 · In reply to: #100851

Am 10.10.2017 um 15:41 schrieb Sebastian Bergmann:
> https://bugs.php.net/bug.php?id=75345 is about the fact that strict type
> declarations are not enforced when a function or method is invoked via the
> Reflection API.
> 
> There is a pull request that addresses this (as well as
> https://bugs.php.net/bug.php?id=74750) at
> https://github.com/php/php-src/pull/2837.
> 
> I consider this a serious bug that leads to unexpected, confusing problems
> such as
> https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273.
> 
> I understand Nikita's point of view (see
> https://github.com/php/php-src/pull/2837#issuecomment-335405067) that
> changing this behavior (aka. fixing this bug) can be considered a
> "non-trivial backwards compatibility break". Therefore I would like to
> bring this issue to the attention of this list with this mail

i don't see the argument of "non-trivial backwards compatibility break" 
in case of a major bug - which code should break? the one rely on broken 
bahvior?

that code is already broken in in such cases it should raise errors as 
soon as possible instead get extended and then years later when the 
wrong behavior si fixed the breakage and work to fix bad code relying on 
the old one is even greater

to say it in other words:
if i have written code that relys on wrong behavior i would like to know 
it yesterday and fix it tomorrow instead that i continue to exnted and 
write such code and that with 7.3 or so i have magnitudes more to change 
and adopt

---

## #100854 — Christoph M. Becker — 2017-10-10T14:57:50+00:00

URL: https://externals.io/message/100851#100854 · In reply to: #100851

> I consider this a serious bug that leads to unexpected, confusing problems
> such as
> https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273.
> 
> I understand Nikita's point of view (see
> https://github.com/php/php-src/pull/2837#issuecomment-335405067) that
> changing this behavior (aka. fixing this bug) can be considered a
> "non-trivial backwards compatibility break". Therefore I would like to
> bring this issue to the attention of this list with this mail.

This is most certainly not an *implementation* bug.  The RFC which
introduced strict typing[1] states:

| By default, all PHP files are in weak type-checking mode.

And later:

| This proposal takes the standpoint that it's up to the caller to
| decide how functions should be called.
| […]
| Therefore, this proposal does not allow internal developers to
| “opt-in” to strict typing.

In case of bug #75345 the caller is a method of ReflectionFunction,
which is defined in weak type-checking mode.

One may argue that this is a *design* bug, but after nearly two years of
PHP 7 there may be a lots of code relying on the current behavior, so in
my opinion changing the behavior could not really be regarded as fixing
a bug.

[1] <https://wiki.php.net/rfc/scalar_type_hints_v5>

-- 
Christoph M. Becker

---

## #100855 — lists@rhsoft.net — 2017-10-10T15:10:15+00:00

URL: https://externals.io/message/100851#100855 · In reply to: #100854

Am 10.10.2017 um 16:57 schrieb Christoph M. Becker:
> 
> 
> > […]
> 
> This is most certainly not an *implementation* bug.  The RFC which
> introduced strict typing[1] states:
> 
> | By default, all PHP files are in weak type-checking mode.

yes but the file in question has strict-types enabled
declare(strict_types=1);

> And later:
> 
> | This proposal takes the standpoint that it's up to the caller to
> | decide how functions should be called.
> | […]
> | Therefore, this proposal does not allow internal developers to
> | “opt-in” to strict typing.

yes but the file in question has strict-types enabled
declare(strict_types=1);

> In case of bug #75345 the caller is a method of ReflectionFunction,
> which is defined in weak type-checking mode.

yes but the file in question has strict-types enabled
declare(strict_types=1);

> One may argue that this is a *design* bug, but after nearly two years of
> PHP 7 there may be a lots of code relying on the current behavior, so in
> my opinion changing the behavior could not really be regarded as fixing
> a bug.
> 
> [1] <https://wiki.php.net/rfc/scalar_type_hints_v5>

yes but the file in question has strict-types enabled
declare(strict_types=1);

that decalare line was the explicit wish of the person who wrote the php 
file

---

## #100856 — Niklas Keller — 2017-10-10T16:41:42+00:00

URL: https://externals.io/message/100851#100856 · In reply to: #100855

2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <lists@rhsoft.net>:

>
> Am 10.10.2017 um 16:57 schrieb Christoph M. Becker:
>
> > […]
>
> yes but the file in question has strict-types enabled
> declare(strict_types=1);


Yes, but `array_map` also uses weak types for the callback, like any other
internal function call.

But `call_user_func` is also special-cased, maybe we should do the same
with the reflection calls.

All in all, two typing modes were a bad idea to begin with, mostly because
nobody payed attention to callbacks.

Regards, Niklas

---

## #100857 — lists@rhsoft.net — 2017-10-10T16:45:10+00:00

URL: https://externals.io/message/100851#100857 · In reply to: #100856

Am 10.10.2017 um 18:41 schrieb Niklas Keller:
> 2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <mailto:lists@rhsoft.net> 
> <lists@rhsoft.net <mailto:lists@rhsoft.net>>:
> 
> 
>     Am 10.10.2017 um 16:57 schrieb Christoph M. Becker:
> 
>         
> 
>             I consider this a serious bug that leads to unexpected,
>             confusing problems
>             such as
>             https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273
>             <https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-335180273>.
> 
>             I understand Nikita's point of view (see
>             https://github.com/php/php-src/pull/2837#issuecomment-335405067
>             <https://github.com/php/php-src/pull/2837#issuecomment-335405067>)
>             that
>             changing this behavior (aka. fixing this bug) can be
>             considered a
>             "non-trivial backwards compatibility break". Therefore I
>             would like to
>             bring this issue to the attention of this list with this mail.
> 
> 
>         This is most certainly not an *implementation* bug.  The RFC which
>         introduced strict typing[1] states:
> 
>         | By default, all PHP files are in weak type-checking mode.
> 
> 
>     yes but the file in question has strict-types enabled
>     declare(strict_types=1);
> 
> 
> Yes, but `array_map` also uses weak types for the callback, like any 
> other internal function call.
> 
> But `call_user_func` is also special-cased, maybe we should do the same 
> with the reflection calls.
> 
> All in all, two typing modes were a bad idea to begin with, mostly 
> because nobody payed attention to callbacks

the two typing modes are perfect and when you converted a 15 years old 
codebase to strcit-ytpes and type-hints everywehere you know why - first 
you start with typehints and you can't do that strict or you will burn 
out and give up

but when i define strict types in a PHP file everything but code in 
includes has to be strict typed inherited

---

## #100858 — Nikita Popov — 2017-10-10T16:53:47+00:00

URL: https://externals.io/message/100851#100858 · In reply to: #100851

On Tue, Oct 10, 2017 at 3:41 PM, Sebastian Bergmann <sebastian@php.net>
wrote:

> https://bugs.php.net/bug.php?id=75345 is about the fact that strict type
> declarations are not enforced when a function or method is invoked via the
> Reflection API.
>
> There is a pull request that addresses this (as well as
> https://bugs.php.net/bug.php?id=74750) at
> https://github.com/php/php-src/pull/2837.
>
> I consider this a serious bug that leads to unexpected, confusing problems
> such as
> https://github.com/sebastianbergmann/phpunit/issues/2796#issuecomment-
> 335180273.
>
> I understand Nikita's point of view (see
> https://github.com/php/php-src/pull/2837#issuecomment-335405067) that
> changing this behavior (aka. fixing this bug) can be considered a
> "non-trivial backwards compatibility break". Therefore I would like to
> bring this issue to the attention of this list with this mail.
>

To repeat what I said in a comment on the pull request: I think this is
fixing the wrong issue.

The problem are not internal function calls, the problem are callbacks. In
fact, the proposed fix does not actually fix the problem you encountered in
PHPUnit, as it is going to use the strictness mode at the reflection
call-site, not the strictness mode used by the file defining the data
provider.

The proposed patch is going to add a discrepancy between internal and
userland code. It means that the internal array_map function will execute
the callback using the strictness mode of the array_map caller, while a
userland reimplementation of the same function will not be able to match
that behavior and always either perform the call strictly or weakly,
independent of the array_map caller.

I believe that the proper way to fix this is to handle dynamic function
invocations differently from direct invocations. Direct invocations should
use the strictness level of the call-site, while dynamic invocations should
use the strictness level of the declaration-site. This means that the
internal array_map and a userland reimplementation will behave consistent.
It also means that PHPUnit will be able to respect the strictness level of
file defining the data provider, etc. I've seen a number of complaints
about our handling of callbacks wrt strict_types, so I think it's
worthwhile to at least consider making such a change.

Regards,
Nikita

---

## #100859 — Christoph M. Becker — 2017-10-10T17:11:50+00:00

URL: https://externals.io/message/100851#100859 · In reply to: #100856

> 2017-10-10 17:10 GMT+02:00 lists@rhsoft.net <lists@rhsoft.net>:

> All in all, two typing modes were a bad idea to begin with, mostly because
> nobody payed attention to callbacks.

"nobody" is not quite right.  If I remember correctly, this issue has
first been brought up on this list by Benjamin Eberlei[1], and it has
been discussed.

[1] <http://news.php.net/php.internals/82619>

-- 
Christoph M. Becker

---

## #100860 — Sebastian Bergmann — 2017-10-11T05:53:40+00:00

URL: https://externals.io/message/100851#100860 · In reply to: #100858

Am 10.10.2017 um 18:53 schrieb Nikita Popov:
> The problem are not internal function calls, the problem are callbacks. In
> fact, the proposed fix does not actually fix the problem you encountered in
> PHPUnit, as it is going to use the strictness mode at the reflection
> call-site, not the strictness mode used by the file defining the data
> provider.

I was aware of that (I only saw the pull request but did not look at it),
thanks for clarifying.

> I believe that the proper way to fix this is to handle dynamic function
> invocations differently from direct invocations. Direct invocations should
> use the strictness level of the call-site, while dynamic invocations should
> use the strictness level of the declaration-site. This means that the
> internal array_map and a userland reimplementation will behave consistent.
> It also means that PHPUnit will be able to respect the strictness level of
> file defining the data provider, etc. I've seen a number of complaints
> about our handling of callbacks wrt strict_types, so I think it's
> worthwhile to at least consider making such a change.

I think this makes sense. Is this something that can be achieved for PHP
7.2 or does it have to wait for PHP 7.3?

---

## #100861 — mail@pmmaga.net — 2017-10-11T09:03:21+00:00

URL: https://externals.io/message/100851#100861 · In reply to: #100858

>
> The problem are not internal function calls, the problem are callbacks. In
> fact, the proposed fix does not actually fix the problem you encountered in
> PHPUnit, as it is going to use the strictness mode at the reflection
> call-site, not the strictness mode used by the file defining the data
> provider.
>

For those that didn't have a look at the PR, my goal was to try to ensure
that if we are looking for which strictness to use, we should look at the
place of the closest "user" call instead of the function (user or internal)
that called the current one. The reason for this problem lies in the fact
that `ZEND_ARG_USES_STRICT_TYPES()` will simply look at the caller
regardless of what it is. Although this is enough to allow calling
functions that were defined as non-strict in a strict manner, it does cause
this kind of shortcomings with callbacks.


> I believe that the proper way to fix this is to handle dynamic function
> invocations differently from direct invocations. Direct invocations should
> use the strictness level of the call-site, while dynamic invocations should
> use the strictness level of the declaration-site.


I agree that this should be about dynamic vs direct instead of user vs
internal but IMHO, making the strictness vary from call-site to
declaration-site depending on that may be a bit too confusing. Would it be
possible/reasonable to try to find the place where the dynamic call was
started? (i.e. the dataProvider, the mapped function for a userland
array_map, etc...)

Regards,
Pedro

---

## #100862 — Niklas Keller — 2017-10-11T11:48:31+00:00

URL: https://externals.io/message/100851#100862 · In reply to: #100861

2017-10-11 11:03 GMT+02:00 Pedro Magalhães <mail@pmmaga.net>:

> On Tue, Oct 10, 2017 at 5:53 PM, Nikita Popov <nikita.ppv@gmail.com>
> wrote:
> > […]
> In
> > […]
> in
> > […]
>
> For those that didn't have a look at the PR, my goal was to try to ensure
> that if we are looking for which strictness to use, we should look at the
> place of the closest "user" call instead of the function (user or internal)
> that called the current one. The reason for this problem lies in the fact
> that `ZEND_ARG_USES_STRICT_TYPES()` will simply look at the caller
> regardless of what it is. Although this is enough to allow calling
> functions that were defined as non-strict in a strict manner, it does cause
> this kind of shortcomings with callbacks.
>
>
> > […]
> should
> > […]
> should
> > […]
>

I'm not sure about that. That might be reasonable for closures, but not for
other dynamic invocations.


> I agree that this should be about dynamic vs direct instead of user vs
> internal but IMHO, making the strictness vary from call-site to
> declaration-site depending on that may be a bit too confusing. Would it be
> possible/reasonable to try to find the place where the dynamic call was
> started? (i.e. the dataProvider, the mapped function for a userland
> array_map, etc...)
>

I don't think that's reasonably possible. Think about some event emitter
like that:

```
class EventEmitter {
    private array $callbacks = [];

    public function on(string $eventName, callable $callback): void {
        $this->callbacks[$eventName][] = $callback;
    }

    public function emit(string $eventName, $value): void {
        foreach ($this->callbacks[$eventName] ?? [] as $callback) {
            $callback($value);
        }
    }
}
```

Where is a dynamic call to a registered callback started? Depending on the
strictness level of the caller of `emit()`? What if that `emit()` is called
from another file with another strictness level?

Regards, Niklas

---

## #100914 — Andrea Faulds — 2017-10-22T19:13:55+00:00

URL: https://externals.io/message/100851#100914 · In reply to: #100858

Hi Nikita,

Nikita Popov wrote:
> I believe that the proper way to fix this is to handle dynamic function
> invocations differently from direct invocations. Direct invocations should
> use the strictness level of the call-site, while dynamic invocations should
> use the strictness level of the declaration-site. This means that the
> internal array_map and a userland reimplementation will behave consistent.
> It also means that PHPUnit will be able to respect the strictness level of
> file defining the data provider, etc. I've seen a number of complaints
> about our handling of callbacks wrt strict_types, so I think it's
> worthwhile to at least consider making such a change.

I agree that callbacks are one of the rough edges of strict_types. I've 
always been hoping we'd get some type of callable typing as a means of 
solving that, but it hasn't happened yet.

As for your specific suggestion, I don't see the benefit of it. If 
anything, it could make one particular problem worse, namely that of 
libraries not written with type-hinted callbacks in mind. Calls to such 
libraries are more likely to blow up if suddenly those calls are made 
with strict types because the callbacks were declared in a strict_types 
file. I also dislike the inconsistency of making dynamic calls work 
differently.

Thanks.
-- 
Andrea Faulds
https://ajf.me/