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

Add an autoloader providing backwards-compatibility #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jaimeperez
Copy link
Contributor

This PR adds a file that implements and registers an autoloader. This autoloader recognizes the old class names:

  • XMLSecEnc
  • XMLSecurityDSig
  • XMLSecurityKey

and translates them into their appropriate new names including namespaces (\RobRichards\XMLSecLibs\*), creating aliases for every class when it is loaded. For those already using namespaces, this autoloader will have no effect and won't be even run.

This allows users of the library to start using the 2.* branch and start migrating their own code to the new classes using namespaces, at their own pace. We will use it in simplesamlphp/saml2 and simplesamlphp/simplesamlphp for that purpose, for instance. For small code bases it might be ok to just bump the version required in composer.json at the same time you are making the actual changes, but for large code bases it's much easier if the migration can be split in several, smaller commits.

I would definitely consider this as temporary, though. I would set a date in the future (say one year from now, for example), and remove the autoloader then. It's time enough for people to migrate, and backwards-compatibility cannot be kept forever, or you risk people ignoring changes and continuing business as usual.

Of course we would need a new stable release of XMLSecLibs with this, if this gets merged.

Thanks in advance!

… the old class names, allowing backwards compatibility for the new branch of the library.
<?php

/**
* Temporary autoloader to ensure compatibility with old, non-PSR-2 compliant classes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PSR-0 or PSR-4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, PSR-4, you are right.

@Maks3w
Copy link
Contributor

Maks3w commented Jan 29, 2016

I think this is an edge case related with your implementation and should your code to be responsible of this.

@jaimeperez
Copy link
Contributor Author

No, it's not. How could it be providing both the old and the new interface at the same time an edge case specific to my implementation? Anyone using XMLSecLibs and having to migrate from the old classes to the new interface using namespaces will benefit from this, as things will keep working when bumping XMLSecLibs version, and migration can then be gradual.

Besides, this is also beneficial for XMLSecLibs itself. Having both interfaces in parallel at the same time means we don't need to maintain two development branches simultaneously, but just one.

@Maks3w
Copy link
Contributor

Maks3w commented Jan 29, 2016

For achieve your need the only thing needed is add class aliases in this file https://github.com/robrichards/xmlseclibs/blob/master/xmlseclibs.php No need for more autoloaders

@jaimeperez
Copy link
Contributor Author

No, not really. Adding an autoloader provides transparency to those not using that file. Remember that file was created to avoid having to load the old monolithic file, or the three of them when the classes were split. If you define the aliases there, then you will have to go back to loading that file manually, every time you want to use this library. And that means, again, huge commits changing lots of files all over the place, together with the bump in the version of XMLSecLibs you depend on. That would be obviously a regression for those who are already using XMLSecLibs with composer and PSR-0.

The only way that could be equivalent to this PR, is telling composer to load it by default, together with the PSR autoloader. But that file is not an autoloader, so it wouldn't really make sense.

@jaimeperez
Copy link
Contributor Author

By the way, I wonder why the build fails with hhvm. It just stalls and outputs nothing. Maybe it would be a good idea to allow failures there as we had to do in SimpleSAMLphp.

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

Successfully merging this pull request may close these issues.

2 participants