fix crash in <PHP8 with ResourceBundle #8416
Conversation
AndrolGenhald
added
release:fix
|
count used to contain ResourceBundle, the previous PR removed it for newer version. I don't think dropping it altogether is the way to go here. We better keep it even on PHP >8 |
|
@orklah then what do you want me to do here? When I leave "ResourceBundle" in (anywhere), I get a fatal; I must drop ResourceBundle. The only other solution is go back to the "mixed" it had before that PR, which would be worse I guess. |
|
count used to have ResourceBundle: https://github.com/vimeo/psalm/pull/8217/files#diff-5611cd00854405fe61befddd211dacd225d9c479b499ffd7e3c0c7d96eb568edR216 The change in #8217 was to remove ResourceBundle in latest php versions definitions. So reverting that would mean still have ResourceBundle on every version like before, not removing it altogether |
kkmuffme
force-pushed
the
regression-4.25.0-count-resourcebundle
branch
from
5536ce1
to
f87b527
Compare
|
I didn't do that, as I feared that would still keep the fatal (I cannot reproduce the fatal anymore though, since I don't have access to that environment anymore). |
|
FWIW I do think |
|
ouch, I didn't know it was an extension... Let's fix this the way it was and we'll look for a solution after I think @kkmuffme can you check tests 1 and 4? There's callmap consistencies failures in tests |
kkmuffme
force-pushed
the
regression-4.25.0-count-resourcebundle
branch
5 times, most recently
from
75ddfbe
to
8bcf725
Compare
kkmuffme
force-pushed
the
regression-4.25.0-count-resourcebundle
branch
from
8bcf725
to
328561d
Compare
|
@orklah put back what I had originally PRed now and fixed the tests (they were broken bc of a misunderstanding from another PR) Ready to merge |
| @@ -519,7 +519,13 @@ public function testIgnoresAreSortedAndUnique(): void | |||
| /** @var string */ | |||
| $function = is_int($key) ? $value : $key; | |||
|
|
|||
| $this->assertGreaterThan(0, strcmp($function, $previousFunction)); | |||
| $diff = strcmp($function, $previousFunction); | |||
There was a problem hiding this comment.
Is this on purpose? Won't PHPUnit scream if there are outputs in a test?
There was a problem hiding this comment.
Yes. Because the error message with just the assert is useless, as it doesn't tell you which function is duplicate/in wrong order.
Yes, phpunit will give an error IF there is output - but that's the purpose of this - if it will fail the assertGreaterThan anyway, it might as well just tell me which function causes it to fail, so I can easily fix it.
|
Thanks! |
Fix #8346
partially reverts #8217