[VOTE] Remove inappropriate inheritance signature checks on private methods

  110540
June 15, 2020 20:42 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
Hi internals,

I have opened the vote on the "Remove inappropriate inheritance signature
checks on private methods" RFC. Which can be found here:

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

The voting period will end on 2020-06-29 22:00 UTC.

Thanks to those who participated in the discussion and provided feedback.

Regards,
Pedro
  110569
June 16, 2020 09:05 nikita.ppv@gmail.com (Nikita Popov)
On Mon, Jun 15, 2020 at 10:43 PM Pedro Magalhães <mail@pmmaga.net> wrote:

> Hi internals, > > I have opened the vote on the "Remove inappropriate inheritance signature > checks on private methods" RFC. Which can be found here: > > https://wiki.php.net/rfc/inheritance_private_methods > > The voting period will end on 2020-06-29 22:00 UTC. > > Thanks to those who participated in the discussion and provided feedback. >
Voting no on this one, for the reason I previously mentioned on the PR: While the abstract and static parts of the RFC make sense, the final part does not (to me). Changing the behavior of private final just removes existing functionality, without any clear benefit that I can see. What is the problem that change tries to solve? The original RFC could at least make a consistency argument, but the final form, which converts an existing special case into an even more convoluted special case, can not. Why is __construct() exempted, but __clone() for example isn't, even though similar considerations apply to it? Regards, Nikita
  110572
June 16, 2020 09:19 ocramius@gmail.com (Marco Pivetta)
Hey Nikita,

On Tue, Jun 16, 2020 at 11:05 AM Nikita Popov ppv@gmail.com> wrote:

