-
Notifications
You must be signed in to change notification settings - Fork 27
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
Remove doctrine annotation reader #203
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## 2.x #203 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 162 168 +6
===========================================
Files 30 30
Lines 457 470 +13
===========================================
+ Hits 457 470 +13
☔ View full report in Codecov by Sentry. |
e87032e
to
bf9775d
Compare
5275585
to
890ce9e
Compare
* Created to avoid "version dependencies" on the original doctrine/annotation v1/v2, psr/log and other cache libraries. * This leader is valid in both development and production environments.
de2eecd
to
20db177
Compare
20db177
to
a805754
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
シナリオに記載の内容は、ドキュメントとして含めてしまっても良さそうな内容と思いました!
/** @var array<string, array<object>> */ | ||
private $loadedAnnotations = []; | ||
|
||
/** @var int[] */ | ||
private $loadedFilemtimes = []; | ||
|
||
public function __construct(Reader $reader, CacheItemPoolInterface $cache, bool $debug = false) | ||
public function __construct(Reader $reader, Cache $cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache も interface があれば、例えば APCu に変更したいときに APCuCache だけを実装すればOKになるかなと思いましたがどうですかね。。?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
アノテーションリーダーからアトリビュートの過渡期で、より高速を望むなら、
A) rectorなどでアトリビュートにするか または B)オリジナルのキャッシュリーダー PsrCachedReader を使うか
という選択肢があり、ここでB)を選択しなかったのは「他のライブラリに依存するより最小で自己完結的にした方が良い」と考えたからです。ここで新しいインターフェイスを導入するのは"自己完結"に背くような気もして入れてない感じです。いつもは可能な限り汎用品を使って租結合にしてますが、ここは違うところでは?と考えました。どうでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"シナリオ"をパフォーマンスに含めました。
ReflectionClass
andReflectionMethod
In order to minimize version dependencies on other libraries such as doctirine v1/v2, psr/cache v1/v2/v3, and other symfony caches, and to enable efficient annotation usage that is useful both in development and in production, we decided that it would be better to keep our own (extremely) minimal cache reader for reading annotations Aop decided that it would be better to maintain its own extremely minimal cache reader for reading annotations, instead of using a generic cache library as before.
Aop/Ray.Di applications can use the following methods for fast annotation/attribute usage instead of using the doctrine/annotation reader as before.
doctirine v1/v2, psr/cache v1/v2/v3, 他symfony cacheなどの他のライブラリのバージョンの依存を最小化し、開発時でもプロダクションでも有用な効率的なアノテーション利用を可能にするために、アノテーションの読み込みにこれまでのように汎用のキャッシュライブラリを利用するのではなく、独自の極端に最小化したキャッシュリーダーを保持するのが良いと判断しました。
。Ray.Aop/Ray.Diを使ったアプリケーションはこれまでの様にdoctrine/annotationリーダーを使うのではなく以下のメソッドで高速にアノテーション/アトリビュート双方が利用できます。
背景
php8でdoctrine/annotationの代替となるPHP8ネイティブのアトリビュートの機能がリリースされ、これからannotationの重要度は低下していく前提があります。また他にも他のライブラリの依存対応の問題もあります。
https://packagist.org/packages/psr/cache
シナリオ
annotationの理由が必須な場合で開発時
ファイルの時間をベースにしたキャッシュでキャッシュ削除などのメンテナンスが不要です。
annotationの理由が必須な場合でプロダクション時
何もしなくてもPHPファイルキャッシュが機能しますが、さらに最適化が必要な場合はブートストラップで以下のコードのように最適化したアノテーションリーダーをセットします。
annotationの理由が不要な場合
rectorなどで既存のアノテーションを全てアトリビュートに変更した場合は、以下のコードのように最適化したアノテーションリーダーをセットします。
いずれの場合も広報互換性は保たれ、アノテーションを取り除いた後でもパフォーマンスインパクトは最小に抑えられると考えられます。