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

Replace stella-maris/clock with psr/clock #651

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

p4veI
Copy link

@p4veI p4veI commented Nov 25, 2022

Hi, I was wondering if this library could replace the stella-maris/clock dependency with the newly accepted phpfig psr-20 interface https://www.php-fig.org/psr/psr-20/ for clock.

@lcobucci lcobucci self-assigned this Dec 7, 2022
@lcobucci
Copy link
Owner

lcobucci commented Dec 7, 2022

@p4veI sure thing! I'll release v2.3 later this week with the newest stella-maris/clock version (@heiglandreas was kind enough to also implement the PSR there) so we have a migration path. Then I'll rebase your PR and ship v2.4 v3.0 👍

@lcobucci lcobucci added this to the 2.4.0 milestone Dec 7, 2022
@heiglandreas
Copy link
Contributor

Will that break any user-code that depends on this library extending stella-maris/clock? As this is a BC-breaking change IIRC

@p4veI
Copy link
Author

p4veI commented Dec 7, 2022

@heiglandreas imo it's not a BC, but it might depend on the configuration in user projects. I'm using the Clock interface for configuration currently so it's not dependent on the interface it's extending (e.g. psr).

But essentially I'd like to configure the psr interface as an alias to lcobucci/clock implementation and reduce code dependency on this package.

@heiglandreas
Copy link
Contributor

heiglandreas commented Dec 7, 2022

You can already configure lcobucci/clock as implementation of psr/clock.

you could the moment PSR20 was available on packagist.

Re. the BC break:

When someone uses an implementation that so far implemented an interface and drops that interface in a minor version, then the code suddenly breaks after updating to the next minor. At least according to SemVer.

Introducing an interface in a minor: Fine.
Removing an interface in a minor: Less so...

And while I appreciate your effort to reduce dependencies, you can only reduce your direct dependencies. You can do that by using PSR instead if the stella-maris implementation. I encourage everyone to do that. But that should not be extended to your dependencies dependencies...

But that's just my (biased) 0.02 €

@p4veI
Copy link
Author

p4veI commented Dec 7, 2022

@heiglandreas hmm, I'm not sure if I understand.. I guess it's possible with the update @lcobucci mentioned, once stella-maris/clock interface actually implements the psr interface (which as I see now it already does)?

From what I checked when I prepared this PR it was only implementing stella-maris/clock interface which was not extending the psr interface. That's why I did this PR.

I agree with the semver breaks updating though, I'm not really suggesting anything regarding to the version this should be released in, but I feel it's not really necessary to implement an interface of another package that extends the psr interface, I'd just like to see this implementing the psr interface directly.

@heiglandreas
Copy link
Contributor

stella-maris/clock extended psr/clock about 4 hours after that was available on packagist.

And I'm with you that all projects should now use psr/clock as dependency. Sadly we all know that that is not happening always.

Therefore the libraries that are currently extending stella-maris/clock should do so until their next major version to avoid BC-breaks in the projects using them that have not yet updated to psr/clock.

For your use-case: Make lcobucci/clock an alias to psr/clock and everything should work out fine 😁

@lcobucci lcobucci modified the milestones: 2.4.0, 3.0.0 Dec 19, 2022
@lcobucci lcobucci changed the base branch from 2.3.x to 3.0.x December 19, 2022 14:44
@lcobucci lcobucci merged commit 8133d6c into lcobucci:3.0.x Dec 19, 2022
@lcobucci
Copy link
Owner

Merged and released on https://github.com/lcobucci/clock/releases/tag/3.0.0 🎉
Thanks @p4veI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants