owen-it / laravel-auditing

Record the change log from models in Laravel
https://laravel-auditing.com
MIT License
3.05k stars 390 forks source link

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

Closed SanderMuller closed 1 year ago

SanderMuller commented 1 year ago

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

image

parallels999 commented 1 year ago

Did you test this fix??

SanderMuller commented 1 year ago

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 commented 1 year ago

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

expondo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@SanderMuller hi, it is a rollback to https://github.com/owen-it/laravel-auditing/pull/734 Please try https://github.com/owen-it/laravel-auditing/pull/809

SanderMuller commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@SanderMuller that change was introduced by me here https://github.com/owen-it/laravel-auditing/pull/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 commented 1 year ago

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

digistorm-developer commented 1 year ago

I just came across the same issue and put a PR for the fix.