Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespace phpCAS #188

Open
ikari7789 opened this issue Apr 26, 2016 · 9 comments
Open

Namespace phpCAS #188

ikari7789 opened this issue Apr 26, 2016 · 9 comments
Milestone

Comments

@ikari7789
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • Will the compatibility layer (like above) actually work?
  • Do we use \Apereo as the root namespace or not?
  • While we're at it, should the main public class still be named phpCAS or should we rename the current CAS_Client class to something else and use the name \Apereo\PhpCas\Client as the name of the main public-interface class?
  • Are there any other class/interface/function names we should clean up for consistency while making this change?

@ikari7789
Copy link
Contributor Author

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
Copy link
Contributor

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 ?

@adamfranco adamfranco added this to the 2.0.0 milestone May 3, 2016
@jfritschi
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

Acksop commented Feb 24, 2021

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 spec and website

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants