Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use FQN instead of alias to fix phpstan internal error #807

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Use FQN instead of alias to fix phpstan internal error #807

merged 1 commit into from
Mar 22, 2023

Conversation

SanderMuller
Copy link
Contributor

Fixes an phpstan (Larastan) internal error which we only started getting after #798

image

@parallels999
Copy link

Did you test this fix??

@SanderMuller
Copy link
Contributor Author

SanderMuller commented Mar 22, 2023

Did you test this fix??

./vendor/bin/phpstan analyse (and ./vendor/bin/phpstan analyse --debug) are green after this change, at least in the project where it isn't workings since v13.5.0

@MortenDHansen MortenDHansen merged commit 74cccb4 into owen-it:master Mar 22, 2023
@MortenDHansen
Copy link
Contributor

I don't see any difference, so if we make phpstan happy, then why not 😄

@expondo
Copy link

expondo commented Mar 23, 2023

this is like rollback from last release of https://github.com/owen-it/laravel-auditing/pull/798/files
if i understand it correctly, latest change shall fix phpstan problems, but there are new problems? 😵‍💫 and we want to back to old problems from last release?
Anyway, for me it is still better than new error 🌵

@SanderMuller
Copy link
Contributor Author

this is like rollback from last release of #798 (files) if i understand it correctly, latest change shall fix phpstan problems, but there are new problems? 😵‍💫 and we want to back to old problems from last release? Anyway, for me it is still better than new error 🌵

I don't think it's a rollback. Before #798 it used a specific Model, which is not fully correct since you can override which model is used via the config, since #798 it uses an alias of the interface, and after this PR it uses the interface but without the alias.

@erikn69
Copy link
Contributor

erikn69 commented Mar 23, 2023

@SanderMuller hi, it is a rollback to #734
Please try #809

@SanderMuller
Copy link
Contributor Author

@SanderMuller hi, it is a rollback to #734 Please try #809

Weird, #809 again results in the same error.
image

I have no clue why, though. Will see if I can find something. It's fine by me to merge #809, we can skip the version update until we have found a solution for this.

@SanderMuller
Copy link
Contributor Author

I found the culprint, we added a scope on this audits relation which was the source of erroring out, if I manually add a return type via docblock to that method, then the error is gone.

@aglipanci
Copy link
Contributor

@SanderMuller that change was introduced by me here #798 because I was getting this error from larastan, now that you changed it back I am still getting it

Screenshot 2023-06-20 at 10 39 31

@stefro
Copy link

stefro commented Jun 28, 2023

I can confirm that I also have the same issues as @aglipanci mentioned above since this week.

@digistorm-developer
Copy link

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.

8 participants