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

Migrate to PSR-2 #53

Closed
wants to merge 14 commits into from
Closed

Migrate to PSR-2 #53

wants to merge 14 commits into from

Conversation

cb8
Copy link
Contributor

@cb8 cb8 commented Nov 29, 2015

Based on #31 I converted the project to PSR-2. Basically the following things have been done:

  • Use namespaces (SAML2_DOMDocumentFactory -> SAML2\DOMDocumentFactory)
  • Update XMLSecLibs (provides equal functionality but with namespaces)
  • Rename SAML2_Const to SAML2\Constants as const is a reserved keyword.
  • Convert all keywords (true, false & null) to lowercase.

@DRvanR
Copy link
Contributor

DRvanR commented Dec 15, 2015

Wow, awesome work! Thanks!

Personally, I'd import root classes (e.g. use Exception; rather than throw new \Exception(/*...*/)) as well, as that makes it easier to see which classes are used by looking at the imports (use statements). Perhaps something best discussed in a separate issue though.

I'd like to see this merged asap. A rebase on master should help resolve the conflicts. It would have to be a new major though, as it is a BC break.

What I would suggest is:

  • Stop releasing 0.x releases
  • Create a new release-1.x branch from master, allowing to still apply fixes or backport issues for the 1.x versions of this library.
  • Merge this branch into master
  • Create a new release-2.x branch from master, and use that for tagging of releases 2.x going forward, using master for development.

Thoughts @jaimeperez /cc @thijskh @relaxnow?

@thijskh
Copy link
Member

thijskh commented Dec 16, 2015

👍

@jaimeperez
Copy link
Member

That is indeed some very impressive job! I pretty much agree with @DRvanR here, and this is something I would really like to see in SimpleSAMLphp 2.0. It's a pretty big change, though, so I think we need to be very careful, and of course fix the conflicts first.

@relaxnow
Copy link
Contributor

👍 Conflict needs to be resolved. Could you do that @cb8 ?
After that both SSP and EB need to be migrated. Maybe we can look into that at the meetup in january (@edvries willing) ?

Also:

  • Stop releasing 0.x releases

I would very much like to, but we have simplesamlphp/simplesamlphp as a dependency and their stable version depends on "~0.3". So I can't use newer versions in EB unless I tag a new 0.* release.

SSP2 will fix that when it is released, also our need for using SSP (Profile) is going away so that will resolve itself shortly.

@DRvanR
Copy link
Contributor

DRvanR commented Dec 17, 2015

Also:

Stop releasing 0.x releases

I would very much like to, but we have simplesamlphp/simplesamlphp as a dependency and their stable version depends on "~0.3". So I can't use newer versions in EB unless I tag a new 0.* release.

You can, simply use inline aliasing as we do in the 5.x branch. Having two release branches releasing the exact same version should be stopped sooner rather than later 😉

After that both SSP and EB need to be migrated

I think that should be considered separate from this PR and should not be blocking, also the reason why I suggested to move to a new major. This allows fixes to be applied to both releases, while not impeding the separate development of this library.

I'd really like to make a push forward here. We indeed should take care not to break things, but apart from that to me there is no blocking reason to delay this any further than after the conflict resolution 😄

Thoughts?

@thijskh
Copy link
Member

thijskh commented Dec 17, 2015

I also don't see a blocker to go forward with the plan that @DRvanR proposes.

We can declare 0.x to be fixed only for truly critical issues that actually affect the remaining usage we know of for how long it takes to migrate those away. That is probably not often, if at all.

@relaxnow
Copy link
Contributor

You can, simply use inline aliasing as we do in the 5.x branch. Having two release branches releasing the exact same version should be stopped sooner rather than later 😉

Ah, nice, I'll swtich to that then and stop releasing 0.x releases 😄

I think that should be considered separate from this PR and should not be blocking, also the reason why I suggested to move to a new major. This allows fixes to be applied to both releases, while not impeding the separate development of this library.

Yes well, I'd like to see actual time commitment to porting SSP because a saml2 lib that isn't used by SSP is useless for us. It would not be the first time in OpenConext history that we end up with an 'next version' that was in effect a fork.

We can declare 0.x to be fixed only for truly critical issues that actually affect the remaining usage we know of for how long it takes to migrate those away. That is probably not often, if at all.

Good point, only patch releases from 1.7.x and no backports unless someone requests it 😄

@DRvanR
Copy link
Contributor

DRvanR commented Dec 17, 2015

Yes well, I'd like to see actual time commitment to porting SSP because a saml2 lib that isn't used by SSP is useless for us.

Why is a SAML2 lib not used by SSP useless? If it is used by other projects (as it currently is) it seems to me it is useful. I really do think that other projects should not be a limiting factor for this project.

It would not be the first time in OpenConext history that we end up with an 'next version' that was in effect a fork.

Introducing namespaces can hardly be considered a fork, especially if the commitment is made that for as long as needed fixes can be backported.

@DRvanR DRvanR closed this Dec 17, 2015
@DRvanR DRvanR reopened this Dec 17, 2015
@DRvanR
Copy link
Contributor

DRvanR commented Dec 17, 2015

woops, wrong button, sorry 😳

@thijskh
Copy link
Member

thijskh commented Dec 17, 2015

I do agree that there's significant value to SSP being an active user of the 'main' branch of this project. However, I think there's no doubt that SSP fully intends to migrate to PSR-2. Jaime has even committed explicitly to the migration on the SSP side in #31.

@DRvanR
Copy link
Contributor

DRvanR commented Dec 17, 2015

Summarizing:

There are two major stakeholders, SSP and OpenConext-engineblock. SSP has already committed to the migration as outlined by @thijskh. With regards to OpenConext-engineblock, the commitment will be made as part of the 5.x development effort (of which I will be a part). The 4.x range of OpenConext-engineblock will move to the 1.x branch using an alias so that it can be used together with SSP.

With respect to the different major versions:

The 0.x series will be discontinued with the caveat that it will receive critical fixes should it be required from users of that major, with the recommendation to move to the 1.x series as they are the same.

The 1.x series will continue to receive fixes and, if required, backports of functionality until SSP and OpenConext-engineblock have migrated to the new 2.x series.

After this PR has been merged, the 2.x series will be started.

If there are no objections I propose the following actions to be taken:

  • Update the README to recommend adopting the 2.x series, alternatively the 1.x series, marking the 0.x series as discontinued (I'll create a PR for this)
  • Create a new release-1.x branch from master (I'll take care of this)
  • The conflicts in this PR should be resolved as soon as possible (is that possible @cb8?)
  • After the above three points have been done, this PR can be merged to master, after which a new release-2.x branch will created and a new 2.0.0 version released (I'm willing to do this as well)

Thoughts? Ideally I'd like to do this all today or tomorrow, to prevent possibly creating more conflicts 😉

@thijskh
Copy link
Member

thijskh commented Dec 17, 2015

thought = I agree

@relaxnow
Copy link
Contributor

👍

@jaimeperez
Copy link
Member

👍

@DRvanR
Copy link
Contributor

DRvanR commented Dec 22, 2015

Closed through #57

@DRvanR DRvanR closed this Dec 22, 2015
@DRvanR
Copy link
Contributor

DRvanR commented Dec 22, 2015

Thanks for the work @cb8!

@DRvanR DRvanR mentioned this pull request Dec 22, 2015
@cb8 cb8 deleted the psr2 branch March 27, 2016 21:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants