Closed vadimonus closed 4 months ago
Hey @vadimonus
Thank you for the report!
Currently resolveFromApplicationContainer catches only BindingResolutionException | ReflectionException, it should catch Throwable, to be protected from all possible errors.
I'm not sure what do you mean. I checked it, and it's fixed some time ago:
https://github.com/psalm/psalm-plugin-laravel/pull/228
And it's fixed on 2.x branch (namely v2.0.1). https://github.com/psalm/psalm-plugin-laravel/commit/e60d2b77522efb04f96a282404ca46039713131a
Or do I misunderstand something?
@alies-dev , thnk you for your reply.
https://github.com/psalm/psalm-plugin-laravel/pull/228 is only the sipliest part of solution, and does not works for 1.x branch. I do no think it's too hard to backport it.
The main problem is that you should not use resolveFromApplicationContainer at all for types resolution. Service provider can have complex logic of resolving type. Cases are:
So type resolution with resolveFromApplicationContainer gives more problems with false resolutions, than not resolving at all. The only true logic that we have about resolution with app() function:
Also, if you receive not class string, you can try resolve it with \Illuminate\Foundation\Application::registerCoreContainerAliases(). But this is already done good enough by barryvdh/laravel-ide-helper when generating meta, so we do not need to do this second time in plugin.
So i think:
Hey @vadimonus Thank you for your detailed reply
Firstly, on 1.x it's also "fixed": https://github.com/psalm/psalm-plugin-laravel/blob/1.x/src/Util/ContainerResolver.php (incl. version 1.6.2 and later: https://github.com/psalm/psalm-plugin-laravel/blob/v1.6.2/src/Util/ContainerResolver.php)
Secondly, Laravel has a lot of magic, and the way Larastan and this plugin work to find more type info - violate static analysis principle and run some code. I do agree, ideally we should have parameters to enable dangerous actions and when more resources (more maintainers) for this plugin -- we can do it. But so far resources are limited, and it's better to focus on the main, it's listed on the README.md. But any contributions to the plugin are always welcome 👍
@alies-dev , I'm trying to figure out how it happened that the problem was already fixed, but I created the task.
Since version 1.5 of plugin, composer.json requires "illuminate/*": "^6.0 || ^8.0", skipping Laravel 7. And i encountered this problem when running plugin on laravel 7, and plugin 1.4.x, because i cannot use higher plugin versions with it.
Do you remember, why 7 version was skipped in composer.json? Can we backport this patch to 1.4 too?
@vadimonus
I was not a contributor to this package at the moment when maintainers dropped L7 support. I see it's done within this commit: https://github.com/psalm/psalm-plugin-laravel/commit/d475892ecc4626377a3c4bc116c098d9195872a1
The commit body is:
chore: drop support for Laravel 7 Laravel 7 has reached its end-of-life.
I created a new branch 1.4.x
and backported two critical fixes. Can you please test this branch over your project and then share your feedback here? If the fix works fine, I'll release it as a new 1.4.10 version.
Yes, but it will take couple of days
@vadimonus thank you, I'll wait, it's not urgent. Thank you!
Hey @vadimonus Do you have any news?
@alies-dev , sorry, didn't have enough free time.
composer require psalm/plugin-laravel:^1.4.10
gived me dev version Downloading psalm/plugin-laravel (1.4.x-dev 018c576)
.
It finished without error, where 1.4.9 previously crashed
@vadimonus thank you! Then, I'm closing the issue. Thank you for reporting!
@alies-dev , 1.4.10 does not have right requirements in composer.json, it skips laravel 7. Will you release 1.4.11 with requires same as v1.4.9?
hey @vadimonus
Done, I just released it as 1.4.11. What I released as 1.4.10 was a wrong branch. But not it should be fine. The changelog is very clear: https://github.com/psalm/psalm-plugin-laravel/compare/v1.4.9...v1.4.11
Describe the bug Psalm can die when Laravel application has binds with parameters.
Impacted Versions Any laravel version, psalm-plugin-laravel 1.x, 2.x
Additional context If application has service provider with bind with params
and call
it will lead to ErrorException
Undefined index: some_param
, that will kill psalm.That's because
\Psalm\LaravelPlugin\Util\ContainerResolver::resolveFromApplicationContainer
do not takes into account, that make method has second param.Fix Currently
resolveFromApplicationContainer
catches onlyBindingResolutionException | ReflectionException
, it should catchThrowable
, to be protected from all possible errors.When Laravel resolves bind with params, it does not cache result, because it can depend on params, so every call to make with params can produce different results. As far it's not possible to simply pass params to container to get real type, resolvePsalmTypeFromApplicationContainerViaArgs should return null, when
count($call_args) !== 1
.