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

Consistent error logging for cache #8415

Merged
merged 5 commits into from Sep 11, 2022

Conversation

kkmuffme
Copy link
Contributor

Generally, psalm throws errors in cache.

However there were 2 places, where error_log was used/leftover from early versions. Changed now for consistency.

@kkmuffme kkmuffme force-pushed the consistent-error-logging-for-cache branch 3 times, most recently from 90a36d3 to fad498a Compare August 16, 2022 14:32
@kkmuffme
Copy link
Contributor Author

@orklah there is a bug in psalm unrelated to this PR? Could you please quickly check

Test fails: https://github.com/vimeo/psalm/runs/7860346355?check_suite_focus=true (which will cause other tests to fail too), due to $config being null (how is this even possible when construct is called with Config, as it should already give an error then??)

When I add a check for that, it says config cannot be null:
https://github.com/vimeo/psalm/runs/7860822542?check_suite_focus=true

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Aug 16, 2022

Psalm\Tests\Internal\Provider\ParserInstanceCacheProvider extends ParserCacheProvider and overrides its constructor to do nothing, so the parent constructor is never called.

Fixing #7877 would be nice.

@kkmuffme
Copy link
Contributor Author

Ok so I guess I will put back the call to static config as it was there before. (since properly fixing this would mean rewriting tons of code that is unrelated to this PR)

@kkmuffme kkmuffme force-pushed the consistent-error-logging-for-cache branch 2 times, most recently from e8e43d2 to 8c20f01 Compare August 24, 2022 12:56
@kkmuffme kkmuffme force-pushed the consistent-error-logging-for-cache branch 7 times, most recently from f483a8f to c0d2ca6 Compare September 9, 2022 00:45
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 9, 2022

Sorry for the gazillion commits/reworks, but there's some unexplainable behavior with a hash in some cases (left that as is) and some other unpleasantries by bad cache/test set up in psalm.

This branch has slightly meandered from what it started out, as I didn't want to create tons of duplicate code for this PR, so I had to remove existing duplicate code and streamline it.
As part of that, I also found some inconsistencies in cache which I fixed too.

This is finally ready to be merged now.

src/Psalm/Config.php Outdated Show resolved Hide resolved
* use exceptions instead of error_log for ParserCacheProvider like all other cache providers do
* remove duplicate code in ParserCacheProvider
* use same hash as other cache providers
* update Config.php cache directory creation to use same code as ParserCacheProvider
Revert "update leftover md5 in provider to commonly used hash"

This reverts commit 66337ec.

partially put back

Update StatementsProvider.php
* fix rare race condition on file cache unlink
* remove unnecessary reset()
* improve code readability using variable
@kkmuffme kkmuffme force-pushed the consistent-error-logging-for-cache branch from c0d2ca6 to 62df25a Compare September 11, 2022 08:28
@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Sep 11, 2022
@orklah orklah merged commit 3748499 into vimeo:4.x Sep 11, 2022
@orklah
Copy link
Collaborator

orklah commented Sep 11, 2022

Thanks

@kkmuffme kkmuffme deleted the consistent-error-logging-for-cache branch September 15, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants