search

filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
15.01k stars 2.43k forks source link

Inconsistent Type Hinting in StatsOverviewWidget/Stat.php #12789

Closed juliangums closed 2 weeks ago

juliangums commented 2 weeks ago

Package

filament/filament

Package Version

latest

Laravel Version

latest

Livewire Version

No response

PHP Version

latest

Problem description

The type hinting seems incorrect in this file: https://github.com/filamentphp/filament/blob/3.x/packages/widgets/src/StatsOverviewWidget/Stat.php#L156

It differs from the hinting here: https://github.com/filamentphp/filament/blob/3.x/packages/widgets/src/StatsOverviewWidget/Stat.php#L16

I believe it should use array<scalar|null, scalar|null> | null to cover all possible options. This is particularly frustrating because PHPStan throws an error if floats are used.

Expected behavior

The type hinting should be consistent and support all possible values without causing errors. Specifically, it should use array<scalar|null, scalar|null> | null to allow for a flexible range of inputs, including floats. This ensures compatibility with PHPStan and prevents type errors.

Steps to reproduce

Steps to Reproduce

  1. Compare the type hinting at these two lines:

  2. Notice the inconsistency between the type hints.

  3. Use the following type hint to cover all possible options and avoid PHPStan errors with floats:

    
array<scalar|null, scalar|null> | null

Reproduction repository

https://github.com/juliangums/filament

Relevant log output

No response

github-actions[bot] commented 2 weeks ago

Hey @juliangums! We're sorry to hear that you've hit this issue. 💛

However, it doesn't look like you've provided much information on how to replicate the issue. Please edit your original post with clear steps we need to take.

juliangums commented 2 weeks ago

@danharrin could you have a look at this? I'm happy to do a PR if you're happy. The bot keeps closing the issue.

danharrin commented 2 weeks ago

Since its chart data that should be numeric, I think we can use array<float> | null

juliangums commented 2 weeks ago

@danharrin It also works with true or false or null which I think could be useful for some people. It technically also works with strings like "1", "2.3".

danharrin commented 2 weeks ago

I don't know why you would pass true or false as values on a graph? Or strings? I think it's better to be strict?

juliangums commented 2 weeks ago

Here's the change to float so far. Let me know if you want me to allow bool/string/null too. https://github.com/filamentphp/filament/pull/12803

juliangums commented 2 weeks ago

@danharrin I agree strings are weird, but maybe people have a certain check they perform every day and they want to see how often it was true or false over the last 30 days or so. If a certain goal or performance indicator was met for a day for example.

danharrin commented 2 weeks ago

You can use 1 and 0 as integers in that example

juliangums commented 2 weeks ago

Yeah you can always find workarounds of course. Just is one more thing to do if you have to cast to int but not a big issue either. I'm happy either way. Float was more of an issue.

juliangums commented 2 weeks ago

@danharrin small blunder on my end. removed the param name by accident. opened a new PR to correct https://github.com/filamentphp/filament/pull/12804