private vs. protected variables & functions

[fh]

Hello there,

I need to expand three function calls within phpCAS and phpCAS::Client due to some internal CAS extensions.

To be able to implement this, I need to be able to overwrite those functions with a child class - so I changed the phpCAS function visibility (right now just plain stupid for all privates):

https://github.com/effhaa/phpCAS/commit/6f7d1ef268c6139eea3e1302f8402d4b3e997d63

Is there any reason for having so many important functions private, or is it fine if I supply the changes above as a pull request?

Cheers,
Florian

[jfritschi]

Well our intention was to be really tight on visibility of function. Every function/variable we expose as public or protected has to be maintained over time. We have many people misuse the API in the past because they didn't know better. We have also had many issues reported subsequently for 3rd party integration code that was basically wrong. That's why we are trying really hard to have a clearly defined public API and our private code that we can mess with at will.

That's why I'm really sceptic to such a commit. Looking briefly at you commit it looks like you want to public every private function/variable. I don't think really goes against the strategy we have used over the last few year. The main objective was to either expose hooks for people to hooks their custom code or have Interface published that people can extend to fit their needs.

Maybe you can give me a brief overview what you are trying to achieve or what your goal is? Then we can try to find the best way forward.

[fh]

Yes, I can understand this reasoning (and just moving all functions to be protected was pretty much a quick shot on my side to fix an issue I had here).

I am happy to discuss some more details, but since this involves a project for one of my customers, I would prefer to do so in a more private way, if there is some kind of a mail contact..

The basic issue I have is some extension for the URL generation - I had to expand _getServerBaseURL() and some public methods.

[jfritschi]

You can find my mail address in the inline code docs. Happy to discuss it privately.

[kynx]

+1 for being able to swap in a subclassed CAS_Client. Pretty much the same use case here - I want to pass some additional parameters to CAS (like iframe=1 so I can embed the login page).

OK, so it's pretty easy to add a setClient() method to phpCAS.php, but it feels ugly and some twit is bound to overwrite it when a new version is released.

Locking stuff down with private doesn't stop people doing something stupid, it just makes all inheritance come via the clipboard ;)

[adamfranco]

@fh Is there further work needed on this issue or do you have a solution in place?

[fh]

@adamfranco We manually patched the class and are using that one, so for us, this is obviously solved. But I am quite certain it still would help other projects.

[adamfranco]

As mentioned by @jfritschi , the phpCAS project had for years struggled to make significant code improvements that didn't result in breaking things for existing users -- many of whom extended the CAS_Client in very unexpected ways. The split between a documented public API and the private internal code that we tackled in PHPCAS-126 has allowed development of phpCAS to significantly increase pace while successfully preserving backward compatibility for users. Features such as unit testing (PHPCAS-109), refactoring of the request-handling and proxy systems, and future work such as refactoring the large and complex CAS_Client class into smaller, more focused pieces ( #62 ) are all dependent on making significant internal changes that preserve the behavior of the external, public API. Users are welcome to patch their copies of phpCAS to allow replacing any given function or the CAS_Client implementation, however encouraging that would put us more on the hook for breaking things when we refactor and would hamper improvements to the project over the long term.

Because of the problems noted above, I'm going to close this issue as "Won't Fix". Rather than simply making all of the internals public, I encourage anyone who feels they need to extend and override the operation of the CAS_Client to create a new issue related to the specific goal of the modification and think about how such a change in phpCAS behavior might be exposed through the public API. For example, if _getServerBaseURL() needs to be customizable, maybe we can add an API method that allows injection of a callback that can re-write that url (as well as potentially rewrite other URLs created internally). By improving the API in this way we can allow the code to continue to be improved internally while covering an ever-greater set of use-cases.

Best,
Adam

[fh]

@adamfranco This is fine for me, and makes a lot of sense. I would love to add more information about the specific use cases I had, but I am no longer contracting for the company I created that issue for, and do not have an easy way to ask for an NDA release. Sorry for that.