[RFC] Class Friendship

9 years ago by Dustin Wheeler — view source — reply

unread

Hello everyone,

Before Winter holiday, I began a discussion on the mailing list
regarding the topic of class friendship in order to gauge interest
towards authoring and implementing an RFC for the feature. This
discussion happened at:

https://marc.info/?l=php-internals&m=144954699701305&w=2

I have drafted an RFC that describes a proposal to add this feature to
the language. I feel like this draft is at a point where it is
reasonable to invite additional perspective and begin the discussion
process. While the RFC does capture advice I received in the original
discussion, it does not capture as much of the justification /
reasoning behind the feature as I felt that the RFC should present the
facts of the proposal. Because of this, I urge you to read through the
original discussion before reviewing this RFC. It is also included as
the top reference for the RFC. If we feel this information is relevant
to the RFC document itself, I would gladly amend.

https://wiki.php.net/rfc/friend-classes

I will be submitting a pull request against master soon so that
progress and changes to implementation may be tracked for the duration
of RFC discussion. There are a few details I need to iron out in the
implementation that surfaced as part of initial discussions.

This is my first contribution to PHP and it is more ambitious that I
am normally comfortable with when making contributions to open-source
software. That said, I fully recognize and respect the expertise of
developers on this mailing list and welcome scrutiny as far as
technical implementation. I am 100% ready and willing to accept all
feedback to improve implementation where necessary.

Finally, as mentioned in the original discussion:

When I began this work, I found that (for a beginner) the
barrier-to-entry in contributing a new feature to PHP is fairly
substantial. While there were several good references for implementing
a feature such as this, much of it was out-of-date; considering the
changes made internally for PHP 7. I would like to contribute notes I
took while working through this project that begin to detail common
differences in how someone might accomplish a feature like what's
represented by this RFC for PHP 7. This feature was unique in that it
has a fairly simple and light-weight cross-cutting implementation that
seems like a decent example for a newcomer.

Is there a good place for me to send this information for review? Is
another RFC a good medium for review or would a separate post to the
mailing list suffice?

Thanks again for your time and I look forward to upcoming discussions!

-Dustin

--
Dustin Wheeler | Software Developer
NC State University
m: mdwheele@ncsu.edu | w: 5-9786
"If you don't know where you're going, it's easy to iteratively not get there."

9 years ago by francois@php.net — view source — reply

unread

Hi Dustin,

thanks for this nice work !

Here are some comments and thoughts :

There is another inheritance property I didn't include in my initial
article, and I think that should be described :

class Base {
friend BaseFriend;
protected $base_prop;
static protected $base_static;
}

class Derived extends Base {
protected $derived_prop;
static protected $derived_static;
}

class BaseFriend {

public function display()
{
$d = new Derived();

var_dump($d->base_prop);	// OK
var_dump(Derived::$base_static) // OK

var_dump($d->derived_prop);	// Error
var_dump(Derived::$derived_static);	// Error

}}

I try to express it with words : a friend of a base class can access the
properties and methods of this base class through a derived instance.
THe same for static properties and methods through a derived class. I
know it's not very clear but the example should be better. Actually,
that could be a sub-chapter of the 'Friendships are not inherited' property.

