Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed ignoreTypeStats & useStrictTypes flag value detection for directories in config #7182

Merged

Conversation

SMAtaurRahman
Copy link
Contributor

I've noticed that setting any of ignoreTypeStats="true" or useStrictTypes="true" flag for directories in config file is ignored due to a bug in Psalm\Config\FileFilter

FileFilter::loadFromXMLElement() reads these flags and turns them into boolean values, then pass the config array to FileFilter::loadFromArray(). loadFromArray reads the flag value again, but this time the function treats the values as string instead of boolean. As a result, it cannot properly detect the value.

I've tried to fix the behavior with this PR.

Note: I wasn't able to understand the purpose of useStrictTypes=true. Can you please let me know what difference does it make?

@orklah
Copy link
Collaborator

orklah commented Dec 18, 2021

useStrictTypes is used to emulate the behavior of declare(strict_types=1);. In practice, it makes Psalm a little more strict when it sees scalar coercion (returning a string instead of int for example) or __toString coercion (returning an object with __toString when a string is expected)

@@ -1483,4 +1483,70 @@ public function pluginRegistersScannerAndAnalyzer(int $flags, ?int $expectedExce
self::assertSame(get_class($analyzerMock), $config->getFiletypeAnalyzers()[$extension] ?? null);
self::assertNull($expectedExceptionCode, 'Expected exception code was not thrown');
}

public function testTypeStatsForFileReporting(): void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing on windows. I'm pretty sure this has to do with path separators in getcwd()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've no idea how to approach this issue. I don't have access to windows machine.
testBarebonesConfig() also uses getcwd(). I wonder how it is working.

Should I try using dirname(__DIR__, 2) instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, that's not getcwd(), the issue is with . '/' at the end of paths. You can replace with . DIRECTORY_SEPARATOR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, it worked. Thanks.
I should be careful with dir separator from next time.

@SMAtaurRahman SMAtaurRahman force-pushed the config-typeStats-and-strictTypes-fix branch from d01a308 to e408abf Compare December 18, 2021 13:59
@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Dec 18, 2021
@orklah orklah merged commit eca1cc3 into vimeo:master Dec 18, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 18, 2021

Thanks!

@SMAtaurRahman SMAtaurRahman deleted the config-typeStats-and-strictTypes-fix branch December 18, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants