search

apereo / phpCAS

Apereo PHP CAS Client
https://apereo.github.io/phpCAS/
Apache License 2.0
796 stars 393 forks source link

Namespace phpCAS #188

Open ikari7789 opened 8 years ago

ikari7789 commented 8 years ago

For all intents and purposes, PEAR style packages are obsolete in favor of namespaced packages handled by a global auto loader (typically Composer).

What are the thoughts on removing the Autoloader, and namespacing phpCAS?

jfritschi commented 8 years ago

I think this could be a good idea. The big question is however how we can remain backwards compatible at least for a period of time?

@adamfranco Any thoughts on this issue?

adamfranco commented 8 years ago

Generally I support clean interfaces as these can make it a lot easier to "do the right thing". PSR4 class-naming and namespacing is a great example of this. That said, changing the class names is a breaking change for most users of the library and something we'd need to approach with care, likely bumping the major version to "2". I just went through this with the PECL/HTTP library and while the consumer changes weren't difficult touching every app that uses the library is painful and some amount of users may not upgrade quickly due to the lack of backward-compatibility, requiring at least security patches to be back-ported to the old version.

All that said, the vast majority of our public interface is methods on the phpCAS class. The other interfaces are the CAS_ProxiedService, CAS_ProxyChain_Interface, their supplied implementations and our Exceptions. We might be able to move our code under an \Apereo\PhpCas\ namespace and then provide a backward-compatiability file at CAS.php that defines things like:

<?php
include_once (dirname(__FILE__)."/bc_autoloader.php");

define("CAS_VERSION_2_0", \Apereo\PhpCas\PhpCas::CAS_VERSION_2_0);
...
class phpCAS extends \Apereo\PhpCas\PhpCas {}
interface CAS_ProxiedService extends \Apereo\PhpCas\ProxiedService {}
interface CAS_ProxyChain_Interface extends \Apereo\PhpCas\ProxyChain\Interface {}
class CAS_Exception extends \Apereo\PhpCas\Exception {}
class CAS_InvalidArgumentException extends \Apereo\PhpCas\InvalidArgumentException implements CAS_Exception  {}
...

I'm not sure if this would actually work and we need to figure out a good namespace/class-naming structure. Questions:

ikari7789 commented 8 years ago

I've created a branch in my own forked repository which has been updated to support namespacing. I had to change a couple class names to make it work due to restrictions in naming, such as "Abstract" or in places where there is already a standard class named the same thing and could create confusion, such as "Exception" <- which is an interface. I wasn't sure what to use as the top namespace when I started, so I just went with \phpCAS\CAS, but should make going forward a little easier?

I also took the liberty to correct spelling errors in comments as I went along, and I fixed the Japanese and Greek translation files which apparently lost their previous encoding at some point between the transition from SVN to Git. I've just created them both as UTF-8 now, copied from their last good revision in SVN. I may make that a separate PR just to fix the issue sooner as well.

adamfranco commented 8 years ago

Thanks for getting this started, @ikari7789. I'm pondering the pluses and minuses of going with \phpCAS\CAS as the main class or using \phpCAS\phpCAS.

On the \phpCAS\phpCAS side, this could allow many existing users to upgrade by adding use \phpCAS\phpCAS to their scripts and then updating some of the constant accessors. If they don't use the proxy system or custom PGT Storage, not much would change.

On the other side, changing that main class name could serve as bit of a test for those upgrading beyond a PHP warning that they are trying to use an undefined constant for the protocol-version. Also, would keeping the phpCAS name make it harder to implement a backward-compatability class or not?

Thoughts @jfritschi ?

jfritschi commented 8 years ago

I guess we should probably make a clean cut with 2.0 and at the same time continue to supply security/bug fixes for 1.3 stable for some time.

At some point we really have to drop all the legacy "chains" and move on to a modern setup/api.

jfritschi commented 8 years ago

The most important thing is probably that we update/rebase our 1.3-stable tree that we can still support 1.3 for some time and at the same time progress all the radical changes that are already in the working.

Is that something you can do @adamfranco ? I currently moving appartments and my whole internet/pc situation is a mess so it might be a while until I can push these things forward.

adamfranco commented 6 years ago

As noted in #256, PHP 5.6 is reaching EOL in less than a year, so at least the unit tests will need some overhaul that isn't backward-compatible with PHP 5.6 -- pushing up the pressure for a major release. I'm trying to wrap my head around outstanding issues and figure out how much refactoring might make sense to happen in a major new release: Just dropping PHP 5.6 support or also big API changes.

adamfranco commented 6 years ago

With a backward-compatibility break it might also be time to investigate what a new public API might look like. #62 touches on this a bit and it probably makes sense to discuss a future API there. Relevant to this discussion is which namespace would be appropriate for the current static-method API and where a new public API might live.

Given the shifts from Jasig to Apereo and similar over the years I'm tempted to not include \Apereo in the namespace. \PhpCas\phpCAS makes sense for the current static-method API since it will allow the addition of use PHPCAS\phpCAS; to get most simple usage working.

I'd suggest we should further discus what a new public API might look like in #62 before finalizing a solution to this issue.

Acksop commented 3 years ago

I think this could be a good idea. The big question is however how we can remain backwards compatible at least for a period of time?

@adamfranco Any thoughts on this issue?

With using semantic versionning on packagist: https://semver.org/

version 1.3.8 must be 1.4.0 and version 1.3.9 must be 1.5.0

:)

maybe this major fix need to be on version 2.0.0

Semantic Versioning
Semantic Versioning 2.0.0
Semantic Versioning spec and website