silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 822 forks source link

RFC-8996 Clarify the scope of public APIs #8996

Closed robbieaverill closed 4 years ago

robbieaverill commented 5 years ago

Affected Version

All

Description

Lately there have been some discussions in pull requests around whether we prefer private or protected methods (when not public).

Key questions:

Acceptance criteria:

cc @silverstripe/core-team @silverstripe/open-sourcerers @silverstripe/creative-commoners

Outcomes

Protected scope IS public API

Protected methods and properties MUST NOT receive incompatible changes in minor and patch releases.

Pros

Cons

PRs

ScopeyNZ commented 5 years ago

This is a pretty worn out drum that I'm banging here, but it's completely dependant on context. I really don't think you can have "default" visibility. If it's an implementation detail of the class/object then it's private. If it's API that conforms with the intention of the class, then consider the idea that someone might wish to extend the class to update compatibility or something, and make it protected. If it's just the API of the class, then it's public.

Example:

class Lottery
{
    protected $numberOfNumbers;

    public function __construct($numberOfNumbers)
    {
        $this->numberOfNumbers = $numberOfNumbers;
    }

    public function drawNumbers()
    {
        return $this->generateNumberList();
    }

    protected function generateNumber()
    {
        return random_int(0, 45);
    }

    private function generateNumberList()
    {
        $numbers = [];

        for ($i = 0; $i < $this->numberOfNumbers; $i++) {
            do {
                $number = $this->generateNumber();
            } while (!in_array($number, $numbers));

            $numbers[] = $number;
        }

        return $numbers;
    }
}

My most complex logic which is an implementation detail is kept in a private method. I could change that at any time to return a generator instead of an array - for example.

dnsl48 commented 5 years ago

Right. Though, in your example some might argue that protected $numberOfNumbers and protected generateNumber should become private.

The first one is controlled through the constructor - children should use it. How exactly the Lottery uses numberOfNumbers and generateNumber within its generateNumberList is an implementation detail, since the generateNumberList is the implementation (private).

Otherwise, with a lack of Lottery interface, people extending the class should only be able to overwrite the constructor and public drawNumbers method.

P.S. I'm not saying that's better or something. Simply showing there are other options. Another option here could be to make a separate RNG interface and pass it through the constructor rather than have protected generateNumber. Though, that introduces another entity to the system, which is a complexity vs flexibility tradeoff, but could be done in children.

sminnee commented 5 years ago

This is a pretty worn out drum that I'm banging here, but it's completely dependant on context. I really don't think you can have "default" visibility. If it's an implementation detail of the class/object then it's private. If it's API that conforms with the intention of the class, then consider the idea that someone might wish to extend the class to update compatibility or something, and make it protected. If it's just the API of the class, then it's public.

Adjusting it slightly with some comments on tests, and turning it into a candidate paragraph for coding guidelines:


Method/property visibility guidelines

When adding new methods and properties, should you make them public, protected, or private?

Above all, recognise that when you are making something public or protected, you are creating an API for other developers to use and rely on, and do so with care. Don't simply dump class internals in as protected members in the name of extensibility – doing so will be likely to break developers' projects in minor releases.


Waddaya think?

ScopeyNZ commented 5 years ago

Perfect. Thanks for cleaning that up 😂

chillu commented 5 years ago

Sounds like this has been sufficiently discussed. Who's closing this out by making a documentation pull request?

ScopeyNZ commented 5 years ago

I've just copied and pasted the section that @sminnee wrote: https://github.com/silverstripe/silverstripe-framework/pull/9298

dnsl48 commented 5 years ago

Yeah, I think we've had plenty of time for everyone interested in the matter to vote or say something.

The current results are:

Regarding the Vote 1 - here is the follow up PR: https://github.com/silverstripe/silverstripe-framework/pull/9299

Regarding the Vote 2 - I don't think we should make any new coding conventions over what's been discussed in the Vote2, since it wasn't approved by the core committers (should be at least more than half of them present, which is not the case).

ScopeyNZ commented 5 years ago

I think what Sam wrote summarises what we discussed pretty well. Protected is API in the eyes of SemVer, use protected knowing that this is maintenance burden - something you should consider being private.

That's why I raised the PR on the docs to resolve this issue.

sminnee commented 5 years ago

I disagree that we're not ready to close this. It's been open for months. My comment has been there for a month. It's had 4 positive responses and no negative. What value is there in dragging this out further?

dnsl48 commented 5 years ago

I didn't say we're not ready to close this. I only said the Vote 2 didn't produce any outcomes (according to the rules we cannot consider any of the options to be approved by core committers).

sminnee commented 5 years ago

Right, I misunderstood. Perhaps the rules are a little cumbersome too. :-P

chillu commented 4 years ago

Reopening until PRs are merged.

robbieaverill commented 4 years ago

All merged, thanks everyone for the time to get this over the line