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

Fix Carbon not accepting string values #272

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

mokhosh
Copy link
Contributor

@mokhosh mokhosh commented Nov 11, 2024

Description

Carbon methods are strongly typed. So, you can't provide seconds as a string.
This is what you get if you try to override JWT_TTL in your .env:

Carbon\Carbon::rawAddUnit(): Argument #3 ($value) must be of type int|float, string given

This PR fixes the issue by casting the config after reading it from .env as is common in Laravel's config files.

Fixes #271

Checklist:

  • I've added tests for my changes or tests are not applicable
  • I've changed documentations or changes are not required
  • I've added my changes to CHANGELOG.md

Copy link
Contributor

@leoralph leoralph left a comment

Choose a reason for hiding this comment

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

LGTM, I just see a problem where people who already published their config files would still have this problem, maybe in a new major version we could use config()->integer() which would be a breaking change for who uses laravel versions <11.

@mokhosh
Copy link
Contributor Author

mokhosh commented Nov 12, 2024

LGTM, I just see a problem where people who already published their config files would still have this problem, maybe in a new major version we could use config()->integer() which would be a breaking change for who uses laravel versions <11.

I guess that's the case for anyone who publishes any vendor file, whether that's config or views etc.

They would need to sync with latest changes if they wanna keep getting the benefits.

@Messhias
Copy link
Collaborator

I'll wait if anyone else has something to say and approve. If not I'll merge it by the weekend.

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

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

I'm not saying the proposed changed here a wrong, but setTtl(), which receives the JWT_TTL config value already performs a cast

public function setTTL($ttl)
{
$this->ttl = $ttl ? (int) $ttl : $ttl;
return $this;

I rather see inconsistencies with other casts, e.g. we've

  • \PHPOpenSourceSaver\JWTAuth\Blacklist::setRefreshTTL which has a cast

    jwt-auth/src/Blacklist.php

    Lines 224 to 226 in 83653d5

    public function setRefreshTTL($ttl)
    {
    $this->refreshTTL = (int) $ttl;
  • but then \PHPOpenSourceSaver\JWTAuth\Validators\PayloadValidator::setRefreshTTL which has no cast
    public function setRefreshTTL($ttl)
    {
    $this->refreshTTL = $ttl;

Again, I'm not against this change but it feels like the fixes are better provided in other places, which would also make working with the library as API directly safer.

How exactly did you produce this, can you provide a full stacktrace?

@specialtactics
Copy link
Member

I saw this issue on the og package without a fix, and people solved it in this way also.

My problem with this solution is it doesn't really solve the underlying problem - people will still have this problem, if they are lucky stumble on this issue and update their config. Although moving forward new installs will be fine, it won't do anything for the vast base of existing users.

I think the better approach would be to gradually start introducing strong types where relevant - and this is a good example, and then as we read these configs from config / provider, we can ensure they are cast to int or whatever relevant at the earliest opportunity, and then elsewhere in the package we can also strongly type them to further ensure this.

It will take a bit more effort (although not much to be honest), but I think that's a better solution, and it will have immediate effect for all existing users, the next time they do a composer update.

@specialtactics specialtactics self-requested a review December 30, 2024 13:24
@specialtactics specialtactics self-assigned this Dec 30, 2024
@mokhosh
Copy link
Contributor Author

mokhosh commented Dec 31, 2024

Strong types can be introduced in all layers.

@specialtactics
Copy link
Member

Ok, I can have a go at doing this in the new year then, I am not sure whether to merge this or not then, I guess we can for now until it's improved internally?

@Messhias
Copy link
Collaborator

Ok, I can have a go at doing this in the new year then, I am not sure whether to merge this or not then, I guess we can for now until it's improved internally?

If you do it we review it. I believe if we do all what @mokhosh said we can merge it without issues. I do not have any intention to keep old PHP versions because of missing strict types.

@specialtactics
Copy link
Member

Ok, then I've approved this PR as an interim solution at least for new users.

@Messhias Messhias merged commit 7f5bb61 into PHP-Open-Source-Saver:main Jan 2, 2025
12 checks passed
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.

Changing JWT_TTL in .env doesn't work
5 participants