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

tmpDirが存在するときのNOTICEを抑制する対応を実施 #208

Closed
wants to merge 1 commit into from

Conversation

momospnr
Copy link
Contributor

再現方法

PHPアプリケーションをビルドする際に
Dockerfileなどで事前にtmpDirを作成していると
ヘルスチェックなどの初回アクセスでコンパイラが動いたときに下記のエラーログが発生します。

エラーログ

NOTICE: PHP message: PHP Warning:  mkdir(): File exists in ~/root/vendor/ray/aop/sl-src/Cache.php on line 67

対応内容

is_dirでtmpDirの存在確認を行い、なければmkdirで作成するようにしました。
is_writableで、falseのときにchmodするように修正しました。

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (41fcf7d) 100.00% compared to head (bb2e125) 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##                 2.x      #208   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       168       168           
===========================================
  Files             30        30           
  Lines            470       470           
===========================================
  Hits             470       470           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@koriym
Copy link
Member

koriym commented Jan 13, 2024

Dockerfileなどで事前にtmpDirを作成していると

これは「書き込み権限がないtmpDirを作成していると」という事でしょうか?

@momospnr
Copy link
Contributor Author

コメントありがとうございます!
Dockerfileでは下記のようにフル権限を与えてディレクトリ作成しても、NOTICEが出てしまっている感じです。

RUN chmod -R 777 var/log var/tmp

原因が特定できないので、とりあえずログが出ている箇所で抑制しようと考えた次第です。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

書き込み権限のあるディレクトリが存在すれば is_writable($dir)が満たされるので mkdirに到達しないはずですよね。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

考慮すべき点を列挙すると

  • tmpDirが正しいものかをチェックするのではあれば、コンストラクタで行ったほうが良いのではないか。
  • Compilerのコンストラクタも同様
  • 通常よくあるのは
      * A) is_dirがfalseだったら !@mkdir($tmpDir, 0777, true) を行う。
      * B) あるいは 以下のように例外を発生させる
   if ($tmpDir && !is_dir($tmpDir) && !@mkdir($this->secretsDir, 0777, true) && !is_dir($tmpDir)) {
            throw new \NotWritable($tmpDir);
   }

@koriym
Copy link
Member

koriym commented Jan 13, 2024

以下のようなスクリプトを作成して確認するとtmpを444で作成していると PHP Warning: mkdir(): File existsのwarningが発生するようです。

$tmpDir = __DIR__ . '/tmp';
if (! is_writable($tmpDir)) {
    mkdir($tmpDir);
}

var_dump(is_writable(__DIR__ . '/tmp'));

@koriym
Copy link
Member

koriym commented Jan 13, 2024

doctrine/cacheの実装

https://github.com/doctrine/cache/blob/1.13.x/lib/Doctrine/Common/Cache/FileCache.php#L97

is_writableが通らなければエラーにしています。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

tmpDirが必要なライブラリでは、A)書き込みができるディレクトリがなければ例外 B)書き込みディレクトリがなければ作成、のいずれかがほとんどで書き込みできないディレクトリが用意されている状態へのエラーはあっても、リカバリーまではない事が多いようです。

Ray.Aopのこの部分だけで対処しても、他のライブラリでは問題になるかもしれず不安が残ります。どうでしょうか。

@momospnr
Copy link
Contributor Author

色々なパターンご提示いただきありがとうございます。

確かにアプリケーション側では例外もしくは、Ray.AopのパターンのようにNOTICEを出して気づかせる。。という役割に留めたほうが良さそうですね。

@momospnr
Copy link
Contributor Author

そもそも、NOTICEが出ているクラスでis_writableに渡される$tmpDirが、var/tmpディレクトリじゃない可能性があるってことですよね。。?

その辺りの確認をしていないのでダメですね。。
一旦PR取り下げようかと思います。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

@momospnr File exists が出てるということは何かのファイルまたはディレクトリは存在してて、ということなのでもし違うディレクトリが指定されてあれば出る可能性はあると思います。

しかしBEAR.Sundayの場合ならvar/tmp は通常通り指し示されてるだろうと考えるのは自然ですね。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

再度確認したら、この箇所は var/tmpの下にディレクトリを作ろうとして出ているエラーですね。

https://github.com/ray-di/Ray.Aop/blob/2.x/sl-src/Cache.php#L63

なのでそもそもディレクトリが無いわけですから mkdir($tmpDir) で0777のディレクトリが生成されるべきで、「mkdirの前に同名のディレクトリが書き込み権限のない状態で存在している」ことがおかしいと思います。なんでしょうね。

@koriym
Copy link
Member

koriym commented Jan 13, 2024

tmpDirのバリデーションをより細かく行うPRを作成しました。

#209

@momospnr
Copy link
Contributor Author

ローカル(Mac)でデバッグしていたところ、$this->tmpDirの値が下記になっていました。

"/var/folders/md/zt16vyds29n0n9d6dw_3cmyr0000gq/T/24"

アプリケーションルートのvar/tmpディレクトリにファイルキャッシュが作られる前に
OSのtmpディレクトリに一度ファイルキャッシュが作成されるということなのでしょうか?

https://github.com/ray-di/Ray.Aop/blob/41fcf7d37eee7fb6b0eba4374eb3ab163f7e3222/sl-src/ServiceLocator.php#L29C27-L29C43

#209 ありがとうございます!

@koriym
Copy link
Member

koriym commented Jan 15, 2024

このキャッシュは対象ファイルの日付を見ていてつまりキャッシュのメンテナンスの必要が無い=ファイルを更新すれば値が変わるのであえて(他と違い)システムのtmpディレクトリの場所にしています。

@momospnr
Copy link
Contributor Author

解説ありがとうございます!
なるほど、、キャッシュの種類が違うのですね。
var/tmpに作成されるキャッシュと、/tmpに作成されるキャッシュの違いをそもそも理解出来てないため今度教えてください🙇‍♂️

@koriym koriym closed this Jan 19, 2024
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.

2 participants