Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:82535
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.172 as permitted sender)
MIME-Version: 1.0
Date: Thu, 12 Feb 2015 15:24:20 +0100
Message-ID: <CAF+90c8iVRU8-er+05CJqLb7Pg2viR_-zgwf5jmVyfVWx-+-8Q@mail.gmail.com>
To: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c38dac36deee050ee4e0d1
Subject: Remove $this from incompatible context
From: nikita.ppv@gmail.com (Nikita Popov)

--001a11c38dac36deee050ee4e0d1
Content-Type: text/plain; charset=UTF-8

Hi internals,

As part of [1] and [2] we have decided to remove support for doing static
calls to instance methods, if this would result in an incompatible $this
variable to be used. When applying [3] this change we found that the change
as originally intended may be too strict. To provide an example (the
comments are the important bit):

class A {
    // This is an *instance* method, but it doesn't actually use $this.
    // This kind of usage is very common in PHP 4 era code, where
    // "static" annotations weren't used
    function test() {
        echo "foo";
    }
}

class B {
    function test2() {
        // This call would be forbidden because it assumes $this of class
B, when
        // calling an instance method of class A. However A::test() does
not actually
        // use $this!
        A::test();
    }
}

This case, where an incompatible $this will be assumed, but never actually
used, seems to be very common in PEAR, for example. Another weirdness of
forbidding this call is that the following very similar code would work and
only throw an E_STRICT notice:

// NOT in a class
function test3() {
    A::test(); // E_STRICT
}

I don't think having this kind of asymmetry is good. Doing A::test() should
behave the same whether I use it a function, static method or instance
method. (Ignoring scoped call with compatible $this here.)

To remove this asymmetry we could completely forbid calling instance
methods statically (ignoring the case of compatible $this again), but I
suspect this may not be a popular option as this has been only partially
deprecated previously.

The compromise I'd like to implement instead is to only throw a fatal error
if $this is actually used in the called method and otherwise throw a
deprecation warning. Calls from instance methods, static methods and
functions will all be handled the same. A PR with this behavior is
available [4]. To clarify what this entails, a few examples:

class A {
    // Instance method does NOT use $this
    function test() {
        echo "foo";
    }
}

// As such all of the following are valid and only throw a deprecation
warning

class B {
    function test2() {
        A::test(); // E_DEPRECATED
    }
    static function test3() {
        A::test(); // E_DEPRECATED
    }
}
A::test(); // E_DEPRECATED

So in this example $this wasn't used and all calls are valid (but
deprecated). Now an example using $this:

class A {
    // Instance method DOES use $this
    function test() {
        var_dump($this);
    }
}

// As such all of the following are NOT allowed:

class B {
    function test2() {
        A::test(); // E_ERROR
    }
    static function test3() {
        A::test(); // E_ERROR
    }
}
A::test(); // E_ERROR

Whether $this is used is detected at compile-time (by amending the already
existing ALLOW_STATIC flag). Of course we cannot detect variable-variable
usages, in which case $this will be an undefined variable (as it was
already the case for calls from outside instance methods):

class A {
    // Uses $this, but we can't detect it
    function test() {
        $name = 'this';
        var_dump($$this);
    }
}

class B {
    function test2() {
        A::test(); // Throws E_DEPRECATED
        // Followed by E_NOTICE for undefined $this variable
    }
}

Furthermore it should be mentioned what isn't affected at all by this
change: "Static" calls with a compatible $this. E.g. parent::__construct()
will continue to work fine like it already does, without any deprecations
etc.

I think this change makes more sense than the original implementation - it
is symmetric for all static calls and has less BC impact. One case that
will still be broken is code checking whether $this is set [5] to have
different behavior for instance and static calls. However this usage is a
lot more rare and rather questionable in itself.

Any objections to implement the incompatible context removal this way?

Thanks,
Nikita


[1]: https://wiki.php.net/rfc/incompat_ctx
[2]: https://wiki.php.net/rfc/remove_deprecated_functionality_in_php7
[3]:
https://github.com/php/php-src/commit/dc9991b167533c7ee13ff96da6048988e6e31bc2
[4]: https://github.com/php/php-src/pull/1026
[5]: https://github.com/pear/pear-core/blob/stable/PEAR.php#L652

--001a11c38dac36deee050ee4e0d1--