-
Notifications
You must be signed in to change notification settings - Fork 135
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
Migrate to PSR-2? #31
Comments
I agree, however this would be a breaking change and the largest contributors of this project (SURFnet.nl and UNINETT.no) have large legacy code bases (simplesamlphp and to a lesser extent OpenConext EngineBlock) that are unlikely to upgrade quickly. |
I wholeheartedly support this. At the same time PSR2 should be fully embraced and the use of uppercase @relaxnow how would this be a breaking change? Both versions would be PSR0 compliant - all that could be needed might be a change to the autoloader, in which case I suggest using a composer generated one 😉 I would strongly suggest implementing this prior to a release 1.0 |
I must admit, my autoloading magic is weak, but I thought that you couldn't do Also, 1.0 is just a number... I mean it's already used in 2 major SAML products (soon with Step Up a third) and in countless production environments. @jaimeperez thoughts? |
You're right about the autoloader, but I figured that you could just change this to namespaces, optionally update the autoloader if needed to be PSR0 compatible and just do a search and replace in simpleSAMLphp and EngineBlock - a PR can be opened for that, I'm more than willing to do the work on that one. It's easily done and at the same time identifies any tight coupling to a vendor which should be resolved anyways 😉 W.r.t. to the 1.0 I just misread/misunderstood, I thought you meant creating a 1.0 and then doing this, not doing this and than releasing 1.0. However with semver in mind, either a 1.0 should already have been released and this would be a 2.0 (arguably the case here, since afaik it is being used in prod envs), or we should not worry about making it 1.0 since we're still in the development phase (the 0.y.z versions) and then there are no guarantees about the API or BC (see point 4). I would strongly recommend releasing 1.0 asap and incorporate this into a @relaxnow @jaimeperez thoughts? |
@DRvanR you make some good points. I have retroactively fixed the version numbers by adding the corresponding 1.x version numbers. If you would, please see about converting the library to PSR-2 and opening PRs. |
I think after even Rob upgraded xmlseclibs to use PSR-2 we should do the same, all that remains is for someone to actually do it... 😄 |
Actually still have a WiP branch that is like 80% done.... Will update and finish it, not sure if I can do that this week, but should be able to within a week or two. |
Thanks guys for moving this forward, I completely support you too 😄 xmlseclibs has already a branch (actually, master) implementing namespaces. I don't know if it's fully PSR-2 compliant, but my plan is to migrate to it right away. I can do the same with the SAML2 library as soon as it is ready. Meanwhile, some new classes in SimpleSAMLphp are using namespaces directly. Regarding the autoloader, you are right. Theoretically, I think it could be possible to rename a class in the autoloader, basically by defining the class you are looking for (the one with underscores) dynamically, based on the class defined with namespaces. However, I haven't tried this myself and that solution would be an overkill IMHO. We could also keep the old classes as wrappers for the renamed ones, but that would also be a huge work. So if we can move the entire library to PSR-2, including namespaces, I commit myself to update all the code needed in SimpleSAMLphp to use the new classes. |
As the first PHP version which supported namespacing is already EOL, I think it's time to move on and use proper namespaces (like SAML2\Assertion\Processor instead of SAML2_Assertion_Processor).
If desired, I can supply a pull-request.
The text was updated successfully, but these errors were encountered: