Skip to content

Conversation

vrana
Copy link
Contributor

@vrana vrana commented Sep 10, 2025

@ondrejmirtes
Copy link
Member

  1. I'd say connect method has to change instead, that will take care of invalidating remembered isConnected when called.
  2. You edited the correct file but you also need to run bin/ script to regenerate the metadats file that doesn't have "original" in its name.

If you're interested, you can check out this part of my talk about pure/impure and so on here: https://youtu.be/AFjr3RlDOZQ?si=dp_XJWfenGJBRQhp (from about 28:50 timestamp)

@vrana
Copy link
Contributor Author

vrana commented Sep 11, 2025

Makes sense. Please suggest how can I test this.

@staabm
Copy link
Contributor

staabm commented Sep 11, 2025

you can add a new testXX method into tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php
and put your code from https://phpstan.org/r/1695567f-4869-428f-8cb6-fdd8cb147be1 into the tests data/ directory.

then assert no errors

similar to

public function testBug13384bOff(): void
{
$this->analyse([__DIR__ . '/data/bug-13384b.php'], []);
}

@vrana vrana force-pushed the redis branch 2 times, most recently from c7c8c89 to 21f4d32 Compare September 11, 2025 10:49
@vrana
Copy link
Contributor Author

vrana commented Sep 11, 2025

I've added the test but I couldn't reproduce the original behavior before this change.

@staabm
Copy link
Contributor

staabm commented Sep 11, 2025

Needs $this->reportTooWideBool = true; in the rule test class

@vrana
Copy link
Contributor Author

vrana commented Sep 11, 2025

Thanks, hallelujah!

@vrana vrana changed the title Treat Redis::isConnected as non-deterministic Treat Redis::connect as non-deterministic Sep 11, 2025
@staabm
Copy link
Contributor

staabm commented Sep 11, 2025

don't get confused by the CI results.. we currently investigate spurious CI errors

@ondrejmirtes ondrejmirtes merged commit 1de1dad into phpstan:2.1.x Sep 12, 2025
448 of 457 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants