At some point we made a decision in Drupal 8 to never, ever use 'private' property/method visibility. This was done because there was concern that our collective experience with OO development was still young, and we couldn't anticipate what Drupal contrib developers would/would not want to override. By making everything protected or higher, everything is overridable and no one's boxed into anything.

However, this flexibility is absolutely terrible for DX. In every single other OO framework, "private" visibility is extremely useful for helping API consumers understand which things they ought not to mess with. It also preserves our ability to refactor any private methods/properties without disrupting contrib, since enforcement of this is a language-level construct, rather than a convention (like a preceding underscore).

I think it's worth re-opening this discussion, because the whole point of D8 is to bring in people who know OO, and people who know OO are not going to remotely understand why we don't use this "OO 101" concept. Fixing this visibility in D8 now seems preferable to booting it to D9, since letting a bunch of protected methods loose means people will probably use them and then we can't fix things without breaking APIs in the future.

It also seems like switching from 'private' to 'protected,' if a proper use case is found later on, is something we could do even post-Drupal 8.0 (it'd be similar to adding a missing hook, which we do in D7 a lot). But in the meantime, the authors of these APIs could express in a standard way how they intended their APIs to be used.

Let's discuss.

Comments

xjm’s picture

Issue tags: +Coding standards
EclipseGc’s picture

So, +100000000 from me. This sounds SUPER sane.

Eclipse

tim.plunkett’s picture

I'd like to see some concrete examples of where we would use this.

I'm not for or against at this point, just curious where it might be a good thing.
We'll likely need examples anyway when trying to explain our guideline/standards.

Mile23’s picture

The use of private/protected/public (and abstract and final) is an implementation of API design.

The existence of this issue demonstrates a lack of the latter. The real issue title should be: "Design An API"

That said: Put your privates back where they belong. :-)

klausi’s picture

See also:
https://drupal.org/node/608152#visibility
http://www.garfieldtech.com/blog/private-composition-inheritance

For me it boils down to: do we want Drupal contrib to copy and duplicate code from core all over the place? Because they can't make use of private methods directly, so they have to copy them to their class. This is bad because then they don't automatically get the bug fixes from core, but this is also good because then core can make radical changes to those private methods even between minor stable releases.

So if we adopt the private pattern then we violate the DRY principle (don't repeat yourself). If we stick with always protected then we are violating the OO Information Hiding principle.

In the past we were obsessed with avoiding code duplication, are we ready to change our culture to copy & paste?

EclipseGc’s picture

I think that might be a bit of an overstatement. Ideally private methods and properties are going to be used because they're irrelevant to the API we're defining, but we'd like the ability to change the implementation details on a dime. Contrib should never have to repeat stuff we're labeling as private, if they DO need to repeat that, then we did it wrong.

Eclipse

klausi’s picture

The problem is that we don't know beforehand how our code is going to be used. It is common in Drupal land that people do amzaing stuff with the framework, which it never was intended or designed to do. Shouldn't we provide all implementation details to contrib developers that want to specifically override/reuse one part of the system?

Note that by always using protected we still have information hiding from the perspective of the API consumer (no all public arrays of doom), but leave the possibility for the API extender to access all parts of the code base. I'm sure whenever you declare something private someone else could find an obscure reason why it should be protected instead.

Could you give an example in core where private would make sense?

twistor’s picture

I think ControllerBase::container() would make sense as private. That way, subclasses are encouraged to "do the right thing" in regard to dependency injection. The helper methods would still be protected.

Mile23’s picture

The problem is that we don't know beforehand how our code is going to be used. [..] Shouldn't we provide all implementation details to contrib developers that want to specifically override/reuse one part of the system?

The whole thing is open-source, so you can look at the implementation details and see how they work. But ideally, in OOP you don't care about implementation, you only care about the service the class provides.

For a non-OOP analogue, marking a property or method as private is a bit like starting a function name with _, as in _helper_function($help_me). It means this code isn't the code you're supposed to call.

Also, there's a thing called reflection, if worse comes to worse. But someone should slap your hand with a ruler if you try that. http://www.php.net/manual/en/reflectionmethod.setaccessible.php

MyXelf’s picture

Title: Reintroduce 'private' visbility to object-oriented code. » Reintroduce 'private' visibility to object-oriented code.

Typo in the title.

Crell’s picture

As the main proponent of the "never private" camp I guess I should weigh in, seeing as how I've already been quoted. :-)

Most of the arguments for blacklisting privates have already been made here or linked to. I do want to correct a few mis-statements in the issue summary, though.

the whole point of D8 is to bring in people who know OO, and people who know OO are not going to remotely understand why we don't use this "OO 101" concept.

Untrue. A quick perusal of Planet PHP or Google will turn up dozens of articles and blog posts on a variety of sides of this issue, ranging from "privates are teh evilz" to "if you need protected at all, *you're doing it wrong!*" and everything in between. My blog post linked above links to a couple such posts, one on each side. This is not an area where "all the smart people do X, but those stupid Drupalers do Y"; it's a case where there's considerable legitimate disagreement within the PHP community (and every other language that has visibility modifiers, AFAIK; I've certainly seen it debated in Java, at least).

In the last round of debates on Planet there was an excellent article, which I sadly cannot find right now. It's basic thrust was that too many people conflate visibility with whether or not something is part of "the API". Just because a method is public doesn't mean it is part of The API(tm); just because it's protected doesn't make it part of The API(tm) for subclasses. There is/can be an API for subclasses, but just being protected doesn't make a method part of it.

The article's suggestion was as simple as it was impressive: Use @api not just for public methods that you want to flag as stable, but for protected methods, too. Protected methods that are not @api tagged are no more guaranteed to not change than public methods that are not @api tagged. That is, we'll try not to but we're not promising it. I think that's a very simple and elegant way to signal to subclasses what they "should" be doing, while still not cutting them off entirely.

Whilst Googling I also stumbled across this article by fabpot:
http://fabien.potencier.org/article/47/pragmatism-over-theory-protected-...

He makes a solid case for protected... most of the time, but does acknowledge that making something private can force the issue and force a cleaner implementation than "just subclass it", either for the consumer or the API provider (or both). However, that is contingent upon being able to refactor and tweak visibility moving forward in core, in those cases where we do find it necessary. (The comments there also show just how much there is not a clear consensus on this question, even just within the Symfony-watching world.)

Therefore, I will moderate my position to say that I can accept private methods/properties iff:

1) We don't "private all the things!" blindly, but have a rational argument for why something should be private vs protected.
2) We leverage @api to signal what we actually intend to be "the API" (even if things outside of "the API" are accessible). This is useful for both public and protected methods.
3) We adopt a more rapid, iterative, intra-version-improvement process as proposed in Prague that would give us places to do the refactoring and, if necessary, expanding of visibility in core in less than a US presidential term: https://prague2013.drupal.org/session/future-friendly-evolution-and-drup...

