search

thephpleague / oauth1-client

OAuth 1 Client
MIT License
968 stars 73 forks source link

Adding branch alias for develop (v2) #110

Closed arcanedev-maroc closed 4 years ago

bencorlett commented 4 years ago

Hi @arcanedev-maroc :) I'm actually thinking we won't need to bump a major version to release what's available in develop currently - we haven't changed any APIs but have dropped support for the older PHP version. I was originally thinking this might break semver (the dropping of the old PHP version) but after looking around a few other OSS projects, where they've done the same thing, they've bumped a minor version.

What are your thoughts?

arcanedev-maroc commented 4 years ago

Hi @bencorlett,

For the develop branch, this is about the repo management. As a maintainer, i create develop branch in most of my projects to progress & accepting breaking changes.

And you can mention this branch structure to your contributors before sending PRs:

Why v2 is an alias for the develop? Nobody knows when the v2 is gonna be released/available, but i'm 100% sure that develop branch is gonna be the next major release because it contains breaking changes.

If i have a project that depends on yours, i can start my develop branch based on your next release of your project even if it's not released.

{
    "name": "arcanedev-maroc/my-oss-project",
    "required": {
        "league/oauth1-client": "^2.0"
    },
    "minimum-stability": "dev",
    "prefer-stable": true
}

This allows me to test the latest changes from your develop branch and see what are the broken parts in my project, fix them and also contribute by testing your project with our projects & sending feedback.

And i don't need to change my composer.json file when oauth1-client v2 is available.

Without the alias, my composer file will look something like:

{
    "name": "arcanedev-maroc/my-oss-project",
    "required": {
        "league/oauth1-client": "dev-develop"
    },
    "minimum-stability": "dev",
    "prefer-stable": true
}

This is the main goal of my PR.

About dropping PHP support, this is a little bit tricky.

This is the semver's summary (https://semver.org):

This is the symfony's semver (https://symfony.com/doc/current/contributing/community/releases.html):

In my perspective, the semver is like a promise towards our community/developers/users:

A promise (for example): If you use a composer's constraint to allow only one specific major version v1.x = You will get all our minor and patch releases without breaking your application.

If you release the drop PHP support as a minor release, this will break your promise towards your users which are still using old PHP version and they will not getting the latest minor & patch releases after the drop.

The PHP was bumped to >=7.2 that translates that users on PHP 7.1 or less will not receive the latest 1.x releases

This is why it's a tricky situation

Bumping the PHP version will look harmless for your project. But if the users do the same thing with their projects just to be able to install a dependency with the latest minor release?! This is a risky task to do because it may set their application on fire.

These are my thoughts, with a broken english :smile: .

But I hope this gives you some insight

arcanedev-maroc commented 4 years ago

About the branch structure. When you think that the develop is ready to be released as v2.

You save the master as a v1.x branch You merge the develop into master (+ release a new version) And you keep develop for the next major release v3

Result:

And you repeat the process for each major releases...

mfn commented 4 years ago

@arcanedev-maroc I've never seen such a great summary about branch alias and then sideway into semver; in general very insightful and "food for thought", thank you!

bencorlett commented 4 years ago

That's a great write up. I agree with your points.

Although, I am thinking because we're not changing the public API in terms of the PHP 7.2 dependency bump, this does not justify a major bump.

If somebody has an older version of PHP, they're simply not going to receive further updates.

Bumping the PHP version will look harmless for your project. But if the users do the same thing with their projects just to be able to install a dependency with the latest minor release?! This is a risky task to do because it may set their application on fire.

If they explicitly try pull in a newer 1.x version, they'll have to upgrade their PHP version. If they do this and break their app, the onus is on them to resolve this. I see that outside the scope of this project by a long shot.

I think I'll push two new versions, which is in part why I closed #104. The first version, which will be a patch release, will include the commit for allowing both Guzzle 6 and Guzzle 7. The second version will include the bump of our own dependencies. This should resolve the case where somebody is running a version lower than PHP 7.2 and Guzzle 6. If they were to update to Guzzle 7, they'll require PHP 7.2 anyway.

I've been composing version 2 in another PR and I think once I merge the first iteration of this in, I'll merge this PR in.

Thanks again, and I'd love your feedback on my other PR once I've moved it from a draft state into a ready to merge state. Note, that won't be the final version of v2, just the first fairly complete prototype.

arcanedev-maroc commented 4 years ago

Hi @bencorlett,

Just to make it clear, my PR wasn't created because of the PHP support drop. It was created to alias a branch.

What i'm trying to explain, having branch structure like master and develop allows us to:

I'm not arguing that PHP support drop must be tagged as a major release as well! I talked about it because you ask me to share my thoughts.

I've just checked the reason behind it (PR #107), and i see that you merged it in develop instead of master even it's not a breaking change.

Here is an example about branches structure: https://laravel.com/docs/7.x/contributions#which-branch

If they explicitly try pull in a newer 1.x version, they'll have to upgrade their PHP version. If they do this and break their app, the onus is on them to resolve this. I see that outside the scope of this project by a long shot.

Frameworks like symfony or laravel will not make the same decision :smile:

arcanedev-maroc commented 4 years ago

Moved to: https://github.com/thephpleague/oauth1-client/pull/111/commits/e9ee7ed819f89b91ec7f9258b2b9a53be98b5ddd

bencorlett commented 4 years ago

@arcanedev-maroc I was planning to still use your PR here. Although I am not bumping a major version because we've required a newer version of PHP (more on that - I've relaxed the version constraint from PHP 7.2 to 7.1 because there's really nothing we're using which requires specifically PHP 7.2 except that it's still supported).

If you wanted to reopen this, I'd love to merge your contribution in prior to my other MR…

bencorlett commented 4 years ago

MR -> PR. I use GitLab for work so I'm used to that terminology!

arcanedev-maroc commented 4 years ago

I've deleted the forked repo because it will be duplicated compared to https://github.com/thephpleague/oauth1-client/pull/111/commits/e9ee7ed819f89b91ec7f9258b2b9a53be98b5ddd !!

Created new PR: #113