Avoiding double spaces (v2)

[ThomasLandauer]

Second attempt of #7835

@AndrolGenhald Is there a test I can use as template? Can't find anything for InstancePropertyFetchAnalyzer.php...

[AndrolGenhald]

There doesn't seem to be a great place for it, I would probably just stick it in PropertyTypeTest.php since there are other tests for that issue in there, but if you see a better place for it that's fine too (test organization isn't the best, but it's also not terrible and it doesn't affect much so it hasn't been a priority).

Most tests just check the issue type, but there are some like here that also check the location and message ('error_message' is matched against the message that's displayed when running Psalm). In this case since the message itself is being tested I would either just include the relevant part of the message ('null variable of type') or do the entire message like the linked test, all that really matters is that it correctly tests the fix to make sure it doesn't get broken in the future.

I almost wouldn't bother asking for a test for a change like this, but since there's no linked issue and it's not immediately obvious to me in what case $stmt_var_id is blank, having a test here also serves as a sort of documentation so future contributors can look at the history and see what the fix is actually for.

There's also still a CS failure, that phpcs isn't able to auto-fix.

Sorry to make this so difficult!

[ThomasLandauer]

In my case, $stmt_var_id was empty, when the code looked something like this:

$foo->getBar()->whatever

So before taking a look at the tests: If the purpose is just to inform contributors, what about adding a comment in the code instead?
Or I could try to create a reproducer and file an issue?

[AndrolGenhald]

So before taking a look at the tests: If the purpose is just to inform contributors, what about adding a comment in the code instead?
Or I could try to create a reproducer and file an issue?

I'll defer to @orklah on that since he's the maintainer, but we do tend to be less strict about testing issue messages, so filing an issue with a reproducer and linking it to the PR is probably fine in this case if it's less trouble (I guess I just think of adding a test as easier, but maybe that's because I'm more familiar with it).

[orklah]

I'm fine without tests on that one. Note: double spaces happen when Psalm can't display the variable identifier. It happens for some convoluted expressions (like $foo->getBar()->whatever in this case). However, it's not just about display, it has an effect on assertions and probably other things so it's not easy to fix.

Can you just fix the CS issue please? :)

[ThomasLandauer]

There you go :-)

[orklah]

Thanks!