Remove $this from incompatible context

10 years ago by Nikita Popov — view source — reply

unread

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

10 years ago by francois@tekwire.net — view source — reply

unread

De : Nikita Popov [mailto:nikita.ppv@gmail.com]

class B {
function test2() {
A::test(); // Throws E_DEPRECATED
// Followed by E_NOTICE for undefined $this variable
}
}

Sorry to get off-topic but could we raise the 'undefined variable' error to E_WARNING, at least ? E_NOTICE seems very low for such an error.

Otherwise, +1 for your proposal. It's much better.

Regards

François

10 years ago by Rowan Collins — view source — reply

unread

François Laupretre wrote on 12/02/2015 14:56:

Sorry to get off-topic but could we raise the 'undefined variable' error to E_WARNING, at least ? E_NOTICE seems very low for such an error.

I think the division between E_NOTICE and E_WARNING is a bit arbitrary
sometimes, but without a good definition of the difference, I think it's
unhelpful to introduce "inflation" by raising severities. Don't forget
that while an undefined variable is sometimes an error, it's sometimes
just poor coding style, and the user is consciously relying on the
implicit null (e.g. $count++, or $hash[$key] = $value, or even echo
$hash[$key]).

Perhaps what's needed is a survey of current messages, and an RFC to
reclassify them based on some consistent definitions?

Regards,

Rowan Collins
[IMSoP]

10 years ago by Kris Craig — view source — reply

unread

On Thu, Feb 12, 2015 at 8:53 AM, Rowan Collins rowan.collins@gmail.com
wrote:

François Laupretre wrote on 12/02/2015 14:56:

Sorry to get off-topic but could we raise the 'undefined variable' error
to E_WARNING, at least ? E_NOTICE seems very low for such an error.

I think the division between E_NOTICE and E_WARNING is a bit arbitrary
sometimes, but without a good definition of the difference, I think it's
unhelpful to introduce "inflation" by raising severities. Don't forget that
while an undefined variable is sometimes an error, it's sometimes just poor
coding style, and the user is consciously relying on the implicit null
(e.g. $count++, or $hash[$key] = $value, or even echo $hash[$key]).

Perhaps what's needed is a survey of current messages, and an RFC to
reclassify them based on some consistent definitions?

Regards,

Rowan Collins
[IMSoP]

--

I would also like to see undefined variables escalated to E_WARNING. Even
though it is often bad coding style, it's also frequently just an oversight
on the part of the developer; an oversight that can lead to unexpected
results. Since it's very common for environments to display E_WARNING but
not E_NOTICE, some devs-- particularly those who are less experienced in
PHP-- can end up wasting a lot of time trying to chase down the behavior
because they're not seeing any errors.

Besides, making it only an E_NOTICE, we're encouraging people to develop
bad habits that can and do lead to all kinds of problems down the line. I
know many devs who erroneously believe that declaring variables is actually
discouraged in PHP. When I point out that not declaring a variable will
throw a notice, they always seem surprised; something along the lines of,
"Oh yeah I never see those."

Given the problems that can arise from this, I think that's reasonable
justification enough to make this an E_WARNING instead of E_NOTICE.

--Kris

10 years ago by Rowan Collins — view source — reply

unread

Kris Craig wrote on 12/02/2015 21:45:

Since it's very common for environments to display E_WARNING but not
E_NOTICE, some devs-- particularly those who are less experienced in
PHP-- can end up wasting a lot of time trying to chase down the
behavior because they're not seeing any errors.

If people have turned off E_NOTICE, there is presumably something in
that level that they don't want to see; giving them that choice is the
whole point of having multiple reporting levels in the first place.
Without a clear definition of what should be in each level, there's a
risk of inflation setting in, with advocates of each message saying why
people should see it even if they have E_NOTICE turned off.