After thinking more about it, I now think we should allow some sort of
friendship inheritance but only in one way : while a Friend of Base is
not a friend of Derived, a DerivedOfFriend class would also be a friend
of Base (it inherits the 'friendship' property from its 'Friend' base
class). I think this would be more natural and avoid several
limitations. It also allows to respect the Liskov Substitution
Principle, which is probably a good thing (but I'm not an expert there ;).
In the RFC Impact / To Reflection API chapter, I think there's a
copy/paste error. 'Trait' should probably be replaced with 'Friend'.
While it's fine to explore future scope, detailing it too much brings
the risk that people focus on this part and reject the whole RFC. That's
just an advice because I also preferred exploring every aspects of the
subject, but I learned that RFC readers don't need too many details,
especially regarding future hypothetical developments. Here, you're
proposing friendship between classes. Do you want people to reply that
they don't like your RFC because friendship to a global function is a
terrible thing ? I may be wrong but I think the RFC must keep the reader
focused on the main subject. Your subject is : friend classes and
friendship properties. The rest, like friendship to functions and
methods, does not deserve more than a few lines, just to show that you
thought about it. It will always be time to develop this in a future
follow-up RFC.
The same for 'Combinations of Class, Class Method and Namespace
Friendship'. I think the chapter should be removed, especially because
the syntax you use for namespaces, functions, and methods is still
undefined. So, people may look at your example and say : 'Oh, this
syntax is terribly ambiguous !'. ignoring your comment. You say that
friendship may be extended to namespaces in the future. Syntax remains
to define. Period.
Voting choices : It's generally preferred that RFCs are as opinionated
as possible. So, a single approve/reject vote is preferred. In the RFC,
you explain that you chose that, unlike C++, friends access protected
properties only, not private ones. So, there 's no reason, IMO, to vote
for this. That's your RFC. People read it to get an opinion to
approve/reject, not to make technical choices. The risk when proposing
too many choices is to look hesitating. An RFC must find the right
compromise between humility and self-confidence, but hesitation is not
an option.
About the voter's majority, I see that as a pure addition and there's
no BC break. So, I would say that a 50%+1 majority should be enough. To
be confirmed.

Well, I hope you won't get me wrong, I'm just trying to help and that's
only suggestions. That's your RFC, your work, your decisions. Anyway,
that's a very good work and I hope it will be approved.

Regards

François

9 years ago by Dan Ackroyd — view source — reply

unread

Hi Dustin,

About the voter's majority, I see that as a pure addition and there's no
BC break. So, I would say that a 50%+1 majority should be enough. To be
confirmed.

FYI The voting RFC is reasonably clear on new syntax and the passing
level: https://wiki.php.net/RFC/voting#voting

"For these reasons, a feature affecting the language itself (new
syntax for example) will be considered as 'accepted' if it wins a 2/3
of the votes."

This is definitely new syntax and behaviour.

cheers
Dan

9 years ago by Dustin Wheeler — view source — reply

unread

Dan,

FYI The voting RFC is reasonably clear on new syntax and the passing
level: https://wiki.php.net/RFC/voting#voting

"For these reasons, a feature affecting the language itself (new
syntax for example) will be considered as 'accepted' if it wins a 2/3
of the votes."

Thanks for clearing that up. I'll leave this part of voting choices as-is.

--
Dustin Wheeler | Software Developer
NC State University
m: mdwheele@ncsu.edu | w: 5-9786
"If you don't know where you're going, it's easy to iteratively not get there."

9 years ago by Dustin Wheeler — view source — reply

unread

Hi François,

There is another inheritance property I didn't include in my initial
article, and I think that should be described :

class Base {
friend BaseFriend;
protected $base_prop;
static protected $base_static;
}

class Derived extends Base {
protected $derived_prop;
static protected $derived_static;
}

class BaseFriend {

public function display()
{
$d = new Derived();
    var_dump($d->base_prop);        // OK
    var_dump(Derived::$base_static) // OK

    var_dump($d->derived_prop);     // Error
    var_dump(Derived::$derived_static);     // Error
}}

I try to express it with words : a friend of a base class can access the
properties and methods of this base class through a derived instance. THe
same for static properties and methods through a derived class. I know it's
not very clear but the example should be better. Actually, that could be a
sub-chapter of the 'Friendships are not inherited' property.

I agree. This should be added. Reviewing this now, I also realize that
I've unintentionally worded things to suggest that only protected
properties (not methods) are accessible to friend classes. This is
incorrect. I will be adjusting wording to replace occurrences of
"properties" with "members". I will likely find a way of peppering
that into the examples as well.

