Skip to content

Commit

Permalink
Implement forward compatible Clock-Interface
Browse files Browse the repository at this point in the history 
The stella-maris/clock package provides an interface based on the
currently proposed status of PSR20. Due to the inactivity of the PSR20 working
group this is a way to already provide interoperability while still
maintaining forward compatibility. When the current status of PSR20 will
be released at one point in time the stella-maris/clock package will
extend the PSR20 interface so that this package becomes immeadiately
PSR20 compatible without any further work necessary. In the long run the
stella-maris/clock package will then be marked deprecated so that people
can then use the PSR20 provided implementation.

Should the implementation of PSR20 change between now and a possible
release then this interface will still exist and a possible
implementation will need more code-changes anyhow so this interface will
still provide some way of interoperability.
  • Loading branch information
@heiglandreas
heiglandreas committed Apr 17, 2022
1 parent c6dc658 commit fddf1b1
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 2 deletions.
3 changes: 2 additions & 1 deletion composer.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
}
],
"require": {
"php": "^8.0"
"php": "^8.0",
"stella-maris/clock": "^0.1"
},
"require-dev": {
"infection/infection": "^0.26",
Expand Down
3 changes: 2 additions & 1 deletion src/Clock.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
namespace Lcobucci\Clock;

use DateTimeImmutable;
use StellaMaris\Clock\ClockInterface;

interface Clock
interface Clock extends ClockInterface
{
public function now(): DateTimeImmutable;
}

7 comments on commit fddf1b1

@janklan
Copy link

@janklan janklan commented on fddf1b1 Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really curious what's the point of introducing the stella-maris/clock Composer dependency.

https://github.com/lcobucci/clock/blob/2.3.x/src/Clock.php extends the interface just to rename it. The interface itself is literally public function now() : DateTimeImmutable;

@heiglandreas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about interoperability.

Projects can require an implementation of StellaMaris\Clock\ClockInterface and this library can provide that implementation. But so can other implementations. So the project itself is no longer limited to this library but can use any library out there that imlpements the StellaMaris\Clock\ClockInterface.

So why not completely replace the Lcobucci\Clock\Clock-interface? As it seems just unnecessary?

That would be a BC-break as then all projects that so far required an implementation of Lcobucci\Clock\Clock would then not be compatible with this minor release.

So to have both possibilities Lcobucci\Clock\Clock extends the StellaMaris\Clock\ClockInterface.

Yes, you are right, we could have removed the code inside the interface as that is duplicate code now. I wanted to keep the impact as minimal as possible and as the inderface-declaration is the same in both interfaces it doesn't really matter.

So if you want to, check out all the implementations of StellaMaris\Clock\ClockInterface and create PRs to remove the duplicate code. Sounds like a fun thing to do for hacktoberfest 😉

@heiglandreas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. And hopefully StellaMaris\Clock\ClockInterface will at some point just extend Psr\Clock\ClockInterface 😉

@janklan
Copy link

@janklan janklan commented on fddf1b1 Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read through https://andreas.heigl.org/2022/04/21/watch-your-clock/, https://github.com/php-fig/fig-standards/blob/master/proposed/clock.md, the readme in stella-maris/clock, the code, this code, that code again, all of that a few times. Despite feeling like I'm missing the big picture, I think I did my due diligence trying to understand the point. I still don't.

Let me rephrase: I understand and agree with the need for a common interfaces for sake of interoperability. What I don't get is "why would this particular interface have to be sourced from a third-party composer package like it is now?"

If the interface was huge, I'd say "great, let's do it". It's not. It's a ->now() method that returns a DateTimeImmutable.

Why not simply add Lcobucci\Clock\Clock\ClockInterface and implement it in Lcobucci\Clock\Clock? Why instead add another link into the supply chain, extending its possible attack surface? I would argue it's completely arbitrary, and unjustified. Perhaps we can call it rushed.

Is there some real need to have a PSR draft implemented while it's in draft? Adding stella-maris/clock feels like more like a marketing stunt, and something along the lines of https://xkcd.com/927/, unless you plan to submit a PR removing this package when the real PSR20 is out.

Anyway, I don't mean to sound disrespectful. This package just got my attention when it popped up while upgrading composer deps. I've never heard of it, so I started digging into my dependency chain. When I saw what the package does, I wrote the first comment in this thread. Maybe I'm too thick to see the bigger picture here :)

@lcobucci
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janklan did you notice in https://github.com/php-fig/fig-standards/blob/master/proposed/clock-meta.md that both @heiglandreas and I are part of the working group of psr-20?

Circumstances are delaying the approval process for it and we wanted to get the benefits of interoperability despite fig's flow. @heiglandreas is not creating yet another standard, the plan is to phase the intermediate package once psr-20 is out there.

@janklan
Copy link

@janklan janklan commented on fddf1b1 Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lcobucci yep I noticed that.

If the plan is to nudge people away from stella-maris/clock by deprecating StellaMaris\Clock\ClockInterface when PSR20 is out, I think it's worth noting the fact in the package's readme file. Its contents, as it stands today, doesn't suggest in any way the package is meant to be a temporary solution.

Instead, the post-PSR20 possibilities specifically say that either "this interface will extend the PSR-20" (= package will stay in place, adding zero value, extending the supply chain), or "this interface will simply coexist with the PSR20 one." (= package will suggest a competing standard)

Just to clear it out, I don't question the need for a common interface, I question whether a whole new composer dependency is necessary, considering how tiny the actual interface is.

Thanks for your response.

@heiglandreas
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all let me express that I am terribly sorry that the choice to host the package on gitlab has caused havoc during your dependency upgrade. This has been taken care of by now.

Regarding the interface itself: It is simple but a separate interface is needed for interoperability. And to make it truly interoperable this is needed as separate package.

The stella-maris/clock-package is this separate interface that allows using different clock-packages in a project. Each of these clock packages defines an individual interface, in all these packages this interface is the same. Hence it was easy to create a unified interface.

Which was also what the working-group for PSR20 came up with. Due to reasons the PSR20 interface though didn't get published as fast as we all hoped for. Hence, to make interoperability possible now (pun intended) I created the interface.

Regarding the postPSR-20 possibilities, there still is a slim chance, that the PSR20 interface will differ from the interface that stella-maris/clock defines. That in itself is not a big deal. But that would also mean that it differs from the above described already used interfaces in the different Clock-libraries. Which means that the clock libraries - if they want to be PSR20-compatible - need to reimplement a new interface anyhow. In that case the current situation would still allow interoperability with the current code. And this interoperability would continue to work as the stella-maris/clock interface will continue to exist. As it is not creating a competing standard but maps the currently - already by multiple libraries used - standard. In the here described situation the PSR-20 would actually be the "competing standard"

So this small and tiny additional library makes it right now possible to swap different libraries that all provide the same functionality to a project.

If that is not your need then - as stated above - I'm sorry for the inconvenience that the infrastructure choice meant for your project.

And regarding

Just to clear it out, I don't question the need for a common interface, I question whether a whole new composer dependency is necessary, considering how tiny the actual interface is.

If you think about this perhaps one more time, then you will realize that a common interface requires a new composer dependency. Otherwise it can not be a common interface.

Please sign in to comment.