(I believe 2 and 3 are good things to do in their own right; 1 is simply a caution to avoid the Drupal tendency to apply a new shiny everywhere without understanding it. Software development requires more than meatsack regex application.)

tstoeckler’s picture

While I totally agree with #11 in terms of a general assessment, I think history has shown the Drupal community to be more cautious. Specifically,

Just because a method is public doesn't mean it is part of The API(tm); just because it's protected doesn't make it part of The API(tm) for subclasses. There is/can be an API for subclasses, but just being protected doesn't make a method part of it.

I think is something that will not hold up in Drupal. I'm not saying that what you say is a bad idea in a way, but I think the Drupal community in general has a different perspective. I think that all public methods are considered part of the API and for classes that are meant to be extended all protected methods are part of the API as well. That means stuff like PluginBase, ControllerBase, FormBase, and so forth. I don't think many people would consider it an API change if we refactor DrupalKernel in 8.4 and remove a bunch of protected methods, but I think e.g. ControllerBase will only be allowed to have non-BC-breaking API additions.

So going with the @api idea, I think we should declare entire classes as @api, and with that ensure that all protected methods are BC-safe. And declare all public methods as @api by definition.

Crell’s picture

@tstoeckler: What you've just said is "Drupal devs can't handle any level of nuance, so let's use @api as a completely redundant marker for 'public' so it means nothing useful."

I disagree on both the claim and the conclusion.

Mile23’s picture

An API is a process. You want to accomplish a thing, so you do a set of steps, and then you end up with the thing you wanted. Leave out steps and it doesn't work. Implement hook_schema(), implement hook_entity_info(), etcetera on down the line.

The process by which you do these things is the API, not the methods you call or the classes that define them. Some methods and classes are part of the API, but they aren't the API in and of themselves. Declaring a given method as public or protected or private only matters from an API level in that those declarations fit the design of the API.

APIs have to be documented in a narrative way. Tagging methods as @api might help with keeping them organized in some way, but having only an annotation is insufficient documentation. As excellent as api.drupal.org is, it's still insufficient documentation of the various Drupal APIs, since it has no way to show you the process that is the API. Even Examples leaves you guessing a little bit until you can start thinking in Drupalisms.

The visibility of methods and properties is an implementation detail. It's not API documentation.

tstoeckler’s picture

(Re #13:)
Hmm...
So where I'm coming from is that in the past, I've observed the following: Even though we have always declared underscore-prefixed functions as non-API functions in previous Drupal versions, we have been very reluctant to actually change, let alone remove them in point releases.
So I was just trying to imagine what that same carefulness means in the new world order.

But if you think we can actually do meaningful @api declaration for public and protected, all the better :-)

EclipseGc’s picture

The main reason to adopt private is because there are times we will know up front that we want to the flexibility to change methods/properties later and that we want a language level guarantee of that change not breaking BC. That's not to say that making such a decision won't be nuanced, but our current standard doesn't even allow for it, and we should change that.

Eclipse

catch’s picture

Is there an issue for using @api yet? I'm not sure this discussion is really resolvable unless we have more of a plan there.

Crell’s picture

benjy’s picture

I agree with @klausi, we just don't know exactly how people will use the code and what they may want to override.

From experience I can say that one of the most annoying DX issues with Magento is all it's private members. The amount of times I've seen code which replaces an entire class just to change one line is absurd and it certainly pushes you into copy and paste.

