search

thephpleague / oauth1-client

OAuth 1 Client
MIT License
968 stars 73 forks source link

Version-2 for PHP7.2+ featuring Guzzle-7 #104

Closed shehi closed 4 years ago

shehi commented 4 years ago

An attempt to make this package PHP7.2+ compatible, featuring the latest versions of its dependencies as possible.

Expectations from this PR:

Further notes:

shehi commented 4 years ago

https://github.com/composer/composer/blob/28f0d8fcf7e2d6cb207b3c6ceca0e6142d141a7f/bin/composer#L22

HHVM 4.0 has dropped support for Composer

shehi commented 4 years ago

https://symfony.com/blog/symfony-4-end-of-hhvm-support

Symfony 4 dropped HHVM support ages ago as well. I suggest doing the same here, instead of wasting time with this package endlessly, which itself works with an old tech of Oauth-1

shehi commented 4 years ago

Ok, so we have PHP 7.4 incompatibility. The rest passes tests.

mfn commented 4 years ago

Try relaxing the mockery version constraint, seems to be source no. 1 of the failing tests.

shehi commented 4 years ago

I think we have to ditch PHP 5.5 as well:

 Problem 1
    - mockery/mockery 1.4.2 requires php ^7.3 || ^8.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.4.1 requires php ^7.3 || ^8.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.4.0 requires php ^7.3.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.3.3 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.3.2 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.3.1 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.3.0 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.2.4 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.2.3 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.2.2 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.2.1 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.2.0 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - mockery/mockery 1.1.0 requires php >=5.6.0 -> your PHP version (5.5.38) does not satisfy that requirement.
    - Installation request for mockery/mockery ^1.1 -> satisfiable by mockery/mockery[1.1.0, 1.2.0, 1.2.1, 1.2.2, 1.2.3, 1.2.4, 1.3.0, 1.3.1, 1.3.2, 1.3.3, 1.4.0, 1.4.1, 1.4.2].
mfn commented 4 years ago

No sure, you can try ^0.9 | ^1.1 for mockery. If lucky the dependencies sort it out itself.

But OTOH, everything not receiving security fixes should probably be dropped…

shehi commented 4 years ago

CC: @bencorlett

Ok, I have a suggestion here:

Laravel/Socialite needs Guzzle-7, which is PHP-7.2+, as Laravel 7 is.

I suggest instead of wasting time with PHP 5.x, we just go PHP-7.x, first settle dependencies, then cleanup the code a bit making it more PHP-7'ish and do a major release. PHP-5 isn't really worth all this effort. Down the line I can clean up the code myself. My personal, primary motivation is to get this package PHP-7-ready for Laravel 7. For PHP-5 users we already have v1 of this package.

So far, Mockery 1.1 works fine. Even PHP 7.4 passes all the tests.

If everyone, especially @bencorlett agrees with this, please make me a Member to this package. I will grab Version-2 branch which is incomplete, cherry pick it, cherry pick this, create a working solution for PHP-7 ecosystem and call it a day.

shehi commented 4 years ago

Took me 7-8 hours to refactor this thing. It is done now. We need someone to test it :D

bencorlett commented 4 years ago

Took me 7-8 hours to refactor this thing. It is done now. We need someone to test it :D

Hey @shehi! Thank you very much for your effort. I noticed we're targeting PHP7.1+ with some of the language features (nullable return types, void return types etc).

I agree in dropping support for PHP 5 and making this work in PHP 7. I will set aside time this week to review your PR in more detail and possibly provide some suggestions.

I would like to put some serious consideration into a newer version of this project that's rewritten. For that, we'd support say PHP 7.2+.

Thank you again :)

shehi commented 4 years ago

Hey @bencorlett !

This PR is indeed 7.2+. And it introduces absolutely nothing, just cleans the code up. One thing we need to make sure is to check whether I didn't mess up somewhere - so it needs to be tested in Live (which will be significant undertaking, considering the number of platforms this package supports - from Magento to Xing). Good luck and have fun!

arcanedev-maroc commented 4 years ago

Hi @shehi, great work :+1:

I think you should target the develop branch instead of master

shehi commented 4 years ago

@arcanedev-maroc , @bencorlett changed base to 'develop'

shehi commented 4 years ago

Nope develop is old and ugly. Tons of conflicts. Delete it, recreate fresh from master and let me know. I will change the base again.

arcanedev-maroc commented 4 years ago

I agree with you, the develop branch must be a copy of the master branch

But don't forget to add this line

shehi commented 4 years ago

Then the branch name should be the one mentioned in that line, not develop.

On Thu, Aug 27, 2020, 21:56 ARCANEDEV notifications@github.com wrote:

I agree with you, the develop branch must be a copy of the master branch

But don't forget to add this line https://github.com/thephpleague/oauth1-client/blob/develop/composer.json#L44

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thephpleague/oauth1-client/pull/104#issuecomment-682159439, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE7SAIIAT36MGBN3NYREJTSC227BANCNFSM4QGIZ2KA .

arcanedev-maroc commented 4 years ago

Hi @bencorlett, can you delete&create develop branch ? Last commit was Aug 17, 2016, it's :skull: now

Or create a new branch, something like 2.x

I'm willing to test the changes (with Laravel Socialite) and help if possible

bencorlett commented 4 years ago

I’ve spoken with other league maintainers and we are very thankful for this PR, however the scope of what it addresses is well outside of allowing a newer guzzle and php version.

The inclusion of custom Docker images etc is not something we typically chase with league packages.

I’m currently working on a rewrite of the project and will update you in the coming days on that.

Thanks! Your contributions and efforts do not go unnoticed :)

Sent from my iPhone

On 28 Aug 2020, at 9:33 pm, ARCANEDEV notifications@github.com wrote:

 Hi @bencorlett, can you delete&create develop branch ? Last commit was Aug 17, 2016, it's 💀 now

Or create a new branch, something like 2.x

I'm willing to test the changes (with Laravel Socialite) and help if possible

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

shehi commented 4 years ago

I’ve spoken with other league maintainers and we are very thankful for this PR, however the scope of what it addresses is well outside of allowing a newer guzzle and php version.

With all due respect, this is a nonsense. This PR has introduced absolutely nothing, except PHP7.2+ compatibility. If you disagree, show me a line plz.

The inclusion of custom Docker images etc is not something we typically chase with league packages.

Again, with all due respect, this isn't even an argument to make. I added it when I worked on this package. Many devs like me don't install anything on their host machines, to be absolutely independent from maintenance of such environment. As such, I needed an env to develop in. I didn't intend to have docker related stuff in this package. They are already removed.

I’m currently working on a rewrite of the project and will update you in the coming days on that.

This one needs the biggest "with all due respect" :) The only reason I did all this work is because we have tons of libraries that depend on yours, and are stuck with old version of Guzzle, not being able to move forward. You guys already "have been working on a new version" literally for several years. The current urgency is the only reason I wanted this to go through.

These being said, please close this PR if you are rejecting it - I will delete the branch and repo from my namespace afterwards.

bencorlett commented 4 years ago

Hi Shehi, thanks for the feedback.

I should add, my first two sentences are addressing the same concern - the inclusion of custom Docker images in a PHP library (not project, library) seem overkill for the aim to minimise dependencies. I see that, after my comment, before your comment, you've removed these. I think that's an acknowledgement that this is in fact an argument to make. That's great.

I am a big fan of all community contributions and I intend to do one of two things:

  1. Use this PR as-is, or
  2. Cherry pick commits out of it.

I think acknowledgement of work is important and I don't want to take your work and not keep your name on your commits. That's something which really gripes me with some open source projects.

Edit

I want to merge your MR or cherry pick commits from it, however there's one large commit at the start that includes a bunch of changes (such as the inclusion of the composer.lock file) that would be hard to mix and match.

I might start another followup MR and take from this what I can and include credit wherever possible.

I think the first step is to reconcile master vs. develop, then open a PR, cherry pick what I can and tidy up. I welcome your assistance there too. I'll update you once I have the PR up in its early form.

shehi commented 4 years ago

I want to merge your MR or cherry pick commits from it, however there's one large commit at the start that includes a bunch of changes (such as the inclusion of the composer.lock file) that would be hard to mix and match.

I fail to understand why would someone need to disect composer.lock file into pieces. It's technically parsed version of composer.json that accompanies it. Noone would commit composer.lock of an unrelated composer.json. Additionally, whenever I commit anywhere, whether it is open-source, professional life or my own pet projects, my commits intend to be atomic (meaning all of them have to be a working instance of the code, not broken one). As such, I never have commits in between where things are messed up. And I always rebase to achieve that.

Anyway, earlier I already mentioned I spent time on this PR to move forward, and am not prepared to spend more than that on this lib which I am not using at all. That still is the same. I thought this would be great starting point for any rewrites, since nothing was changed on the codebase. But as it seems the work isn't acceptable, I will wish good luck and leave this topic here. Really looking forward to a working new version as soon as possible, as there are other libs which are stuck behind because of this one. Have fun, Ben and Co.! :)