Skip to content
New issue

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

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

Already on GitHub? # to your account

Validate injector cache saved and optimize local cache selection #417

Merged
merged 7 commits into from
Apr 21, 2023

Conversation

koriym
Copy link
Member

@koriym koriym commented Apr 20, 2023

Symfonyはキャッシュが失敗したときにLogに記録して
https://github.com/symfony/cache/blob/v5.4.22/Traits/AbstractAdapterTrait.php#L230

その後trigger_errorでエラー発生させますが、@で抑制されていてエラーハンドラーで検出しないとユーザーが気がつきません。
https://github.com/symfony/cache/blob/v5.4.22/CacheItem.php#L189

このPRはキャッシュの検証を行い、検出可能にします。

それに加え、ローカルキャッシュの判定を修正し、CLIのときにApcuAdapterを使わないようにしました。
27c7d8e


Symfony logs cache failures to
https://github.com/symfony/cache/blob/v5.4.22/Traits/AbstractAdapterTrait.php#L230

It then raises an error with trigger_error, but it is suppressed with @ and the user will not notice it unless it is detected by the error handler.
https://github.com/symfony/cache/blob/v5.4.22/CacheItem.php#L189

This PR validates the cache and makes it detectable.

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2e6d717) 99.87% compared to head (729cc94) 99.87%.

❗ Current head 729cc94 differs from pull request most recent head 27900ae. Consider uploading reports for the commit 27900ae to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##                1.x     #417   +/-   ##
=========================================
  Coverage     99.87%   99.87%           
  Complexity      243      243           
=========================================
  Files            54       54           
  Lines           814      816    +2     
=========================================
+ Hits            813      815    +2     
  Misses            1        1           
Impacted Files Coverage Δ
src/Injector.php 100.00% <100.00%> (ø)
src/Injector/PackageInjector.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@koriym koriym requested review from jingu and NaokiTsuchiya April 20, 2023 05:15
@koriym koriym changed the title Validate injector cache saved Validate injector cache saved and optimize local cache selection Apr 20, 2023
@koriym koriym marked this pull request as ready for review April 20, 2023 06:32
@koriym
Copy link
Member Author

koriym commented Apr 20, 2023

ray-di/Ray.PsrCacheModule#17Ray.PsrCacheModule のリリースを行った後にcomposer.jsonを修正します。

src/Injector.php Outdated
@@ -26,7 +25,7 @@ public static function getInstance(string $appName, string $context, string $app
{
$meta = new Meta($appName, $context, $appDir);
$cacheNamespace = str_replace('/', '_', $appDir) . $context;
$cache ??= ApcuAdapter::isSupported() ? new ApcuAdapter($cacheNamespace) : new FilesystemAdapter('', 0, $meta->tmpDir . '/injector');
$cache = (new LocalCacheProvider($meta->tmpDir . '/injector', $cacheNamespace))->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

受け取った $cache の値に関わらず LocalCacheProvider の値に置き換えてしまっていますかね?

Copy link
Member Author

Choose a reason for hiding this comment

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

確かに!!

Copy link
Member Author

Choose a reason for hiding this comment

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

修正しました!

@koriym koriym requested a review from NaokiTsuchiya April 20, 2023 08:10
@koriym koriym merged commit 2bb73ef into 1.x Apr 21, 2023
@koriym koriym deleted the check-cache branch April 21, 2023 08:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants