Add generic hints to Cache

[AJenbo]

This provides a way for IDEs and other analyzer tools to reason about the value returned by the cache and there by not cause lose of context when ever it is used. See #38257 for a general discussions about this topic.

This PR is opened against master as generic annotations on the cache component is meant to come after 9.x.

P.s. @nunomaduro could you please share some more information regarding the plans for introducing generic annotations on the framework as it would make it easier to work on :)

[driesvints]

Can't this be sent to 9.x instead?

[AJenbo]

Can't this be sent to 9.x instead?

I was told it coudn't

[driesvints]

Who did? @nunomaduro just said that we can't add these in general:

#40759 (comment)

It's been over a year so maybe we can reconsider but it's up to @taylorotwell to decide.

[AJenbo]

Ok, I'm not privy to what the reasoning is. But I'm happy to have it go in 9.x if possible.

@nunomaduro just said that we can't add these in general:

They where added for other components so it felt specific for the cache component?

P.s. I'm pretty sure it's still 2022, or I forgot to show up for xmas which would be pretty bad :D

[axlon]

These seem wrong to me, just because we pass a default doesn't mean that the value in the cache is of that type as well:

cache()->put('foo', 'bar');
cache()->get('foo', 123);

This will return a string but according to this PR it will return an int.

[taylorotwell]

Thanks for sending this in. I'm going to mark it as draft for now while people discuss it and also while we wait for @nunomaduro to return from vacation.

[AJenbo]

These seem wrong to me, just because we pass a default doesn't mean that the value in the cache is of that type as well:

cache()->put('foo', 'bar');
cache()->get('foo', 123);

This will return a string but according to this PR it will return an int.

I guess this is true, thought this feels like somewhat dubious usage.

The same issue could technically also occur with remember():

cache()->remember('foo', 60, function (): string { return 'bar'});
cache()->remember('foo', 60, function (): int { return 123});

For such cases I think it would be fair to still warn the user that they are likely doing something wrong.

[nunomaduro]

I've made some comments that must be addressed before merging. Also, can you rebase this on v9.x please?

[nunomaduro]

Please use TCacheValue everywhere, and remove of mixed.

[AJenbo]

Oh neat I wasn't aware of that short hand

[nunomaduro]

No need for null here.

[nunomaduro]

This should consideration that the $default value may be a closure that returns TCacheValue. And this comment can applied to other arguments annotations in this pull request.

[AJenbo]

Wasn't aware of this possibility in Laravel, hopefully this will help more discover it as well :)

[nunomaduro]

You will need to add tests for the cases when a closure is given for the $default arguments.

[AJenbo]

I've made some comments that must be addressed before merging. Also, can you rebase this on v9.x please?

Thanks! I'll look in to it 👍

[AJenbo]

@nunomaduro the issues should now have been addressed and the PR has been rebased and targeted towards 9.x

[driesvints]

@AJenbo can you maybe also fix the failing docblock check?

[AJenbo]

@AJenbo can you maybe also fix the failing docblock check?

@driesvints could you give some input on how it should be resolved?

My thinking is that the tool should allow for additional lines, at least in the bloc preceding the generated part. This would also allow for adding descriptions to Facades.

/**
 * This facade adds a convenient way to interact with the application cache
 * 
 * @template TCacheValue
 *
 * ... generated methods
 */

There also is this line in the tool that I'm not sure if maybe is relevant:
https://github.com/laravel/framework/blob/9.x/bin/facades.php#L213

We could also have it pickup any remaining generics and add them automatically like I have done manually in this PR?

[driesvints]

Just copy paste the output of the build.

[AJenbo]

Well I have, and then added the following bit to make it actually valid:

 * @template TCacheValue
 *

@driesvints are you saying that we should go with the non functional generated one for now?

P.s. I think it would also be good to have the tool generate a diff to make the discrepancies more clear. But that is a bit out of scope here.

[driesvints]

@AJenbo just leave it for now then. We're automating this soon: #45275

[timacdonald]

I'll fix up the generics Facade docblocks stuff once we merge this one. Don't let it be a blocker.

[nunomaduro]

Currently working on something else. But I will look at this week at this.

[nunomaduro]

Made a few adjustments, and added as you as co-author: #45467.

[Tklaversma]

@nunomaduro Since this update, doing the following will give the error 'void' method 'rememberForever' result used.

    public function doStuff(): string
    {
        return Cache::rememberForever('key', function (): string {
            return 'example';
        });
    }

In other words, it sees the return type TCacheValue on rememberForever() as a void. Probably, because TCacheValue has no declaration.