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

サブディレクトリ配置時に、商品画像が表示されないバグ対応 #5815

Closed
wants to merge 6 commits into from

Conversation

kurozumi
Copy link
Contributor

概要(Overview・Refs Issue)

以下のIssueの対応をしました。

#5813

方針(Policy)

実装に関する補足(Appendix)

テスト(Test)

相談(Discussion)

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

@kurozumi
Copy link
Contributor Author

ローカルで開発する場合、

Symfony CLI の symfony serve を利用すれば画像が表示されますが、

ビルトインウェブサーバーの php -S localhost:8000 だと画像が表示されませんでした。

Docker Compose も確認しましたがこちらは表示されました。

@chihiro-adachi
Copy link
Contributor

@kurozumi
ご対応ありがとうございます。

bin/console cache:warmup --no-optional-warmersで、サーバ内でキャッシュ生成した場合、data_rootが/で設定され、画像が表示されないようです。

@kurozumi
Copy link
Contributor Author

@chihiro-adachi

ちょっとダサい実装なのですが、DOCUMENT_ROOTの取得をやめて
プロジェクトルートから第三階層上まで data_root をチェックするようにしました。

@codecov-commenter
Copy link

Codecov Report

Merging #5815 (3d589e9) into 4.2 (66a7ac9) will decrease coverage by 0.22%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                4.2    #5815      +/-   ##
============================================
- Coverage     78.97%   78.74%   -0.23%     
  Complexity     6287     6287              
============================================
  Files           470      470              
  Lines         21106    21096      -10     
============================================
- Hits          16668    16612      -56     
- Misses         4438     4484      +46     
Flag Coverage Δ
E2E 64.78% <ø> (-0.18%) ⬇️
Unit 77.76% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/Validator/EmailValidator/NoRFCEmailValidator.php 29.41% <0.00%> (-70.59%) ⬇️
src/Eccube/Util/FilesystemUtil.php 75.00% <0.00%> (-25.00%) ⬇️
...Eccube/Repository/Master/OrderStatusRepository.php 72.00% <0.00%> (-6.27%) ⬇️
src/Eccube/EventListener/ExceptionListener.php 77.55% <0.00%> (-6.13%) ⬇️
.../Eccube/Doctrine/Common/CsvDataFixtures/Loader.php 75.00% <0.00%> (-5.00%) ⬇️
src/Eccube/Form/Type/Admin/TemplateType.php 95.23% <0.00%> (-4.77%) ⬇️
src/Eccube/Repository/BlockPositionRepository.php 90.90% <0.00%> (-4.55%) ⬇️
src/Eccube/Service/MailService.php 77.90% <0.00%> (-4.37%) ⬇️
...chaseFlow/Processor/SaleLimitMultipleValidator.php 91.30% <0.00%> (-4.35%) ⬇️
src/Eccube/Form/Type/Admin/MasterdataType.php 95.83% <0.00%> (-4.17%) ⬇️
... and 65 more

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

@chihiro-adachi
Copy link
Contributor

@kurozumi
ありがとうございます。
不必要な上位階層まで探索させるのは、ディレクトリトラバーサルとか発生させそうで若干怖いですね。。

FileSystemLocator あたりでパス探索やってそうなのでこちらを拡張するか、インストール時にdocument rootを.envに保持してそれを参照するかみたいな実装が望ましいかもです。

いずれにしても少しテストが足りないように思いますので、いったんrevertさせてください。
4.2.0以降で取り込み進められればと思います。

力およばずすみません。


class FileSystemEccubeLocator extends FileSystemLocator
{
protected function generateAbsolutePath(string $root, string $path): ?string
Copy link
Contributor Author

@kurozumi kurozumi Sep 28, 2022

Choose a reason for hiding this comment

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

以下のようにサブディレクトリで運用した場合、

/var/www/html/eccube

$rootには /var/www/html/eccube が格納され、
$pathには eccube/html/upload/save_image/sand-1.png が格納されます。

結果、/var/www/html/eccube/eccube/html/upload/save_image/sand-1.png となり画像が見つかりません。

なので画像が見つかるまで $root の上位階層を探索するようしました。

@chihiro-adachi
Copy link
Contributor

@kurozumi
ありがとうございます。
やはり上位階層まで探索させるのはあまりやりたくないです。

こんな感じで$pathからbasePathをカットできないでしょうか。
※locatorじゃなくてresolverの仕事な気もしますが。。

protected function generateAbsolutePath(string $root, string $path): ?string
{
      $basePath = $this->requestStack->getMainRequest()->getBasePath(); // ※requestStackはinjectionさせる必要あります
      $basePath = ltrim($basePath, '/');
      $path = ltrim($path, $basePath);

@xuelian311 xuelian311 modified the milestones: 4.2.x, 4.2.3 Jul 5, 2023
@kurozumi kurozumi closed this Aug 25, 2023
@kurozumi kurozumi deleted the liip-imagine-sub-directory-fix branch February 27, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants