rappasoft / laravel-livewire-tables

A dynamic table component for Laravel Livewire
https://rappasoft.com/docs/laravel-livewire-tables/v2/introduction
MIT License
1.78k stars 336 forks source link

[Bug]: NumberFilter doesn't validate a string value, and as a result never validates any number #1999

Open khwadj opened 2 weeks ago

khwadj commented 2 weeks ago

What happened?

Using a NumberFilter prevents the value in the input from being treated since NumberFilter::validate always returns false.

public function validate(float|int|string|array $value): float|int|string|false
    {
        if (is_array($value)) {
            return false;
        } elseif (is_float($value)) {
            return (float) $value;
        } elseif (is_int($value)) {
            return (int) $value;
        }

        return false;
    }

This is a regression from v3.4.14 that still had the old definition as

public function validate(mixed $value): float|int|bool
    {
        return is_numeric($value) ? $value : false;
    }

I do understand and share the will to type everything.

However, in this case, a valid $value comes from a input type="number" (or input type="text", or nput without a type) and will always be a string.

workaround: use a TextFilter instead, which is not great.

How to reproduce the bug

No response

Package Version

3.4.22

PHP Version

None

Laravel Version

10.48

Alpine Version

No response

Theme

None

Notes

Proposed solution:

public function validate(float|int|string|array $value): float|int|string|false
    {
        if (is_float($value)) {
            return (float) $value;
        } elseif (is_int($value)) {
            return (int) $value;
        }

        return is_numeric($value) ? (float) $value : false;
    }

I'm removing the test against the array type, since is_numeric(array) is false. I'm leaving the first two tests against the float and int because you could add anything before reaching this point where you would cast the value.

Let me know if this is an acceptable solution

Error Message

No response

lrljoe commented 1 week ago

Thanks for your other FR, please issue a PR against the "development" branch, and I'll get it merged in this weekend.