[Bug]: Better support for static analysis
DanielGSoftware opened this issue · 11 comments
What happened?
Hello there!
To start off, I want to say that this "bug" isn't that big of a deal. I believe it to be more relevant when using static analysis tools such as PHPStan and Psalm, which is exactly what we are now implementing in our project. So, I guess this bug report is kind of a discussion to see if it's something that is wanted to be "fixed", and if so, I have a proposed solution.
Alright, let me explain.
Assume we want to use a column that extends the normal Column... let's say a ComponentColumn.
I might implement this in a table like so:
ComponentColumn::make('Status', 'status')
->slot(fn ($foo) => //...),All goes well. Let's say we also want to use attributes.
ComponentColumn::make('Status', 'status')
->component('badges.badge')
// Let's set attributes
->attributes()
->slot(fn ($foo) => //...),When running PHPStan on level 1 or higher, it raises the following error.
Call to an undefined method Rappasoft\LaravelLivewireTables\Views\Column::slot().
When visiting the page this table would be present on, there is no error.
So what's going on here? It seems like we are calling slot() on... the Column class? But slot() exists in the ComponentColumnConfiguration trait, which is inherited by our ComponentColumn. The problem here is that the attributes() method, and many others, return self. This means that tools like static analysis see this as returning the class they were originally declared on (Component), and not any that extend this class (ComponentColumn).
So
public function attributes(Closure $callback): self
{
$this->attributesCallback = $callback;
return $this;
}Should return static
public function attributes(Closure $callback): static
{
$this->attributesCallback = $callback;
return $this;
}I think that's all!
Now, I would understand that this issue doesn't have a high priority, but I believe we are not the only ones using PHPStan and fixing this would be pretty neat. Let me know your thoughts, and if wanted, I'm pretty sure I can create a PR for this.
Package Version
3.2.5
PHP Version
8.3.x
Laravel Version
11.9.2
Theme
Tailwind 3.x
Firstly - thank you for asking the question! Always more than happy to take PRs, as is Anthony (rappasoft)
Unless it's a break-fix, we do require PHPStan Level 5 or higher to pass for v3.0+, and when I'm running PHPStan on Level 5, I'm not seeing that error.
We do have a couple of bits for skipping generic errors/known issues (purely so that we can focus on anything NEW that gets introduced)
Example from a new direct pull from the Master branch:
php vendor/bin/phpstan --level=5
Note: Using configuration file master/phpstan.neon.
⚠️ You're using a deprecated config option checkMissingIterableValueType ⚠️️
It's strongly recommended to remove it from your configuration file
and add the missing array typehints.
If you want to continue ignoring missing typehints from arrays,
add missingType.iterableValue error identifier to your ignoreErrors:
parameters:
ignoreErrors:
-
identifier: missingType.iterableValue
⚠️ You're using a deprecated config option checkGenericClassInNonGenericObjectType ⚠️️
It's strongly recommended to remove it from your configuration file
and add the missing generic typehints.
If you want to continue ignoring missing typehints from generics,
add missingType.generics error identifier to your ignoreErrors:
parameters:
ignoreErrors:
-
identifier: missingType.generics
144/144 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
[OK] No errors
Please do reach out on the Discord (feel free to send me a DM), so that I can try to replicate what you're experiencing.
Hey Joe!
Sorry for the little delay, I didn't get any notification of your reply, I'm gonna need to have to figure out why 😛
Anyway, it's correct that the PHPStan build of this package succeeds without any problem.
That's because in source code, there is no instance similar to the one I described above.
To verify that this error still happens if the code does exist, we can use the existing PetsTable and add the following:
public function columns(): array
{
return [
ComponentColumn::make('test')
->component('livewire-tables::components.test-component')
->attributes(function () {})
->slot(fn ($row) => 'test'),
];and then we can run PHPStan on this specific file (by default files in tests dir are not included):
vendor/bin/phpstan analyse tests/Http/Livewire/PetsTable.php
Result:
In case you're somehow still not able to replicate the error, I will contact you through Discord 😄
Ah! Now I see what you're saying, thanks for the example.
It's almost certainly from the pre PHP 8.x days.
I'll update the return types, probably in the next week or so. I'd say feel free to open a PR, but it'll be so many line changes to review that it'll probably take me as long to review a PR as to do it
It is also a good prompt to add some more complex implementations into the test/example cases so that these things get caught.
Very happy if you want to throw any of those into a PR!
There are some dusk tests coming soon as well, mostly to help me retain what is left of my sanity when it comes to testing across bs4/bs5/tw2/tw3. I'm hoping to get those all done before tailwind 4 releases as stable and starts getting adopted.
I've done a quick test with the attributes() method in the HasAttributes trait:
public function attributes(Closure $callback): static
{
$this->attributesCallback = $callback;
return $this;
}
This then seemed to work fine with:
ComponentColumn::make('test')
->slot(fn ($row) => 'test')
->attributes(function () {})
->component('livewire-tables::components.test-component'),
With attributes/slot/component in any order
Analyse then seems to report on the undefined properties on the Models, and that's it.
I did ultimately want it passing at level 9, but quickly realised it was going to take a lot of PHPDoc tags to get there.
Awesome, if you can handle changing the return types, I will make a PR for adding more complex scenario's to trigger more static analysis errors. I think the boundary of which errors I want to trigger should be related to the return types self -> static. Of course the static analysis build should fail, and then once your PR has been merged, the errors should resolve itself after a merge with dev. Is that the way to go? Or maybe it might be better to build on your PR to instantly check if the errors are gone? 🤔
Also, we currently exclude the tests directly from PHPStan, so I'm not sure how to introduce more complex scenario's in this case as I think the place to do that is in the PetsTable. Maybe it's an idea to include the test directory as well?
I did ultimately want it passing at level 9, but quickly realised it was going to take a lot of PHPDoc tags to get there.
Oof, I don't want to call myself an expert on PHPStan, but to my understanding, I believe level 9 is pretty hard to get. Also I think it would be.. let's say... a more pleasurable working experience to go 1 level up per PR instead of going straight to 9. At least that's what we are doing for our project. But to be fair, we also have a lot more code 😄
Yeah, Level 9 is pretty hard to get, and basically requires you to abandon "mixed" totally, I've had a few projects require passing at level 9, which upset some devs who "hadn't yet decided" on what exactly they were passing into different elements.
In terms of including tests directory in PHPStan, typically not the done thing, as those aren't actually present in production instances, and it's hard enough to get people writing tests for their contributions without them having to also comply with that!
Would definitely agree that it's necessary in a corporate environment tho!
In case you're interested in the present test coverage in terms of lines of code covered:
https://app.codecov.io/gh/rappasoft/laravel-livewire-tables/tree/master/
There is of course lots more to do, I've recently done some testing with some custom Dusk approaches thanks to the Livewire core guys sharing some useful code.
I've merged that fix into "development" branch, so it'll go into master sometime this week, and out as a release by next weekend.
There should also be the fixes that get us to Level 6 by then as well
Just to note, I've merged in a quick fix to use larastan/larastan rather than the original. I've also fixed one issue with the phpstan.neon file, cleaned up one test, and added two typehints for properties.
If you use the "development" branch, is your issue now resolved?
Clearly, don't use it in production, but for any non-prod work, "development" can be considered at least reasonably decent, it has the same requirements for tests to pass etc before anything gets added into it.
Do give me a poke on Discord so that I know who you are on there as well!
Awesome, I can confirm it fixed our errors! Also I wanted to thank you for the extra info about PHPStan and some other stuff, it's always helpful to hear other's stories & reasoning. I can very much see why it would indeed be "better" to not include tests for PHPStan 😄.
To continue, now that including tests is not an option, I don't know how we want to test that verify those PHPStan errors, since I was relying on using the PetsTable or adding a whole new table itself 😬. Would you have any alternative idea's?
I've shoot you a message on discord as well!
Now released as v3.2.8, please advise if anything not as expected!