[RFC][VOTE] Allow abstract function override

7 years ago by netmo.php@gmail.com — view source

unread

Hello PHPeeps,

There hasn't been much discussion around the proposed feature, and I've
interpreted it as a good sign :P It is not a super important change but it
has some advantages, it's consistent with the recent improvements to type
variance and also with future ones. Also, it's hopefully not too hard to
implement.

https://wiki.php.net/rfc/allow-abstract-function-override

I've decided to start the vote. It will end two weeks from this message, on
10th April 2017.

Thanks in advance for participating.

Wes

7 years ago by Fleshgrinder — view source

unread

Hello PHPeeps,

There hasn't been much discussion around the proposed feature, and I've
interpreted it as a good sign :P It is not a super important change but it
has some advantages, it's consistent with the recent improvements to type
variance and also with future ones. Also, it's hopefully not too hard to
implement.

https://wiki.php.net/rfc/allow-abstract-function-override

I've decided to start the vote. It will end two weeks from this message, on
10th April 2017.

Thanks in advance for participating.

Wes

There is absolutely no BC break at all in this RFC. The current status
of things makes no sense because it is inconsistent. Heck, even the fact
that it is inconsistent shows that there are different rules for the
same things. Semantically there is no difference between a class, an
abstract class, or an interface. They are all classes, some simply omit
implementation details and leave it to their subclasses.

I hope this gets a yes, regardless of possible use cases, this is simply
applying proper variant rules everywhere.

--
Richard "Fleshgrinder" Fussenegger

7 years ago by Marco Pivetta — view source

unread

Hey Folks,

Sorry, my mistake on detecting the BC break. I had a misconception about
how ReflectionMethod#getDeclaringClass() works. I thought that the
declaring class was the declaring abstraction, but it's actually always
pointing to the most concrete implementation in the inheritance.

See https://3v4l.org/t7PiL

Code, for reference:

<?php

class Thing {}

interface Foo {
public function bar($baz) : Thing;
}

interface Bar extends Foo {
public function bar($baz) : Thing;
}

final class Baz implements Bar
{
public function bar($baz) : Thing
{
return new Thing;
}
}

var_dump((new
ReflectionClass(Foo::class))->getMethod('bar')->getDeclaringClass()->getName());
// expected Foo, it actually is Foo
var_dump((new
ReflectionClass(Bar::class))->getMethod('bar')->getDeclaringClass()->getName());
// expected Foo, got Bar (my assumption was wrong)
var_dump((new
ReflectionClass(Baz::class))->getMethod('bar')->getDeclaringClass()->getName());
// expected Foo, got Baz (my assumption was wrong)

This is my mistake, as I didn't verify my assumption on code before
reporting the BC break. Sorry again - changing my vote to yes, as the
discussed covariance/contravariance changes make sense for interfaces as
well.

Greets,

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

7 years ago by netmo.php@gmail.com — view source

unread

No problem. I instead thank you for checking this, because I forgot to
check it myself (sorry).

7 years ago by Patrick ALLAERT — view source

unread

Le mar. 28 mars 2017 à 06:51, Wes netmo.php@gmail.com a écrit :

Hello PHPeeps,

There hasn't been much discussion around the proposed feature, and I've
interpreted it as a good sign :P It is not a super important change but it
has some advantages, it's consistent with the recent improvements to type
variance and also with future ones. Also, it's hopefully not too hard to
implement.

https://wiki.php.net/rfc/allow-abstract-function-override

I've decided to start the vote. It will end two weeks from this message, on
10th April 2017.

Thanks in advance for participating.

Wes

Hello,

Voted "no" because the RFC starts with:

"
PHP 7 introduced type declarations for return and improved ones for
parameters. They currently support a very limited kind of variance (only to
no-type):

<?php
class A { function bar(stdClass $x){} }
class B extends A { function bar($x): stdClass{} }
"

Which looks false in both PHP 7.0 and PHP 7.1.

Sorry if I missed something in a past discussions, but it looks wrong to me
to implement anything based on a false assertion.

I would be happy to change my vote if you can fix the RFC and/or enlight me.

Cheers,
Patrick

7 years ago by Niklas Keller — view source

unread

2017-04-10 11:34 GMT+02:00 Patrick ALLAERT patrickallaert@php.net:

Le mar. 28 mars 2017 à 06:51, Wes netmo.php@gmail.com a écrit :

Hello PHPeeps,

There hasn't been much discussion around the proposed feature, and I've
interpreted it as a good sign :P It is not a super important change but
it
has some advantages, it's consistent with the recent improvements to type
variance and also with future ones. Also, it's hopefully not too hard to
implement.

https://wiki.php.net/rfc/allow-abstract-function-override

I've decided to start the vote. It will end two weeks from this message,
on
10th April 2017.

Thanks in advance for participating.

Wes

Hello,

Voted "no" because the RFC starts with:

"
PHP 7 introduced type declarations for return and improved ones for
parameters. They currently support a very limited kind of variance (only to
no-type):

<?php
class A { function bar(stdClass $x){} }
class B extends A { function bar($x): stdClass{} }
"

Which looks false in both PHP 7.0 and PHP 7.1.

Sorry if I missed something in a past discussions, but it looks wrong to me
to implement anything based on a false assertion.

I would be happy to change my vote if you can fix the RFC and/or enlight
me.

Cheers,
Patrick

It's implemented in PHP 7.2 (master).

Regards, Niklas

7 years ago by netmo.php@gmail.com — view source

unread

Hi Patrick, this is basically the completion of what Niklas did in his RFC
and that PHP 7.0 started. I could've being more clear :) PHP 7 introduced
variance (variance from mixed) for returns, then Niklas added the same
thing for parameters (variance to mixed) and this extends the support to
abstract declarations as well.

I'm closing the vote as 2 weeks have passed. Thanks to all the voters!

7 years ago by Patrick ALLAERT — view source

unread

Le mar. 11 avr. 2017 à 06:32, Wes netmo.php@gmail.com a écrit :

Hi Patrick, this is basically the completion of what Niklas did in his RFC
and that PHP 7.0 started. I could've being more clear :) PHP 7 introduced
variance (variance from mixed) for returns, then Niklas added the same
thing for parameters (variance to mixed) and this extends the support to
abstract declarations as well.

I'm closing the vote as 2 weeks have passed. Thanks to all the voters!

Yes, I got the intention after digging more. I would also have changed my
vote to 'Yes', but it passes, so it's all good!

Thanks for the clarification!
Patrick

7 years ago by Marco Pivetta — view source

unread

Voted "no" because of this paragraph:

This RFC proposes to allow this, even if it has very few uses, and
because there is no reason to disallow a compatible redefinition.

I couldn't see a use-case.

In addition to that, this breaks the assumption that the topmost abstract
signature is the authoritative one. That is a subtle BC break, but still a
BC break. Yes, I know I'm annoying, sorry ?

Hello PHPeeps,

https://wiki.php.net/rfc/allow-abstract-function-override

I've decided to start the vote. It will end two weeks from this message, on
10th April 2017.

Thanks in advance for participating.

Wes

7 years ago by netmo.php@gmail.com — view source

unread

Yes, yes you are :D Quick reminder:

interface iA                 { function bar(stdClass $x); }
interface iB extends iA      { function bar($x): stdClass; } // OK

class cA                     { function bar(stdClass $x) {} }
class cB extends cA          { function bar($x): stdClass {} } // OK in 7.2

abstract class cC            { abstract function bar(stdClass $x); }
abstract class cD extends cC { abstract function bar($x): stdClass; } //
Fatal error: why?
         class cE extends cD {          function bar($x): stdClass{} }

7 years ago by Marco Pivetta — view source

unread

Hey Wes,

Yes, yes you are :D Quick reminder:

interface iA                 { function bar(stdClass $x); }
interface iB extends iA      { function bar($x): stdClass; } // OK

class cA                     { function bar(stdClass $x) {} }
class cB extends cA          { function bar($x): stdClass {} } // OK in 7.2

abstract class cC            { abstract function bar(stdClass $x); }
abstract class cD extends cC { abstract function bar($x): stdClass; } //
Fatal error: why?
         class cE extends cD {          function bar($x): stdClass{} }

What is this useful for, precisely? Also, I really never saw the need for
re-defining the signature in multiple abstract inheritance levels (a single
abstract class already smells in most scenarios).

7 years ago by netmo.php@gmail.com — view source

unread

Hi again. There is no BC break, at all.

7 years ago by Derick Rethans — view source

unread

Voted "no" because of this paragraph:

This RFC proposes to allow this, even if it has very few uses, and
because there is no reason to disallow a compatible redefinition.

I couldn't see a use-case.

In addition to that, this breaks the assumption that the topmost abstract
signature is the authoritative one. That is a subtle BC break, but still a
BC break. Yes, I know I'm annoying, sorry ?

As there is a subtle BC break, then that needs to be mentioned in the
RFC under "Backward Incompatible Changes".

cheers,
Derick

7 years ago by netmo.php@gmail.com — view source

unread

Hi Derick, sorry, previous message was meant to reply to you. There is no
BC break, not even subtle.

Ocramius' assumption is wrong. The most authoritative one is the innermost
and closest ancestor definition. It is like that for classes, it is like
that for interfaces, should be like that for abstract methods as well. And
it's just an assumption anyway.

Substitutability rules define this and it is very straightforward to me:
are the overridden signatures type-compatible with the super definition?
Yes. Then it should be allowed; we should not care if it's redundant, we
should not care if it has few uses, just keep it consistent, IMO.

Use cases are the same of a type change in a sub-interface, which, again,
PHP does allow, even if to/from no-type only for now.

Regards.

7 years ago by Bob Weinand — view source

unread

Am 28.03.2017 um 06:58 schrieb Marco Pivetta ocramius@gmail.com:

Voted "no" because of this paragraph:

This RFC proposes to allow this, even if it has very few uses, and
because there is no reason to disallow a compatible redefinition.

I couldn't see a use-case.

In addition to that, this breaks the assumption that the topmost abstract
signature is the authoritative one. That is a subtle BC break, but still a
BC break. Yes, I know I'm annoying, sorry ?

Hey,
May you please explain where that's a BC break?
If there was a BC break, the BC break was in PHP 7, where that guarantee was broken, as the RFC states in its introduction.
Now the only guarantee is that the all abstract function signatures are the same ones. Which is not an useful guarantee by any means.

The only thing this RFC changes is making abstract and normal classes consistent, which is a good thing. Less special cases.

Bob