The alternative, which I've tried to picture before, is a more
customisable system based on types of message rather than severity - so
that a user who has a lot of $foo[$new_key][$new_sub_key] = $value in
their code can suppress E_ACCESSING_NONEXISTENT_ARRAY_KEY, and a user
who has a lot of $foo[unquote_key] can suppress
E_UNDECLARED_CONSTANT_USED_AS_STRING (obviously with better names than
that!). That way important messages which you definitely want to fix
don't get drowned in the noise of things which you know you should get
around to fixing one day but are working as designed right now.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Rowan Collins — view source — reply

unread

Nikita Popov wrote on 12/02/2015 14:24:

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.

[snip]

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.

[snip]

I'm definitely in favour of this over completely forbidding such calls.

The only thing I'd question is the use of E_DEPRECATED when the static
call doesn't use $this. Is there actually any intent to change the
behaviour in a future version? What is the advantage to doing so? It
strikes me as rather like the case of "var" for properties, which had
the deprecation notice removed.

I think it would make more sense to raise an E_STRICT, since the main
purpose seems to be to discourage users making such calls.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Nikita Popov — view source — reply

unread

On Thu, Feb 12, 2015 at 5:45 PM, Rowan Collins rowan.collins@gmail.com
wrote:

Nikita Popov wrote on 12/02/2015 14:24:

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.

[snip]

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.

[snip]

I'm definitely in favour of this over completely forbidding such calls.

The only thing I'd question is the use of E_DEPRECATED when the static
call doesn't use $this. Is there actually any intent to change the
behaviour in a future version? What is the advantage to doing so? It
strikes me as rather like the case of "var" for properties, which had the
deprecation notice removed.

I think it would make more sense to raise an E_STRICT, since the main
purpose seems to be to discourage users making such calls.

The reason is that previously it was using E_DEPRECATED in some places and
E_STRICT in others. I'd like to have a consistent error level in all cases,
so using E_DEPRECATED everywhere (I am not willing to downgrade existing
deprecations to strict warnings).

And yes, I'm pretty sure there is intent to remove this completely in the
future - this is just another leftover from PHP 4. The approach outlined
here only avoids dropping support in PHP 7 already (which the original
patch did for all practical purposes).

Nikita

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

And yes, I'm pretty sure there is intent to remove this completely in the
future - this is just another leftover from PHP 4. The approach outlined
here only avoids dropping support in PHP 7 already (which the original
patch did for all practical purposes).

Why btw remove it? OK, somebody did not mark the function as "static",
but if it doesn't use $this, what's the difference? It certainly won't
produce any undesirable behavior, so why not just let it work as if it
was declared "static"? Is there any practical reason not to?

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Ferenc Kovacs — view source — reply

unread

On Thu, Feb 12, 2015 at 5:45 PM, Rowan Collins rowan.collins@gmail.com
wrote:

Nikita Popov wrote on 12/02/2015 14:24:

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.

[snip]

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.

[snip]

I'm definitely in favour of this over completely forbidding such calls.

The only thing I'd question is the use of E_DEPRECATED when the static
call doesn't use $this. Is there actually any intent to change the
behaviour in a future version? What is the advantage to doing so? It
strikes me as rather like the case of "var" for properties, which had the
deprecation notice removed.

I think it would make more sense to raise an E_STRICT, since the main
purpose seems to be to discourage users making such calls.

The reason is that previously it was using E_DEPRECATED in some places and
E_STRICT in others. I'd like to have a consistent error level in all cases,
so using E_DEPRECATED everywhere (I am not willing to downgrade existing
deprecations to strict warnings).

And yes, I'm pretty sure there is intent to remove this completely in the
future - this is just another leftover from PHP 4. The approach outlined
here only avoids dropping support in PHP 7 already (which the original
patch did for all practical purposes).

Nikita

adding the E_DEPRECATED was only about the assuming $this behavior:
https://wiki.php.net/rfc/incompat_ctx
so if we remove that behavior ($this is set to null) then I'm not sure why
shouldn't we leave the calling non-static method as static as E_STRICT.
are we planning to remove the ability to call non-static methods statically
later on? that would be against the original proposal:
"This proposal is not about removing static calls to instance methods or
instance-like calls to static methods."
and the original plan for removing this behavior was simply to remove/not
assume $this:
"Even though I think an error at call site would be the most useful to the
user, the most sensible option is to just have $this === null inside the
callee, like when you do"

I think the best course of action would be going with the original plan,
null-ing out $this and keeping the calling a non static method statically
as an E_STRICT.

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

10 years ago by Nikita Popov — view source — reply

unread

adding the E_DEPRECATED was only about the assuming $this behavior:
https://wiki.php.net/rfc/incompat_ctx

While the RFC may claim that, the way it was implemented will throw
E_DEPRECATED irregardless whether $this is used or not. As such all static
calls to non-static methods will currently throw a deprecation, if the call
happens from a non-static method. In practice you can leave off that last
restriction, because in codebases that don't use "static" annotations for
PHP 4 compatibility obviously all methods are non-static, so always
E_DEPRECATED will be thrown instead of E_STRICT.

Reverting this to E_STRICT means we undeprecate this again, which is
something I am certainly against.

and the original plan for removing this behavior was simply to remove/not
assume $this:
"Even though I think an error at call site would be the most useful to the
user, the most sensible option is to just have $this === null inside the
callee, like when you do"

The RFC is not very clear on what is supposed to be implemented. I'm
proposing to go with the option "most useful to the user" here, while you
suggest to use what's referred to as "the most sensible option".

However, I am not willing to invest more time in this issue. I have decided
that I will implement this by nulling out $this and keeping the error
levels exactly as they are right now (a mix of E_STRICT and E_DEPRECATED).
I would have liked consistent error levels and I would have really liked
being nice to the user and giving him a error at the call site, instead of
a weird undefined variable notice which is even suppressed by default. But
in the end this is just not important enough for me to put up with all the
php internals crap.

Nikita

10 years ago by Rowan Collins — view source — reply

unread

Nikita Popov wrote on 12/02/2015 23:33:

On Fri, Feb 13, 2015 at 12:06 AM, Ferenc Kovacs <tyra3l@gmail.com
mailto:tyra3l@gmail.com> wrote:
adding the `E_DEPRECATED` was only about the assuming $this
behavior: https://wiki.php.net/rfc/incompat_ctx
While the RFC may claim that, the way it was implemented will throw
E_DEPRECATED irregardless whether $this is used or not. As such all
static calls to non-static methods will currently throw a deprecation,
if the call happens from a non-static method. In practice you can
leave off that last restriction, because in codebases that don't use
"static" annotations for PHP 4 compatibility obviously all methods are
non-static, so always E_DEPRECATED will be thrown instead of E_STRICT.

Reverting this to E_STRICT means we undeprecate this again, which is
something I am certainly against.
and the original plan for removing this behavior was simply to
remove/not assume $this:
"Even though I think an error at call site would be the most
useful to the user, the most sensible option is to just have $this
=== null inside the callee, like when you do"
The RFC is not very clear on what is supposed to be implemented. I'm
proposing to go with the option "most useful to the user" here, while
you suggest to use what's referred to as "the most sensible option".

However, I am not willing to invest more time in this issue. I have
decided that I will implement this by nulling out $this and keeping
the error levels exactly as they are right now (a mix of E_STRICT and
E_DEPRECATED). I would have liked consistent error levels and I would
have really liked being nice to the user and giving him a error at the
call site, instead of a weird undefined variable notice which is even
suppressed by default. But in the end this is just not important
enough for me to put up with all the php internals crap.

I'm sorry this discussion has become so frustrating for you. Personally,
I think consistent errors would be great; and I agree that the aim
should be to give the user a helpful notice.

Whether that should be E_DEPRECATED or E_STRICT is a minor detail, and
depends on us predicting the future. My personal prediction is that this
will go the way of "var", and remain as a discouraged but supported
feature, but it's really not that big a deal.

Regards,

Rowan Collins
[IMSoP]

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

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();
}
}