Whether this is because private was used in the wrong place or whether private shouldn't be used at all is still debatable.

I think some examples of places we might use private in Drupal would be good?

@Crell, great talk at Prague. I'm 100% for the incremental revisions and faster iterations.

tim.plunkett’s picture

#2110643: ControllerBase::container() and FormBase::container() needs to be private is an actionable issue. Can we just close this? Or do we need to update a handbook page here?

webchick’s picture

Title: Reintroduce 'private' visibility to object-oriented code. » [Policy, no patch] Reintroduce 'private' visibility to object-oriented code.
Issue summary: View changes
Status: Active » Fixed

Yeah, let's do that. It sounds like there's general agreement in doing this where it makes sense, but not going nuts with it, so let's tackle private visibility elsewhere in specific, actionable issues.

klausi’s picture

Status: Fixed » Needs work

Ok, so the decision is now that "private" is allowed in rare cases, correct?

TODO:
* We should document that on https://drupal.org/node/608152 where it currently says "The use of private methods or properties is strongly discouraged.".
* Update Coder Sniffer so that the use of "private" does not throw an error anymore.

jhodgdon’s picture

I was going to just update that page but I couldn't figure out what to say about when they should be used.

Coder -- should file a separate issue for that I think?

webchick’s picture

I think basically we should just omit that line, since it's no longer accurate. It's encouraged to use whenever you would normally use it for standard OO code, which doesn't need to be spelled out in coding standards IMO.

jhodgdon’s picture

Status: Needs work » Fixed

So we currently have this:

The use of public properties is strongly discouraged, as it allows for unwanted side effects. It also exposes implementation specific details, which in turn makes swapping out a class for another implementation (one of the key reasons to use objects) much harder. Properties should be considered internal to a class.

The use of private methods or properties is strongly discouraged. Private properties and methods may not be accessed or overridden by child classes, which limits the ability of other developers to extend a class to suit their needs.

I agree with webchick -- and a bit more -- it seems to me that all of this is standard OO general principles and can be removed? I went ahead and did this; we can always revert the edit if we need to.
https://drupal.org/node/608152/revisions/view/2739353/6786689

For coder, I filed
#2159567: Private class members should not throw errors

So I think this is fixed.

Crell’s picture

Ak! No, only the second paragraph is affected. Discouraging public properties is absolutely still the case and should remain so. Please bring that paragraph back post haste.

As I noted above, this is not an area where there is One Standard OOP Use That Everyone Knows(tm). We shouldn't rely on a convention that doesn't exist.

jhodgdon’s picture

Restored. Here's the diff between the pre-my-previous-edit version and what is there now (I added a missing hyphen in the "public" paragraph):
https://drupal.org/node/608152/revisions/view/2739353/6788011

I still think the "don't make it public" is fairly standard OO philosophy, but whatever.

Crell’s picture

That one is, moreso than protected/private; but that one Drupal developers used to D7 and earlier are more likely to do without thinking because they're not familiar with "standard OO philosophy". I want to keep it there to call it out for newer-OOP folks to help point them in the correct direction as they ramp up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ronline’s picture

According to my SCJP exams all method and variables should be kept private, until you need them to be otherwise.

Bearing in mind that we are dealing with PHP I went for googling and I found out that the same question was discussed here => http://stackoverflow.com/questions/419844/best-to-use-private-methods-or... . The answer that is checked as “right” states that you should keep private by default all methods and variables and expose only what you really need to.

Regarding the comment ref: https://drupal.org/node/2081945#comment-8136389 it is possible that we are dealing with an issue where programmers are jumping directly to coding instead of using a standard modeling methodology (UML?). Drawing classes before writing the code allows spreading the project in to class diagrams. Using a modeling methodology gives an initial overview and helps attributing the right visibility modifiers. Initial conception is time consuming but it pays back when ppl doesn't have rewrite thinks from scratch.

I see that Drupal comminity is turning to modern OOP programming and I think this is the right way to go.
Isn't it time to start to think about http://en.wikipedia.org/wiki/Object-oriented_modeling ?

ronline’s picture

Status: Closed (fixed) » Needs review
ronline’s picture

Status: Needs review » Needs work

List of php compatible modeling tools -> http://www.proyectosphp.org.es/?q=en/node/93 . My selection is http://argouml.tigris.org/ , it is open source and it runs on all operating systems.

Crell’s picture

Status: Needs work » Closed (fixed)

ronline, please don't reopen long-closed issues.

We have a policy of "protected, most of the time" to allow for an escape valve. We're building an extensible platform, not a bespoke application, which means we need to favor flexibility. OO modeling is all well and good but you're talking about a nearly-completed system; re-modeling it is now is not happening. :-) What you're talking about has already happened, to whatever degree it's going to.

That said, there was a Core Conversation in Austin around defining what constitutes an API that includes discussion of when to use privates. I will be posting a proposed policy soon, once I finish writing it, on g.d.o somewhere.

hussainweb’s picture

@Crell: Can you put in the link to that proposed policy like you mentioned in #33?