Bug #18058
closedMake private methods protected.
0%
Description
In the file classes/class.modulemenu.php the methods are marked private. This creates a problem when extending the class.
Would it be possible to have the methods marked protected?
Generally I will propose to mark methods protected and only use private when it is really needed.
(issue imported from #M7286)
Updated by Ingo Renner almost 17 years ago
which ones do you need to extend or overwrite?
Updated by Kasper Ligaard almost 17 years ago
That is hard to say: I am trying to find the correct place to propose a hook for a feature I am working on. I do that by XClassing the class, and the only have the functions in the XClass, that I am poking at. Then, in those functions I insert hooks where I think they should be and try that out.
This insulates me from the develoment in svn but makes it very easy to safely follow bleeding edge. It also makes it extremely easy to submit a request for a hook with a reason.
Because I extend the class when xclassing, I can suddenly not call any private method in the parent. The specific problem I had was that I was working on where to insert a hook in the function getRawModuleData(). This function accesses getNavigationFramePrefix(), which is private and thus I get a fatal error. Furthermore, also in getRawModuleData() the value of the private variable $loadedModules also gives a fatal error in the subclass, this I could luckily work around, since there is a public function getLoadedModules().
I would propose to prefer protected over private for functions. As illustrated above, using private makes extending by subclassing very hard. In the worst case you would have to copy-paste all code into the subclass, which makes it very brittle to changes in Typo3 proper.
I hope this illustrates the problem, please ask further, if needed.
Updated by Ingo Renner almost 17 years ago
I can understand your concerns, however I think that it is good practice to have private methods and members. As you outlined yourself there should be public getters and setters where needed instead. Private declarations are usually there for a reason, we can and should work around this with getters and setters to keep things clean.
Another advantage of having getters and setters is that the core can stay independent of backwards compatibility up to some point - we can alter the inner workings as long as we stay compatible with the defined public interface.
Updated by Kasper Ligaard almost 17 years ago
The public api is not changed at all whether private or protected is used.
I agree private have its use cases, but usually to hinder access to security sensitive methods in some classes. Since Typo3 is open source, then we do not have secret code. Instead we have a community of coders that like to experiment. Using private as the default (as opposed to protected), makes it harder to experiment decoupled from editing directly in the source files.
Anyways, it seems to me this discussion should be taken more broadly. What will Typo3 5 do in this regard? Well, I am hung up at the moment, so I will leave it here.
PS: PHPUnit changed the default to protected, since developers found it hard to integrate with private.
PPS: Why is there a public getLoadedModules() function in this class? Shouldn't this be consolidated in class.t3lib_loadModules?
Updated by Ingo Renner almost 17 years ago
TYPO3 5.0 will have other ways to extend the core - think AOP and dependency injection.
PS: ok, we'll think about it, but it'd sure be ok to bring this up at the dev list
PPS: probably, maybe I put it there as a getter method, can't remember right now without looking at it.
Updated by Kasper Ligaard almost 17 years ago
We discussed this in out dev-team this morning, and ended up undecided. I think this issue can be safely closed for now.
Updated by Ingo Renner almost 17 years ago
ok, thanks anyways. As I said already I'm fine with discussing this at the dev list.
all the best
Ingo
Updated by Kasper Ligaard over 16 years ago
With the release of Typo3 4.2 RC1, where private methods are changed to protected, this issue should be solved as 'Fixed' (instead of 'suspended').