Everything

php

[RFC] Forbid $this being null in instance methods

php.internals

Nikita Popov

10 years ago

Hi internals!

I'd like to submit a small RFC for your consideration, which ensures
something that really ought to be a given already: If you use $this in an
instance method, you should actually get a $this (and not NULL).

https://wiki.php.net/rfc/forbid_null_this_in_methods

As this is targeting PHP 7.1, the RFC is careful to retain compatibility
with certain legacy PHP 4 code patterns.

Thanks,
Nikita

Bishop Bettini

10 years ago

On Fri, Apr 29, 2016 at 1:12 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> Hi internals!
>
> I'd like to submit a small RFC for your consideration, which ensures
> something that really ought to be a given already: If you use $this in an
> instance method, you should actually get a $this (and not NULL).
>
>     https://wiki.php.net/rfc/forbid_null_this_in_methods
>
> As this is targeting PHP 7.1, the RFC is careful to retain compatibility
> with certain legacy PHP 4 code patterns.
>

What is the performance cost of inspecting the method for $this, and is
that inspection impervious to tricks that shield $this?

Example:

A::method();
class A {
public function method() {
$foo = function () {
$this->something();
}
}
}

Does that throw an \Error?  Would it throw an \Error if $foo() were called?

Stas Malyshev

10 years ago

Hi!

> I'd like to submit a small RFC for your consideration, which ensures
> something that really ought to be a given already: If you use $this in an
> instance method, you should actually get a $this (and not NULL).
>
>     https://wiki.php.net/rfc/forbid_null_this_in_methods
>
> As this is targeting PHP 7.1, the RFC is careful to retain compatibility
> with certain legacy PHP 4 code patterns.

Unfortunately, this does not exactly preserve compatibility. Consider:

public function legacyMethod() {
if(is_null($this)) {
trigger_error(E_DEPRECATED, "legacyMethod() should be called with an
object now!");
return self::getDefaultInstance()->legacyMethod();
}
print "I am an ugly legacy method!";
$this->doSomeStuff();
return true;
}

Unfortunately, I've seen this pattern not once when dealing with APIs
which were grown from PHP 4 code and must maintain BC - e.g. have code
relying on legacyMethod() both being called statically and called on an
object and legacyMethod() being overridden by extending classes. It is
an ugly code and an ugly situation to be in, but it is a real pattern
that happens.

Despite the RFC claiming HHVM does not support this pattern, it does:
https://3v4l.org/1QrY7
Unlike PHP, it doesn't even throw deprecated error in that case.

Given that this RFC does not seem to enable anyone to do anything
additional that couldn't be done before, just removes the features from
language, I see any advantage in doing it so far.

I also do not think enabling using $this as a regular variable is a
particularly good idea. It does not add anything - there's no real need
to name your variables specifically "$this", you can choose any other
name - but it creates potential both for confusion and for weird errors
when you refactor stuff. Again, don't see any real advantage in this.

--
Stas Malyshev
smalyshev@gmail.com

Nikita Popov

10 years ago

On Fri, Apr 29, 2016 at 9:10 PM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi!
>
> > I'd like to submit a small RFC for your consideration, which ensures
> > something that really ought to be a given already: If you use $this in an
> > instance method, you should actually get a $this (and not NULL).
> >
> >     https://wiki.php.net/rfc/forbid_null_this_in_methods
> >
> > As this is targeting PHP 7.1, the RFC is careful to retain compatibility
> > with certain legacy PHP 4 code patterns.
>
> Unfortunately, this does not exactly preserve compatibility. Consider:
>
> public function legacyMethod() {
>   if(is_null($this)) {
>     trigger_error(E_DEPRECATED, "legacyMethod() should be called with an
> object now!");
>     return self::getDefaultInstance()->legacyMethod();
>   }
>   print "I am an ugly legacy method!";
>   $this->doSomeStuff();
>   return true;
> }
>
> Unfortunately, I've seen this pattern not once when dealing with APIs
> which were grown from PHP 4 code and must maintain BC - e.g. have code
> relying on legacyMethod() both being called statically and called on an
> object and legacyMethod() being overridden by extending classes. It is
> an ugly code and an ugly situation to be in, but it is a real pattern
> that happens.
>
> Despite the RFC claiming HHVM does not support this pattern, it does:
> https://3v4l.org/1QrY7
> Unlike PHP, it doesn't even throw deprecated error in that case.
>

Wow, thanks for pointing this out! I'm really stumped as to why I thought
HHVM doesn't support it. I've dropped the remark now.

> Given that this RFC does not seem to enable anyone to do anything
> additional that couldn't be done before, just removes the features from
> language, I see any advantage in doing it so far.
>
> I also do not think enabling using $this as a regular variable is a
> particularly good idea. It does not add anything - there's no real need
> to name your variables specifically "$this", you can choose any other
> name - but it creates potential both for confusion and for weird errors
> when you refactor stuff. Again, don't see any real advantage in this.
>

The thing is that currently we allow using $this as a normal variable in
some ways (accepted as a function argument for example), but then don't
allow it in others (accessing properties on it). However, I agree that this
is probably the wrong direction to go here, instead we should make sure
that $this cannot be used at all. I will consider this.

Thanks,
Nikita