Skip to content

Conversation

@peterfox
Copy link
Collaborator

No description provided.

@TomasVotruba
Copy link
Collaborator

TomasVotruba commented Dec 11, 2022

Test are passing, so everything seems ok :). Could you add the failing test fixture you linked on Twitter?

Also, fix PHPStan errors first and run PHPStan locally. It often guides you to the fix itself :)

@peterfox
Copy link
Collaborator Author

@TomasVotruba I've created an example test, but the problem doesn't seem to be an issue other than when it's tested on a Laravel app. I might have just set up a demo Laravel app quickly to demonstrate the problem there.

I've also updated the code to fix the PHPStan issues.

Appreciate you looking at this.

@TomasVotruba
Copy link
Collaborator

Great job 👍

The test fixture must fail here, not outside, so we have the bug covered and isolated. Try to figure out the test fixture to fail here.

@peterfox
Copy link
Collaborator Author

peterfox commented Dec 11, 2022

@TomasVotruba I've tried to understand the problem but with no luck. Ultimately, I created a Laravel project showing that the rule fails.

@TomasVotruba
Copy link
Collaborator

I see you've put in the lot of effort, so I'm checking the repository to help you out 👍

For some reason, the $response is seen as mixed.
I assume it will be somehow related to Laravel magic autoloading. I've crashed on this hard before and didn't get pass it.

I'd recommend same setup as Larastan has. There will be probably some magic file that you include and PHPStan will see the types.

@peterfox
Copy link
Collaborator Author

Thank you @TomasVotruba. I really appreciate the help. It's odd because, as far as I can tell, it should be picking up the return tag to know that it's a TestResponse class, but you're right, it's detecting it as mixed in the real-world example. It seems to do this with Larastan installed as well. It might be something Larastan itself doesn't pick up. I'm sure I can find some way to resolve this, if not Laravel 10 will add return types I believe, so it might just be something for the future if I can't right now.

@peterfox
Copy link
Collaborator Author

@TomasVotruba, just a heads up, I managed to fix the problem with a tweak to Larastan, but debugging PHPStan, it seems there might just be a bug with Rector?

To summarise. When running PHPStan, it debugs the correct type TestResponse, but when running with Rector, it finds it as a mixed type.

Was curious to know if you had any thoughts on how that might of happened and how you might debug that within Rector.

@TomasVotruba
Copy link
Collaborator

That's probably due to the type extensions, possibly loaded magically via phpstan/extension-installer.
They must be included in phpstan-for-rector.neon too.

@peterfox
Copy link
Collaborator Author

It isn't installed in the demo project.

Repository owner deleted a comment from what-the-diff bot Jan 5, 2023
@driftingly
Copy link
Owner

Thanks for your work on this @peterfox.
I took a crack at it and wasn't able to figure out the issue. PHPStan knows the type with/without Larastan. I don't understand why Rector would have an issue there.

@driftingly driftingly marked this pull request as ready for review April 26, 2023 20:41
@driftingly
Copy link
Owner

I tested this again in the demo app you provided @peterfox, it works with and without your larastan fork. I'm going to merge this in now. Thanks 🙌

@driftingly driftingly merged commit 3c78b0a into driftingly:main Apr 26, 2023
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.

3 participants