search

okta / okta-sdk-php

PHP SDK for the Okta API
Apache License 2.0
38 stars 71 forks source link

Replace tightenco/collect package with illuminate/collections #113

Closed jakelehner closed 3 years ago

jakelehner commented 3 years ago

In attempting to include the Okta PHP SDK in a new Laravel/Lumen package, composer was unable to resolve dependencies. I was able to track it down to a conflict within the tightenco/collect package. On further investigation, I found this package is no longer maintained now that illuminate/collections is available as a separate package.

This PR swaps out tightenco/collect for illuminate/collections which appears to be a clean swap. All tests pass.

bretterer commented 3 years ago

Thank you for the PR.

First, we would need a signed CLA for this PR which can be done through developers.okta.com/cla

Second. I will have to confirm that this will work with all of our supported PHP versions, which currently goes down to PHP 7.2. I believe that illuminate/collections is only 7.3+

jakelehner commented 3 years ago

Thanks for the quick reply. I noticed the failing Travis CI build which suggested legacy PHP support wasn't passing. It looks like the oldest available version of illuminate/collections is PHP ^7.3.

If implementing illuminate/collections is not the best approach, do you have another suggestion that would allow the okta sdk to be included in a fresh Laravel/Lumen project? The tightenco package could be updated but I'm not sure that does the trick, etiher.

Will look at the CLA link.

bretterer commented 3 years ago

It appears that we are able to drop support for PHP 7.2 in our SDK now that it is EOL and out of support as of December. This will allow the change to illuminate/collections since it allows for 7.3. I will be looking at what else needs to be done from our end, but will be able to merge in this PR after that.

jakelehner commented 3 years ago

Thanks that's great news. Do you still need me to sign the CLA? Or is this a minor enough change? Also I am an Okta customer (Workforce and CIAM) at my job and we have NDAs there of course.

Let me know if I still need to do the CLA or if I can help with this anymore.

bretterer commented 3 years ago

If you don't have an issue with signing the CLA, why don't you go ahead and do that just to be safe.

I have this investigation work slated to be done next week.

bretterer commented 3 years ago

There were a lot of other changes that were needed in order for tests to pass. I have brought your changes into #114.

Thank you for your changes!