don't disable checks when includes can't be resolved#2818
don't disable checks when includes can't be resolved#2818muglug merged 2 commits intovimeo:masterfrom
Conversation
|
@muglug is there something else I can do here? I think the test illustrates the issue nicely Lines 784 to 791 in 14a843a and I think it's bad enough to warrant doing something about this. Right now any unresolvable include can hide fatal errors (for example the call to |
|
Could you make this configurable? I'm happy for this to be the default from now onwards, but there are probably some projects that rely on this behaviour. I can imagine someone doing something strange like require("foo_$bar.php");
function_defined_in_that_file();and expecting Psalm not to worry. After a lot of refactoring over the years, at Vimeo we only depend on this behaviour on a single line, so I'm happy for this change in general. |
|
Yep. I'll add a setting. I'll update the PR. In my case, I thought I finally got our 14 years old code-base in shape only to stumble over an obvious type that psalm didn't detect, so I dug into this. The end result is 520 more issues that were completely hidden from me. I've since fixed them too. |
Awesome! |
Any unresolvable include (even suppressed ones) would lead to some subsequent tests being disabled as a side-effect. this fixes vimeo#2817
as suggested in the PR it's best to make the setting configurable. In order not to break existing installations, we default to keeping the old behaviour, but in a later version of psalm, we might change the default.
14a843a to
03bda8c
Compare
|
I have rebased on top of current master and added a second commit that makes the behavior configurable - including the documentation to explain how it works and including an additional test (using the same fixture but the other setting) to make sure the setting is applied correctly. In an abundance of caution I'm defaulting to the old behavior in order to not blow up all over people's faces when they upgrade psalm. Feel free to change the default if the next release is going to be 4.0 - and maybe add a note to #1732 for doing so. I still strongly believe that the default should be to not silently disable essential functionality that could prevent fatal errors in production. |
|
Thanks! |
Any unresolvable include (even suppressed ones) would lead to some subsequent tests being disabled as a side-effect.
this fixes #2817
As noted on that bug report, this might cause big swaths of already-present-but-to-date-hidden issues to be uncovered for people updating to a release with this patch applied.
I'm unsure as to whether we need a switch to toggle this updated behavior or whether we need some changelog warning or whether just fixing this is ok.