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

Use symfony/clock component when available instead of non-testable time() #1204

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

Dean151
Copy link

@Dean151 Dean151 commented Feb 10, 2024

Motivation:
Tests are already hard enough to build so that they are testing, covering, isolated and in an expected way.
Therefore, managing external dependencies is an obligation. And time is an external dependency.

With the introduction of symfony/clock in 6.3, Symfony proposes a solution for managing time in an application, with a shared instance that is overridable during the tests.

Solution:
This PR looked over the time() usage, and replaced them with SymfonyClock::now()->getTimeStamp() when available.

Workaround:
It's possible to use Symfony/Clock in your own application without this PR by using a Events::JWT_CREATED event listener to update nbf, iat and exp accordingly.

Retro-compatibility:
Since 2.x still support down to symfony 4, this dependency is optional, therefore it was only added in suggest key.
If there is a conflict with specific version of clock, we'll be able to restrict it by using the conflict key.

3.x:
This PR does not target 3.x because it's not released yet. But in a 3.x version, symfony/clock could become a requirement and allow code simplification overall.

Tests:
Current test suite still uses time(), but they could be improved to take advantage of this new testability.

@Dean151
Copy link
Author

Dean151 commented Feb 10, 2024

Sadly, this might be incompatible with:

Therefore, I do think the dependency should be flexible in the source, and we could use the one we prefer in the tests so that the end user can rely on whatever psr/clock implementation he prefers

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

Successfully merging this pull request may close these issues.

1 participant