IMHO, this should work. Maybe issue E_STRICT, but even then I'd think
it's not really necessary, I can't see any purpose E_STRICT serves in
this case - since $this is not used, no potential bug is averted, as the
code works exactly as it was supposed to be working.

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
}

Yes, this should work too, and I'm again not sure how useful is E_STRICT
even here.

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

I would go even further and if $this is not there just let it work.
Assuming $this from different context is broken, but if $this not
involved, what's the harm? The call to different context should just
drop $this and treat it as if it were a call from global scope.

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.

This should just behave as if $this is undefined there.

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Nikita Popov — view source — reply

unread

On Thu, Feb 12, 2015 at 10:31 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

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();
}
}

IMHO, this should work. Maybe issue E_STRICT, but even then I'd think
it's not really necessary, I can't see any purpose E_STRICT serves in
this case - since $this is not used, no potential bug is averted, as the
code works exactly as it was supposed to be working.

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
}

Yes, this should work too, and I'm again not sure how useful is E_STRICT
even here.

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

I would go even further and if $this is not there just let it work.
Assuming $this from different context is broken, but if $this not
involved, what's the harm? The call to different context should just
drop $this and treat it as if it were a call from global scope.

It was always an E_STRICT error to call a non-static method statically (as
usual, not counting scoped calls). I don't see why we would suddenly want
to go back on this and make it work without any warnings.

Nikita

10 years ago by Nikita Popov — view source — reply

unread

On Thu, Feb 12, 2015 at 10:31 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:

Hi!

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();
}
}

IMHO, this should work. Maybe issue E_STRICT, but even then I'd think
it's not really necessary, I can't see any purpose E_STRICT serves in
this case - since $this is not used, no potential bug is averted, as the
code works exactly as it was supposed to be working.

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
}

Yes, this should work too, and I'm again not sure how useful is E_STRICT
even here.

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

I would go even further and if $this is not there just let it work.
Assuming $this from different context is broken, but if $this not
involved, what's the harm? The call to different context should just
drop $this and treat it as if it were a call from global scope.

It was always an E_STRICT error to call a non-static method statically (as
usual, not counting scoped calls) and since PHP 5.6 it was E_DEPRECATED in
the most common case. I don't see why we would suddenly want to go back on
this and make it work without any warnings.

Nikita

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

It was always an E_STRICT error to call a non-static method statically
(as usual, not counting scoped calls) and since PHP 5.6 it was
E_DEPRECATED in the most common case. I don't see why we would suddenly
want to go back on this and make it work without any warnings.

The reason why is simple - it works, it doesn't produce any bad
behavior, it's clear what it does, so why produce warnings that are
essentially useless to the user as they don't warn them about any
potential problem? Unless, of course, I'm missing some potential problem

please point it out then. But "we've always done it this way" is not
particularly good argument in this case - we did many things one way and
then changed and started doing it a better way.

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Marc Bennewitz — view source — reply

unread

Hi Stas,

Am 12.02.2015 um 22:31 schrieb Stanislav Malyshev:

Hi!

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();
}
}
IMHO, this should work. Maybe issue E_STRICT, but even then I'd think
it's not really necessary, I can't see any purpose E_STRICT serves in
this case - since $this is not used, no potential bug is averted, as the
code works exactly as it was supposed to be working.

Such code will break, not in the first place but later on!
You propose that every instance method not using the variable $this
internally will be magically a static method and can never ever be
changed to use $this without an bc break.

<snip>

Marc

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

Such code will break, not in the first place but later on!

Later on when?

You propose that every instance method not using the variable $this
internally will be magically a static method and can never ever be

The difference between static method and non-static method is pretty
much this - the former can't use $this.

changed to use $this without an bc break.

I don't think you use "bc break" in a meaning it is usually used.
Usually, "bc break" means the user code does not change, but the
language or environment changes and code that worked before doesn't work
anymore. That's bad. But you seem to mean that if the user changes the
code to do a different thing than it did before, the code may not be
correct anymore. That's only natural.

Stas Malyshev
smalyshev@gmail.com

10 years ago by Yasuo Ohgaki — view source — reply

unread

Hi Nikita,

Any objections to implement the incompatible context removal this way?

It was long time question for me allowing the code you would like to remove.
If these are removed altogether(make them illegal), it would be good for
users
in the long run. This is my opinion for this.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

there should be no bc break as the API doesn't change and the method
produces the exact same result as before.

Sorry, this makes no sense to me. You claim that if you changed the
method code to do different thing it should continue working as if you
didn't change it? Why? I just don't get it.

It's not a good thing to magically change the method API in base of a
method body that's not port of the API.

"magically change the method API"? What are you talking about? You
changed the code, not magic.

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Marc Bennewitz — view source — reply

unread

Hi
Am 13.02.2015 um 08:48 schrieb Stanislav Malyshev:

Hi!

there should be no bc break as the API doesn't change and the method
produces the exact same result as before.
Sorry, this makes no sense to me. You claim that if you changed the
method code to do different thing it should continue working as if you
didn't change it? Why? I just don't get it.

It's not a good thing to magically change the method API in base of a
method body that's not port of the API.
"magically change the method API"? What are you talking about? You
changed the code, not magic.

Sorry it wasn't clear. I hope I will now:

The static modifier for methods is part of the method signature and
method body isn't.
(That's way interfaces doesn't describe method bodies but signatures)

The static modifier defines a method as static and therefore defines the
method is callable using "::".

What I mean with "magically" is that you like to define a method as
static without the static modifier in base of the method body but the
body isn't part of the signature.

Now if you change the method body only without this very special
knowledge into something using $this you break all code that calls the
method statically.

So for me a +1 to throw an E_DEPRECATED error in this case and remove
this feature in PHP 8. This will inform existing applications about the
wrong call and provide enough time to be fixed.

Marc

10 years ago by Rowan Collins — view source — reply

unread

Hi
Am 13.02.2015 um 08:48 schrieb Stanislav Malyshev:

Hi!

there should be no bc break as the API doesn't change and the method
produces the exact same result as before.
Sorry, this makes no sense to me. You claim that if you changed the
method code to do different thing it should continue working as if
you
didn't change it? Why? I just don't get it.

It's not a good thing to magically change the method API in base of
a
method body that's not port of the API.
"magically change the method API"? What are you talking about? You
changed the code, not magic.

Sorry it wasn't clear. I hope I will now:

The static modifier for methods is part of the method signature and
method body isn't.
(That's way interfaces doesn't describe method bodies but signatures)

The static modifier defines a method as static and therefore defines
the
method is callable using "::".

More specifically, it defines that the method is only callable statically. There is currently no opposite keyword to say that a method can only be called non-statically, but the absence of a modifier has up until now meant that either call type is possible. An interface cannot currently forbid static calls, only the use of $this.

Our choice is between keeping that ambiguity, and introducing a new rule that any method not marked static must never be called statically (with provisos for technically non-static parent:: etc).

Regards,

Rowan Collins
[IMSoP]

10 years ago by Marc Bennewitz — view source — reply

unread

Am 15.02.2015 um 19:09 schrieb Rowan Collins:

<snip>
The static modifier for methods is part of the method signature and
method body isn't.
(That's way interfaces doesn't describe method bodies but signatures)

The static modifier defines a method as static and therefore defines
the
method is callable using "::".
More specifically, it defines that the method is only callable statically. There is currently no opposite keyword to say that a method can only be called non-statically, but the absence of a modifier has up until now meant that either call type is possible. An interface cannot currently forbid static calls, only the use of $this.

Our choice is between keeping that ambiguity, and introducing a new rule that any method not marked static must never be called statically (with provisos for technically non-static parent:: etc).

Regards,

The problem with the current behavior of "... the absence of a modifier
has up until now meant that either call type is possible" is that the
library author can't define a method as non static callable and the
caller defines if $this can be used.

As a simple example (again). A library defines a class like that:

