search

simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
284 stars 134 forks source link

Migrate to PSR-2? #31

Closed cb8 closed 8 years ago

cb8 commented 9 years ago

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_AssertionProcessor).

If desired, I can supply a pull-request.

relaxnow commented 9 years ago

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 think however that this shouldn't block future development. Maybe we can fork off the 0.x branch and release a 1.0 with some breaking changes like this? Do you agree @jaimeperez ?

DRvanR commented 9 years ago

I wholeheartedly support this. At the same time PSR2 should be fully embraced and the use of uppercase TRUE, FALSE and NULL should be eradicated :wink:. Should be easy to do.

@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 :wink:

I would strongly suggest implementing this prior to a release 1.0

relaxnow commented 9 years ago

I must admit, my autoloading magic is weak, but I thought that you couldn't do new SAML2_Configuration_IdentityProvider and have it return Saml2\Configuration\IdentityProvider? Though I guess you could do runtime class generation of a subclass, or just precompile a bunch of subclasses... Hmm... adding a bwc file with subclasses might just be crazy enough to work. Does composer do that for you? I can't seem to find anything on it...

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. I think we should strictly follow semver when it comes to version numbers and go to a 1.0 whenever we decide for a breaking change in the API.

@jaimeperez thoughts?

DRvanR commented 9 years ago

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 :wink:

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 develop branch aiming for a new 2.0, which coincidentally might align with the SSP drive towards a 2.0 and the possible development on EngineBlock this year. Which would be beneficial on all accounts.

@relaxnow @jaimeperez thoughts?

relaxnow commented 9 years ago

@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.

relaxnow commented 9 years ago

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... :smile:

DRvanR commented 9 years ago

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.

jaimeperez commented 9 years ago

Thanks guys for moving this forward, I completely support you too :smile:

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.

DRvanR commented 8 years ago

Resolved through #53 and #57