Add Psalm to check for possibly fatal issues

[muglug]

As a followup to #878, this PR adds Psalm to the build process.

It uses the dev-master version of Psalm, because I ran into a bug patched here. The fix downgrades UndefinedMethod issues to PossiblyUndefinedMethod) in this scenario. That code isn't actually problematic at all, but Psalm doesn't know that view() always returns a \Illuminate\View\View when there are args passed to it.

If you run Psalm with vendor/bin/psalm you'll see a lot of issues. To just see the errors, run

vendor/bin/psalm --show-info=false

[muglug]

As part of this I got rid of Laravel-supported aliases, as Psalm doesn't understand class_alias

[djaiss]

While I truly appreciate your contribution, I don't understand why Auth::user() is bad compared to Illuminate\Support\Facades\Auth::user(). If anything it makes things more complicated and getting rid of all the aliases seem a bit useless to me. Can you add more explanation?

[muglug]

Sure! So, basically, class_alias (the mechanism that allows Laravel aliases to work) is bothersome to reason about via static analysis, and it’s pretty uncommon in PHP outside of Laravel-specific code.

class_alias also makes things tricky from an autoloading perspective, as aliased classes aren’t interpreted accurately in try/catch statements at runtime (the Laravel docs warn of that limitation).

If you’re really attached to them, I think they make the most sense in the views, which seem to use a PHPish syntax that Psalm doesn’t analyse (and where you can’t use use statements). But in regular PHP files it’s basically a choice between Psalm (and other similar tools) or Laravel aliases.

[asbiin]

That's not good here

[asbiin]

Did you try using https://packagist.org/packages/barryvdh/laravel-ide-helper instead of removing aliases ? If it's just to please to psalm when it's running, I don't see the benefit of removing laravel aliases.

[asbiin]

We can consider here 2 cases : php code files, and blades files.
I think we could have full namespace in uses for php code files.
But for blades files, aliases simplifies reading ...

[muglug]

I've added back the aliases for views.

Psalm finds five fatal issues introduced by #992:

ERROR: UndefinedVariable - app/Http/Controllers/Api/ApiContactController.php:361:13 - Cannot find referenced variable $contact
            $contact->setRelationshipWith($partner);
ERROR: UndefinedVariable - app/Http/Controllers/Api/ApiContactController.php:385:13 - Cannot find referenced variable $contact
            $contact->unsetRelationshipWith($partner);
ERROR: UndefinedVariable - app/Http/Controllers/Api/ApiContactController.php:450:36 - Cannot find referenced variable $contact
            $kid->isTheOffspringOf($contact);
ERROR: UndefinedVariable - app/Http/Controllers/Api/ApiContactController.php:474:13 - Cannot find referenced variable $contact
            $contact->unsetOffspring($kid);
ERROR: UndefinedVariable - app/Http/Controllers/Api/ApiContactFieldController.php:73:13 - Cannot find referenced variable $contactField
            $contactField->update($request->all());

[asbiin]

@muglug Indead !! I've fixed these issue with #1013

[muglug]

I've rebased and everything's good. There was a test timeout, but I think it's transitory?

[asbiin]

@muglug I tried psalm with barryvdh/laravel-ide-helper package, it's working well: The _ide_helper.php file produce by laravel-ide-helper can be used as stub by psalm, and you don't need to expand facades like DB or App. What do you think ?

[muglug]

@asbiin sorry, must have missed that last comment.

Trouble with barryvdh/laravel-ide-helper is that it needs to be run before running Psalm, and that's not great.

[asbiin]

@muglug I don't see the point. You can easily run php artisan ide-helper:generate before vendor/bin/psalm

[CLAassistant]

All committers have signed the CLA.

[asbiin]

@muglug OK it's working now (I've added CircleCI support).
The only problem I see is we can't use Laravel aliases anymore. Maybe it was on purpose ? Are they bad php implementation ?

[muglug]

@asbiin I really don't care that much, but I also don't want to spend the time undoing my changes to get it to work with ide-helper - if you want to submit that patch, be my guest!

[asbiin]

@muglug I tried it, but it doesn't work with ide_helper so far ... I was just wondering if there is any chance psalm could use Laravel aliases or if they are useless with it ?

[muglug]

I’d have to add, I think, a bunch of extra redirection to support class_alias which would slow down the project. And there’s the additional issue that class_alias is akin to goto - most often a hack, rarely necessary.

[asbiin]

@muglug Great. I would be glad to help adding some hacks to support aliases.
For now I'm merging this great PR. We could try to improve integration with psalm later. Thank you for your help !

[muglug]

You're welcome!

[djaiss]

@muglug indeed, thanks for your hard work and patience 😀

[github-actions]

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.