> On Mon, Jun 15, 2020 at 10:43 PM Pedro Magalhães <mail@pmmaga.net> wrote: > > > Hi internals, > > > > I have opened the vote on the "Remove inappropriate inheritance signature > > checks on private methods" RFC. Which can be found here: > > > > https://wiki.php.net/rfc/inheritance_private_methods > > > > The voting period will end on 2020-06-29 22:00 UTC. > > > > Thanks to those who participated in the discussion and provided feedback. > > > > Voting no on this one, for the reason I previously mentioned on the PR: > While the abstract and static parts of the RFC make sense, the final part > does not (to me). Changing the behavior of private final just removes > existing functionality, without any clear benefit that I can see. What is > the problem that change tries to solve?
Possibly missing in the RFC, but a parent class declaring a new `private` symbol shouldn't affect `private` API in child classes that may be pre-existing: helps backwards compatibility.
> The original RFC could at least > make a consistency argument, but the final form, which converts an existing > special case into an even more convoluted special case, can not. Why is > __construct() exempted, but __clone() for example isn't, even though > similar considerations apply to it? >
While `private final function __construct()` prevents child classes from making the constructor open, or accessing it (important for abstract types), `protected final function __clone()` achieves the same without having to rely on the `private` visibility modifier (https://3v4l.org/psR5F, for example). It is true that cloning is then possible from within a subtype, which is indeed a bit of a problem: https://3v4l.org/qupHR Maybe the magic methods would indeed all need to respect `final`, and we haven't considered all scenarios? These considerations were part of the previous discussion on the RFC - maybe should be added to its notes? Greets, Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/
  110576
June 16, 2020 10:20 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
On Tue, Jun 16, 2020 at 10:19 AM Marco Pivetta <ocramius@gmail.com> wrote:

> On Tue, Jun 16, 2020 at 11:05 AM Nikita Popov ppv@gmail.com> > wrote: > >> The original RFC could at least >> make a consistency argument, but the final form, which converts an >> existing >> special case into an even more convoluted special case, can not. Why is >> __construct() exempted, but __clone() for example isn't, even though >> similar considerations apply to it? >> > > While `private final function __construct()` prevents child classes from > making the constructor open, or accessing it (important for abstract > types), `protected final function __clone()` achieves the same without > having to rely on the `private` visibility modifier ( > https://3v4l.org/psR5F, for example). >
In my opinion, the example given by Marco on the discussion thread about sealed types is a good argument on why __construct should keep the behavior (https://externals.io/message/110251#110255). You would need to write a lot of convoluted code to achieve the same behavior as you currently can. It is true that cloning is then possible from within a subtype, which is
> indeed a bit of a problem: https://3v4l.org/qupHR >
I don't find that problematic. If ultimately your goal is to disallow cloning, doing it by visibility constraints is cheaper and covers 99% of the cases but it's still possible to enforce it even for the child class by throwing: https://3v4l.org/U1LMQ Regards, Pedro
  110680
June 19, 2020 09:17 nikita.ppv@gmail.com (Nikita Popov)
On Tue, Jun 16, 2020 at 11:19 AM Marco Pivetta <ocramius@gmail.com> wrote:

> Hey Nikita, > > On Tue, Jun 16, 2020 at 11:05 AM Nikita Popov ppv@gmail.com> > wrote: > >> On Mon, Jun 15, 2020 at 10:43 PM Pedro Magalhães <mail@pmmaga.net> wrote: >> >> > Hi internals, >> > >> > I have opened the vote on the "Remove inappropriate inheritance >> signature >> > checks on private methods" RFC. Which can be found here: >> > >> > https://wiki.php.net/rfc/inheritance_private_methods >> > >> > The voting period will end on 2020-06-29 22:00 UTC. >> > >> > Thanks to those who participated in the discussion and provided >> feedback. >> > >> >> Voting no on this one, for the reason I previously mentioned on the PR: >> While the abstract and static parts of the RFC make sense, the final part >> does not (to me). Changing the behavior of private final just removes >> existing functionality, without any clear benefit that I can see. What is >> the problem that change tries to solve? > > > Possibly missing in the RFC, but a parent class declaring a new `private` > symbol shouldn't affect `private` API in child classes that may be > pre-existing: helps backwards compatibility. >
Right, and I totally agree with that part insofar as it concerns static/abstract handling. However, "private final" in particular is not an accidental addition in the base class, it's an explicit design decision to forbid certain behaviors in child classes. The original RFC could at least
> >> make a consistency argument, but the final form, which converts an >> existing >> special case into an even more convoluted special case, can not. Why is >> __construct() exempted, but __clone() for example isn't, even though >> similar considerations apply to it? >> > > While `private final function __construct()` prevents child classes from > making the constructor open, or accessing it (important for abstract > types), `protected final function __clone()` achieves the same without > having to rely on the `private` visibility modifier ( > https://3v4l.org/psR5F, for example). > > It is true that cloning is then possible from within a subtype, which is > indeed a bit of a problem: https://3v4l.org/qupHR > > Maybe the magic methods would indeed all need to respect `final`, and we > haven't considered all scenarios? >
Or ... we could just not change this part of the behavior at all :) If we acknowledge that this is definitely useful for constructors and possibly useful for cloning, then maybe it is useful for other things as well? I still haven't heard a reason *why* we would want to do this change (not the general change, but specifically the "private final" one). This stuff really isn't super important to me, and I'd be happy to sacrifice this functionality in the name of the greater good, but I honestly don't understand what that greater good is in this case. It seems like a strict loss in functionality. Regards, Nikita
  110682
June 19, 2020 11:30 drealecs@gmail.com (=?UTF-8?Q?Alexandru_P=C4=83tr=C4=83nescu?=)
On Fri, Jun 19, 2020 at 12:17 PM Nikita Popov ppv@gmail.com> wrote:
> > On Tue, Jun 16, 2020 at 11:19 AM Marco Pivetta <ocramius@gmail.com> wrote: > > > Hey Nikita, > > > > On Tue, Jun 16, 2020 at 11:05 AM Nikita Popov ppv@gmail.com> > > wrote: > > > >> On Mon, Jun 15, 2020 at 10:43 PM Pedro Magalhães <mail@pmmaga.net> wrote: > >> > >> > Hi internals, > >> > > >> > I have opened the vote on the "Remove inappropriate inheritance > >> signature > >> > checks on private methods" RFC. Which can be found here: > >> > > >> > https://wiki.php.net/rfc/inheritance_private_methods > >> > > >> > The voting period will end on 2020-06-29 22:00 UTC. > >> > > >> > Thanks to those who participated in the discussion and provided > >> feedback. > >> > > >> > >> Voting no on this one, for the reason I previously mentioned on the PR: > >> While the abstract and static parts of the RFC make sense, the final part > >> does not (to me). Changing the behavior of private final just removes > >> existing functionality, without any clear benefit that I can see. What is > >> the problem that change tries to solve? > > > > > > Possibly missing in the RFC, but a parent class declaring a new `private` > > symbol shouldn't affect `private` API in child classes that may be > > pre-existing: helps backwards compatibility. > > > > Right, and I totally agree with that part insofar as it concerns > static/abstract handling. However, "private final" in particular is not an > accidental addition in the base class, it's an explicit design decision to > forbid certain behaviors in child classes. > > The original RFC could at least > > > >> make a consistency argument, but the final form, which converts an > >> existing > >> special case into an even more convoluted special case, can not. Why is > >> __construct() exempted, but __clone() for example isn't, even though > >> similar considerations apply to it? > >> > > > > While `private final function __construct()` prevents child classes from > > making the constructor open, or accessing it (important for abstract > > types), `protected final function __clone()` achieves the same without > > having to rely on the `private` visibility modifier ( > > https://3v4l.org/psR5F, for example). > > > > It is true that cloning is then possible from within a subtype, which is > > indeed a bit of a problem: https://3v4l.org/qupHR > > > > Maybe the magic methods would indeed all need to respect `final`, and we > > haven't considered all scenarios? > > > > Or ... we could just not change this part of the behavior at all :) If we > acknowledge that this is definitely useful for constructors and possibly > useful for cloning, then maybe it is useful for other things as well? I > still haven't heard a reason *why* we would want to do this change (not the > general change, but specifically the "private final" one). > > This stuff really isn't super important to me, and I'd be happy to > sacrifice this functionality in the name of the greater good, but I > honestly don't understand what that greater good is in this case. It seems > like a strict loss in functionality. >
From how I understood it, right now "private final" cannot possibly have a real world use-case for other cases except for __construct and possibly for other fixed named magic methods like __clone. Someone can use it in a parent class and a child class developed by someone would have a restriction on what private method it can use to do his own private stuff. In reality, that's easily circumvented by choosing a different private name but this RFC will allow the user to use his prefered name for the private method. That's mainly why it's useful from my point of view. Regards, Alex
  110726
June 25, 2020 12:54 nikita.ppv@gmail.com (Nikita Popov)
On Fri, Jun 19, 2020 at 1:30 PM Alexandru Pătrănescu <drealecs@gmail.com>
wrote:

> On Fri, Jun 19, 2020 at 12:17 PM Nikita Popov ppv@gmail.com> > wrote: > > > > On Tue, Jun 16, 2020 at 11:19 AM Marco Pivetta <ocramius@gmail.com> > wrote: > > > > > Hey Nikita, > > > > > > On Tue, Jun 16, 2020 at 11:05 AM Nikita Popov ppv@gmail.com> > > > wrote: > > > > > >> On Mon, Jun 15, 2020 at 10:43 PM Pedro Magalhães <mail@pmmaga.net> > wrote: > > >> > > >> > Hi internals, > > >> > > > >> > I have opened the vote on the "Remove inappropriate inheritance > > >> signature > > >> > checks on private methods" RFC. Which can be found here: > > >> > > > >> > https://wiki.php.net/rfc/inheritance_private_methods > > >> > > > >> > The voting period will end on 2020-06-29 22:00 UTC. > > >> > > > >> > Thanks to those who participated in the discussion and provided > > >> feedback. > > >> > > > >> > > >> Voting no on this one, for the reason I previously mentioned on the > PR: > > >> While the abstract and static parts of the RFC make sense, the final > part > > >> does not (to me). Changing the behavior of private final just removes > > >> existing functionality, without any clear benefit that I can see. > What is > > >> the problem that change tries to solve? > > > > > > > > > Possibly missing in the RFC, but a parent class declaring a new > `private` > > > symbol shouldn't affect `private` API in child classes that may be > > > pre-existing: helps backwards compatibility. > > > > > > > Right, and I totally agree with that part insofar as it concerns > > static/abstract handling. However, "private final" in particular is not > an > > accidental addition in the base class, it's an explicit design decision > to > > forbid certain behaviors in child classes. > > > > The original RFC could at least > > > > > >> make a consistency argument, but the final form, which converts an > > >> existing > > >> special case into an even more convoluted special case, can not. Why > is > > >> __construct() exempted, but __clone() for example isn't, even though > > >> similar considerations apply to it? > > >> > > > > > > While `private final function __construct()` prevents child classes > from > > > making the constructor open, or accessing it (important for abstract > > > types), `protected final function __clone()` achieves the same without > > > having to rely on the `private` visibility modifier ( > > > https://3v4l.org/psR5F, for example). > > > > > > It is true that cloning is then possible from within a subtype, which > is > > > indeed a bit of a problem: https://3v4l.org/qupHR > > > > > > Maybe the magic methods would indeed all need to respect `final`, and > we > > > haven't considered all scenarios? > > > > > > > Or ... we could just not change this part of the behavior at all :) If we > > acknowledge that this is definitely useful for constructors and possibly > > useful for cloning, then maybe it is useful for other things as well? I > > still haven't heard a reason *why* we would want to do this change (not > the > > general change, but specifically the "private final" one). > > > > This stuff really isn't super important to me, and I'd be happy to > > sacrifice this functionality in the name of the greater good, but I > > honestly don't understand what that greater good is in this case. It > seems > > like a strict loss in functionality. > > > > From how I understood it, right now "private final" cannot possibly > have a real world use-case for other cases except for __construct and > possibly for other fixed named magic methods like __clone. > > Someone can use it in a parent class and a child class developed by > someone would have a restriction on what private method it can use to > do his own private stuff. > In reality, that's easily circumvented by choosing a different private > name but this RFC will allow the user to use his prefered name for the > private method. That's mainly why it's useful from my point of view. >
I agree that the "private final" concept is only useful for cases where the engine assigns special meaning to the method name, and can see the rationale for not allowing it on normal (non-magic) methods. I've dropped my "no" vote on the RFC. I don't like the specific decision on this particular aspect, but not enough to block the rest. Regards, Nikita
  110683
June 19, 2020 12:59 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
On Fri, Jun 19, 2020 at 10:17 AM Nikita Popov ppv@gmail.com> wrote:

> On Tue, Jun 16, 2020 at 11:19 AM Marco Pivetta <ocramius@gmail.com> wrote: > >> Maybe the magic methods would indeed all need to respect `final`, and we >> haven't considered all scenarios? >> > > Or ... we could just not change this part of the behavior at all :) If we > acknowledge that this is definitely useful for constructors and possibly > useful for cloning, then maybe it is useful for other things as well? I > still haven't heard a reason *why* we would want to do this change (not the > general change, but specifically the "private final" one). > > This stuff really isn't super important to me, and I'd be happy to > sacrifice this functionality in the name of the greater good, but I > honestly don't understand what that greater good is in this case. It seems > like a strict loss in functionality. > > Regards, > Nikita >
Hi Nikita, The motivations for changing the behavior of final private are the following: - Theoretical: Even though private members are technically inherited by child classes, they are not overridable. You can already have public methods in a child class with the same name as a private parent, but you are not overriding it (if you call that method in the parent, you will use the private method declared there. If you call it in the child, you will use the public method declared there - https://3v4l.org/TImO8). You can have different visibility, different signatures (thanks to a bugfix that was actually my first contribution to PHP :) ) and the methods are completely unrelated except that they share the name. Sharing the name shouldn't be a reason to enforce any inheritance rules. - Implementation: My preferred way to implement this would have been to skip the call to `do_inheritance_check_on_method_ex` on private methods completely, as that would make the intention very clear. The only reason why I couldn't is the check for abstract private methods coming from traits. And currently, the ZEND_ACC_CHANGED flag is also added there, but I think that could be easily avoided. - Other languages: - Java: https://www.ideone.com/P71k8J - final has no effect when added to a private method ( https://www.javaworld.com/article/2077399/private-and-final.html) - C#: https://www.ideone.com/i1OtFe - In C#, for a method to be overridable it has to be marked as virtual. In this snippet, we are not marking it as virtual (hence it acts as final) and nothing prevents us from declaring it in the child class. Regards, Pedro
  110790
June 29, 2020 23:18 mail@pmmaga.net (=?UTF-8?Q?Pedro_Magalh=C3=A3es?=)
Hi internals,

I'm glad to inform you that the RFC has been accepted with 24 votes for and
11 votes against.

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

I'll add some notes to UPGRADING on the PR and hope to get a new review of
the implementation before merging it.

Thank you again to all who contributed to the discussion and to all who
voted.

Regards,
Pedro

>