What do you think about wording above the examples (which would
include elements quoted above) to the effect of "In addition,
protected members of Base are also accessible through Derived". This
would make this one larger example, with the last code block
demonstrating the rule you've described.

After thinking more about it, I now think we should allow some sort of
friendship inheritance but only in one way : while a Friend of Base is not a
friend of Derived, a DerivedOfFriend class would also be a friend of Base
(it inherits the 'friendship' property from its 'Friend' base class). I
think this would be more natural and avoid several limitations. It also
allows to respect the Liskov Substitution Principle, which is probably a
good thing (but I'm not an expert there ;).

This is interesting. I'm going to need to think on this a bit more but
I understand what you're saying. If included, I want to have a good
example of when I would grab for this. Additionally, we can always
leave ourselves open to this in the future as well. I'm neither for or
against this right now. I'm going to reserve further comment here for
now. If you have an application of this property in-mind, I am
interested.

In the RFC Impact / To Reflection API chapter, I think there's a
copy/paste error. 'Trait' should probably be replaced with 'Friend'.

I've amended this in v0.2.1. Indeed, it is a copy/paste error from the
documentation regarding how ReflectionClass supports query on attached
Traits.

While it's fine to explore future scope, detailing it too much brings the
risk that people focus on this part and reject the whole RFC. That's just an
advice because I also preferred exploring every aspects of the subject,

I agree. I left it in for discussion to see if I got exactly that
feedback, haha. There are a lot of details, already, in explaining the
feature and its properties. Near the tail-end of the document, the
reader probably is at "information overload".

The chapter on combining friendship types is easily removed as it
really doesn't add anything non-obvious. I think it would be
reasonable to remove examples from other future work and pare down
from there. I wonder if it is valuable (or not) to justify the future
scope rather than simply list future work that could be done. In fact,
this chapter started out as a simple bullet-ed list.

Voting choices : It's generally preferred that RFCs are as opinionated as
possible. So, a single approve/reject vote is preferred. In the RFC, you
explain that you chose that, unlike C++, friends access protected properties
only, not private ones. So, there 's no reason, IMO, to vote for this.
That's your RFC. People read it to get an opinion to approve/reject, not
to make technical choices. The risk when proposing too many choices is to
look hesitating. An RFC must find the right compromise between humility and
self-confidence, but hesitation is not an option.

You're the third person to make a similar comment regarding striking
that balance. I'm glad to get the feedback as this is definitely new
waters for me. I am confident in the decision to disallow friend
access to private members and believe that is the best decision for
this RFC. I'll remove the additional voting choice.

Well, I hope you won't get me wrong, I'm just trying to help and that's only
suggestions. That's your RFC, your work, your decisions. Anyway, that's a
very good work and I hope it will be approved.

François, I very much appreciate your suggestions and look forward to
discussing this more.

Thanks,

-Dustin

--
Dustin Wheeler | Software Developer
NC State University
m: mdwheele@ncsu.edu | w: 5-9786
"If you don't know where you're going, it's easy to iteratively not get there."

9 years ago by Dan Ackroyd — view source — reply

unread

Hello everyone,

I have drafted an RFC that describes a proposal to add this feature to
the language.

https://wiki.php.net/rfc/friend-classes

Hi Dustin,

I try to avoid sounding too negative when people come up with ideas
that I might not agree with. However....I feel obliged to give honest
feedback earlier rather than later; I can't see any circumstances
under which I'd vote for this RFC.

In my dim and distant youth I was C++ programmer.....and friends
relationship between classes are not something I look back on fondly.
In fact, they were a source of much sorrow.

Although they solve a particular problem, they do so in a way that has
too many downsides. From the examples:

class Fibonacci {
friend FibonacciTest;
....
}

class Person {
friend HumanResourceReport;
...
}

The classes that are exposing their protected properties have to know
about what classes are going to be using them. It means if I want to
add a new test similar to the FibonacciTest, I also have to edit the
Fibonacci class itself.....this is pretty terrible.

That does not scale well - either in just the amount of code typed, or
in how difficult it is to reason about the code. What ends up
happening is that if a property needs to be exposed to friends once,
it ends up being exposed to lots of friends.

From the other example:

class HumanResourceReport
{
public function getFullName() {
// HumanResourceReport would not have access to protected
// members of Person if not explicitly listed as a friend.
return $this->person->firstName . ' ' . $this->person->lastName;
}
}

Again, this has a massive downside. The 'domain logic' of how to
construct a full name from a first name and last name belongs in the
person class. By allowing the individual elements to be accessed, the
code is violating the 'separation of concerns'. When middle names are
added to the Person object, the HumanResourcesReport will continue to
generate full names as it has always done....until someone notices a
files a bug report.

I agree that we shouldn't put RFCs in a head-to-head fight. But to me
the package level protection of properties/methods sounds like a much
bette potential solution to the problem that currently visibility is a
binary option. But even if that idea wasn't floating around, I would
not like to see friendship based visibility in PHP, as the solution it
provides is worse than the problem it fixes, while at the same time
adding more complexity to the engine. Apologies again for being so
negative.

cheers
Dan

p.s. For the FibonacciTest example, if it was required to write a test
like that, below is how I would do it in PHP currently. It achieves
the aim of testing the class, without having to make the Fibonacci be
aware of the classes that are going to be testing it, or having to add
any new features to PHP.

class Fibonacci
{
protected $previous;
protected $current;

public function __construct()
{
    $this->previous = 0;
    $this->current = 0;
}

public function `next()`
{
    $current = $this->current;
    $next = $this->previous + $this->current;

    if ($next == 0) {
        $next = 1;
    }

    $this->previous = $this->current;
    $this->current = $next;

    return $current;
}

}

class IntrospectableFibonacci extends Fibonacci
{
function getPrevious()
{
return $this->previous;
}

function getCurrent()
{
    return $this->current;
}

}

class FibonacciTest extends PHPUnit_Framework_TestCase
{
public function testAssignmentAlgoForStateIsCorrect()
{
$fibo = new IntrospectableFibonacci();

    $this->assertEquals(0, $fibo->getPrevious());
    $this->assertEquals(0, $fibo->getCurrent());

    $n0 = $fibo->next();

    $this->assertEquals(0, $n0);
    $this->assertEquals(0, $fibo->getPrevious());
    $this->assertEquals(1, $fibo->getCurrent());
}

}

9 years ago by Dustin Wheeler — view source — reply

unread

Hi Dan,

Hi Dustin,

I try to avoid sounding too negative when people come up with ideas
that I might not agree with. However....I feel obliged to give honest
feedback earlier rather than later; I can't see any circumstances
under which I'd vote for this RFC.

No need to hesitate with honest feedback. I understand that to be part
of the RFC process. This RFC is already close to "paying for itself"
from my perspective, in that it collects critiques and feedback to be
attached to the document so that in the event this is declined there
is "official" documentation of why. Currently, there are a handful of
list archives, chat logs, etc. scattered across the web. This process
bakes a more official opinion on the feature.

In addition, as part of doing this I learned a ton that I can use to
make further contributions myself and to help others get started.
Plus, I have an opportunity to go through the process, which is
another learning experience! :)

Although they solve a particular problem, they do so in a way that has
too many downsides. From the examples:

class Fibonacci {
friend FibonacciTest;
....
}

[snip]

The classes that are exposing their protected properties have to know
about what classes are going to be using them. It means if I want to
add a new test similar to the FibonacciTest, I also have to edit the
Fibonacci class itself.....this is pretty terrible.

I agree that by exposing protected properties to friended classes,
you're tightly-coupling an object to another. In the case of white-box
testing, friendship is a purposefully introduced smell, in my opinion.
Production code marked as coupled to test-cases is 100% a smell, but I
see this specific usage as a tactical strategy that is part of a
longer-running refactor. In-progress, I might leave these couplings in
place, but I would have sniffers that signaled these areas of the
project were targets for continued improvement.

Your testing example (''IntrospectableFibonacci'') is a great example
of alternative solution. I can't argue that friendship offers much
over this since we have deviated from the C++ implementation whereby
friends have access to ''private'' members. The only benefit I see
(currently and for this specific example, which is intentionally
trivial) is the class being statically marked for purposes of sniffing
/ analysis.

That does not scale well - either in just the amount of code typed, or
in how difficult it is to reason about the code. What ends up
happening is that if a property needs to be exposed to friends once,
it ends up being exposed to lots of friends.

