Support PSR-20 (clock)

[tscni]

Q	A
Branch?	3.2.x
Bug fix?	no
New feature?	yes
Deprecations?	yes
License	MIT

Hi there

To simplify testing of systems that use the time based checkers (exp, iat, nbf), I'd like to introduce support for PSR-20 (psr/clock). This would remove the necessity for cumbersome time()-mocking.

Depending on your preferences, I could adjust the implementation.

• I chose to add symfony/clock as the psr/clock implementation, but we could just as well use another one, implement an internal one (it's trivial after all), or none (and default to time()).
  The benefit of not adding an implementation dependency is that users wouldn't be forced to add one they might otherwise not use.
• There are some more "unrelated" time() usages in EncrypterTest and JWSTest that could also use a clock implementation for consistency, though it wouldn't have any value beyond that.

[Spomky]

Hi @tscni,

Many thanks for this PR. I agree with the need to decouple from the platform and the use of the PSR-20 instead of direct calls to time() is a great opportunity for that.
The problem is that this PR now adds a concrete dependency we don't eed at all.

1. Remove symfony/clock
2. Create a common internal class that implements the PSR-20 interface (example below)
3. Let $clock parameters be null. In the constructors, if $clock is null, trigger a deprecation notice and set the Clock defined in 2. (example here).
4. For tests, use the same type of internal class as 2., but with a configurable output

//Example not tested
// Note that I do not care of the timezone as the library only needs the timestamp

use Psr\Clock\ClockInterface;

/**
 * @internal
 */
final class InternalClock implements ClockInterface
{
    public function now(): DateTimeImmutable
    {
        return DateTimeImmutable::createFromFormat('U.u', (string) microtime(true));
    }

}

[tscni]

I've adjusted the implementation accordingly.

It's not quite clear to me which locations you'd prefer for the clock, so I just put it in the Component\Checker namespace for now. (and the mock clock in the matching Stub namespace)
I'd have put it into Component\Core\Util instead, but based on the deprecation part I assume you don't want to keep that around, so this choice might make some sense.

Speaking towards the deprecation though.
I feel making the clock required would make the checkers a bit more difficult to use for users, especially in the Symfony bundle.
Personally I'd have just added the internal implementation as a default and kept it around.

[Spomky]

I've adjusted the implementation accordingly.

It's not quite clear to me which locations you'd prefer for the clock, so I just put it in the Component\Checker namespace for now. (and the mock clock in the matching Stub namespace)
I'd have put it into Component\Core\Util instead, but based on the deprecation part I assume you don't want to keep that around, so this choice might make some sense.

As this is only used in the checker sub-package, it seems to be the best place.

Speaking towards the deprecation though.
I feel making the clock required would make the checkers a bit more difficult to use for users, especially in the Symfony bundle.
Personally I'd have just added the internal implementation as a default and kept it around.

I am not sure this is really complicated for the Symfony bundle. What about turning the InternalClock into a service and add a configuration option with it as default?

[Spomky]

In any case, this PR looks good to me. There is no need for chasing all green lights in this PR.

[tscni]

I am not sure this is really complicated for the Symfony bundle. What about turning the InternalClock into a service and add a configuration option with it as default?

That'd be an option, but then you'd still have to maintain InternalClock - and then you could just as well keep it around as the default value in the checkers.
Unless you mainly want users to be explicit about the clock when using the checkers directly? I don't really see the benefit there though.

[Spomky]

That'd be an option, but then you'd still have to maintain InternalClock

Yes that's the idea for this branch and to avoid BC breaks. That's why it is marked as internal.
But for the next major release, because a will become clock is a required argument, this option could also be required.

[Spomky]

Hi @tscni,

I have just pushed a commit where a new configuration option is present for the Checker configuration section.
Is is now possible to set the clock. The internal one you created is marked as deprecated to force developers to use the one of their choice.

[stof]

having this argument as the last one means that you cannot actually remove the nullable type in 4.0 as you cannot make it mandatory without making all other arguments mandatory as well.

[Spomky]

Hi @stof,

You are right, this will cause issues in the futur.
I created #467 and by setting constructors as private this should resolve that.
By the way, I am not sure the @deprecated statement on the __construct method makes sense.