class A {
protected $base = 1;
public function doSomething() {
return $this->base * 2;
}
public function doSomethingMore() {
return (2 * 2) + 4;
}
}

Now you notes a bug in "doSomethingMore()" as it calculates a wrong
number and you go to fix that into the following:

class A {
protected $base = 1;
public function doSomething() {
return $this->base * 2;
}
public function doSomethingMore() {
return $this->doSomething() + 4;
}
}

You have done a simple bugfix but this kind of change generates a big bc
brea if someone calls A::doSomethingMore().

The message of the current behavior is "Hey you calls the function
wrongly. This works but it's not safe." but the message isn't
consistent. The change of Nikita makes the massage consistent and that
is a very good step forward. Additionally Nikita is going to change the
message a little into "Hey you calls the function wrongly. This works
but it's not safe AND WILL BE REMOVED IN THE FUTURE". It's something I
like because the issue described above will be catched earlier and the
caller have to make it right from the beginning.

Removing the message would mean the library author has the fault here
because it would mean the caller does it right and it's allowed and
common in PHP to call a method statically even if it wasn't defined as
it. The library author would end up in something hackery to use the
variable $this without any need only to define a method as non static.

PS: A static call of parent::doSomething() and self::doSomething() from
an instance context is something special because it's widely used in PHP
and changing this would be a big bc break. (Personally I would like to
see something like parent->doSomething() and self->doSomething() would
work, too)

Thanks
Marc

10 years ago by Rowan Collins — view source — reply

unread

Am 15.02.2015 um 19:09 schrieb Rowan Collins:

<snip>
The static modifier for methods is part of the method signature and
method body isn't.
(That's way interfaces doesn't describe method bodies but signatures)

The static modifier defines a method as static and therefore defines
the
method is callable using "::".
More specifically, it defines that the method is only callable
statically. There is currently no opposite keyword to say that a
method can only be called non-statically, but the absence of a
modifier has up until now meant that either call type is possible. An
interface cannot currently forbid static calls, only the use of $this.

Our choice is between keeping that ambiguity, and introducing a new
rule that any method not marked static must never be called
statically (with provisos for technically non-static parent:: etc).

Regards,

The problem with the current behavior of "... the absence of a
modifier has up until now meant that either call type is possible" is
that the library author can't define a method as non static callable
and the caller defines if $this can be used.

Agreed. Personally, I'm a great fan of symettry, so part of me would
prefer to introduce a keyword for when you can't call a method
statically, e.g. "instance function foo() { ... }". But I guess instance
methods are, or at least "should" be, the majority, so it makes sense to
reserve the un-annotated form for them.

I worry about how many people will still miss the deprecation warning,
but then I'm a broken record on the subject of PHP's warning system
needing an overhaul.

Regards,

--
Rowan Collins
[IMSoP]

10 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

Agreed. Personally, I'm a great fan of symettry, so part of me would
prefer to introduce a keyword for when you can't call a method
statically, e.g. "instance function foo() { ... }". But I guess instance

This would be another form of saying "foo() method uses $this", but if
that's true then once it uses $this you'd get the error anyway, so not
much changes from adding "instance".

--
Stas Malyshev
smalyshev@gmail.com

10 years ago by Rowan Collins — view source — reply

unread

Stanislav Malyshev wrote on 16/02/2015 05:14:

Hi!

Agreed. Personally, I'm a great fan of symettry, so part of me would
prefer to introduce a keyword for when you can't call a method
statically, e.g. "instance function foo() { ... }". But I guess instance
This would be another form of saying "foo() method uses $this", but if
that's true then once it uses $this you'd get the error anyway, so not
much changes from adding "instance".

Well, no, it would mean "you cannot call this method statically, even if
it doesn't use $this", i.e. the same meaning proposed for any method
that omits the keyword "static". By your reasoning, you could say that
adding "static" just means "foo() method doesn't use $this". The point
is that you're making an explicit choice one way or another, and the
version with neither annotation remains ambiguous.

Regards,

Rowan Collins
[IMSoP]