If a class ends up exposed to many friends without real reason
semantic to a domain model, my opinion would be that it is a
misapplication of the feature. You'd be effectively shooting yourself
in the foot with the "tightly-coupled gun". I can make a similar
argument against a feature like Traits in that if I over-apply or
misapply traits, I am shooting myself in the foot with the "copy-paste
and duplication of code gun". I do agree with you that this feature
does expose an opportunity for abuse and headache, but I think that
can be said about many features in the language.

It's hard for me to dictate best practice versus simply the mechanics
of the feature. Do you think this type of information is worth baking
into the RFC to clarify intended usage of the feature?

From the other example:

class HumanResourceReport
{
public function getFullName() {
// HumanResourceReport would not have access to protected
// members of Person if not explicitly listed as a friend.
return $this->person->firstName . ' ' . $this->person->lastName;
}
}

Again, this has a massive downside. The 'domain logic' of how to
construct a full name from a first name and last name belongs in the
person class. By allowing the individual elements to be accessed, the
code is violating the 'separation of concerns'.

This is really an overly simplified example intended to express the
mechanics of the feature and not-necessarily "best practice". To a
degree, I agree with you that this specific type of presentational
logic might belong as part of a single object's API. Over time, as
that object grows, I see friendship as an expression of 'separation of
concerns', not a violation. Consider CQS (command-query separation),
whereby an object's API is constructed by either methods that return
observable state free of side-effect or methods that mutate state and
do not return anything. As that API grows, it may be appropriate to
refactor that model to separate concerns. In many cases, deepened
discovery in a domain might grant clarity that you've actually modeled
a conflation of ideas. In other cases, you might just have a case
where the abstraction is correct, but the API serves two concerns
(read and write, query and command). In these cases, you grab for
friendship as a means of expressing physical separation of concerns of
the command and query portions of a given object. Taken further, CQRS
(command-query responsibility segregation) expresses totally
segregated models for reads and writes across a system.

When middle names are added to the Person object,
the HumanResourcesReport will continue to generate full names as it
has always done....until someone notices a files a bug report.

This would be a misstep modeling and failure in testing, in my
opinion. Granted, again, that this is an intentionally trivial example
so the argument will by extension seem trivial.

Keep in mind, that in this example Person might represent solely the
business rules and invariants of what it means to be a Person.
HumanResourcesReport is a separation of concerns describing what a
Person looks like in the domain of human resources. They are truly
separate concerns. In this way, I would argue that simply the addition
of middle name as an invariant of Person does not imply that it would
be incorrect (or a "bug") for the report to continue generating full
name as it always had. This is an advantage of friendship, not a
failing! Can it be abused? Of course!

The tests for HumanResourcesReport should dictate when it's time to
change behaviour of that query. Imagine if, instead, Human Resources
says, "We like what we see in the report, but we're now required by
law to include middle names as part of the full printed name.". We'd
write a test for the friended class to add this behaviour and that
would influence changes that might (or might not) need to be made on
Person.

Apologies again for being so negative.

No need to apologize! I appreciate the feedback and look forward to
continued discussion. Good stuff!

Thanks

--
Dustin Wheeler | Software Developer
NC State University
m: mdwheele@ncsu.edu | w: 5-9786
"If you don't know where you're going, it's easy to iteratively not get there."