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
Comment #1
xjmComment #2
EclipseGc CreditAttribution: EclipseGc commentedSo, +100000000 from me. This sounds SUPER sane.
Eclipse
Comment #3
tim.plunkettI'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.
Comment #4
Mile23The 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. :-)
Comment #5
klausiSee 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?
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #7
klausiThe 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?
Comment #8
twistor CreditAttribution: twistor commentedI 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.
Comment #9
Mile23The 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
Comment #10
MyXelf CreditAttribution: MyXelf commentedTypo in the title.
Comment #11
Crell CreditAttribution: Crell commentedAs 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.
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.)
Comment #12
tstoecklerWhile I totally agree with #11 in terms of a general assessment, I think history has shown the Drupal community to be more cautious. Specifically,
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.
Comment #13
Crell CreditAttribution: Crell commented@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.
Comment #14
Mile23An 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.
Comment #15
tstoeckler(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 :-)
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedThe 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
Comment #17
catchIs there an issue for using @api yet? I'm not sure this discussion is really resolvable unless we have more of a plan there.
Comment #18
Crell CreditAttribution: Crell commentedThere is now: #2116841: [policy] Decide what the Drupal Public API should be and how to manage it
Comment #19
benjy CreditAttribution: benjy commentedI 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.
Comment #20
tim.plunkett#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?
Comment #21
webchickYeah, 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.
Comment #22
klausiOk, 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.
Comment #23
jhodgdonI 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?
Comment #24
webchickI 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.
Comment #25
jhodgdonSo we currently have this:
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.
Comment #26
Crell CreditAttribution: Crell commentedAk! 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.
Comment #27
jhodgdonRestored. 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.
Comment #28
Crell CreditAttribution: Crell commentedThat 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.
Comment #30
ronline CreditAttribution: ronline commentedAccording 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 ?
Comment #31
ronline CreditAttribution: ronline commentedComment #32
ronline CreditAttribution: ronline commentedList 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.
Comment #33
Crell CreditAttribution: Crell commentedronline, 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.
Comment #34
hussainweb@Crell: Can you put in the link to that proposed policy like you mentioned in #33?