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

マスタデータのEntityを拡張すると、マスタデータ管理ページでテーブルを選択できなくなる問題の対策 #5401

Open
wants to merge 1 commit into
base: 4.3
Choose a base branch
from

Conversation

kztomita
Copy link
Contributor

概要(Overview・Refs Issue)

#5400 の修正

方針(Policy)

Proxyファイルのパスを生成する際、basename($sourceFile)ではなく、
$entityClass = substr($sourceFile, strlen($projectDir . '/src/Eccube/Entity/'));
として$sourceFileから先頭の/var/www/ec-cube/src/Eccube/Entity/を除去するように
変更しました。

実装に関する補足(Appendix)

(1)
basename()からsubstrに()よる部分的なパスの除去に変更になっていますが、
入力値である$sourceFileはRecursiveDirectoryIteratorにより生成されるデータであるため、directory traversalの問題は発生しないと考えます。

(2)
従来のコードではあらゆるファイルに対してProxyファイルのパスを生成して、
Proxyファイルの有無をチェックしていましたが、

$entityPath = $projectDir . '/src/Eccube/Entity/';
if (strpos($sourceFile, $entityPath) === 0) {
}

のようにしてEntity配下のクラスについてのみProxyファイルのパスを生成して、
Proxyファイルの有無チェックが動作するように変更しています。

探索対象である$this->pathsは外部から指定できるので、
将来、['/var/www/ec-cube/src/Eccube/']のようにEntity以外のクラスも含むパスを
指定して呼び出された場合にも正しく動作させるためです。

テスト(Test)

(1)
プラグインから
Plugin/[Plugin code]/Entity/Master/DeviceTypeTrait.php
のようにしてマスタデータのEntityを拡張し、
\Eccube\Doctrine\ORM\Mapping\Driver\AnnotationDriver::getAllClassNames()
が返すクラス一覧に'Eccube\Entity\Master\DeviceType'が含まれるようになった
ことを確認。

(2)
管理画面のマスタデータ管理のプルダウンにmtb_device_typeが表示されるようになったことを確認。

相談(Discussion)

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

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

レビュワー確認項目

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

@codecov-commenter
Copy link

Codecov Report

Merging #5401 (53fd424) into 4.1 (6c466ad) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                4.1    #5401      +/-   ##
============================================
+ Coverage     68.58%   68.59%   +0.01%     
- Complexity     6162     6164       +2     
============================================
  Files           463      463              
  Lines         25306    25310       +4     
============================================
+ Hits          17356    17362       +6     
+ Misses         7950     7948       -2     
Flag Coverage Δ
E2E 57.95% <100.00%> (+<0.01%) ⬆️
Unit 76.15% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...e/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php 76.47% <100.00%> (+2.00%) ⬆️
...be/Service/PurchaseFlow/Processor/TaxProcessor.php 73.77% <0.00%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c466ad...53fd424. Read the comment docs.

@chihiro-adachi chihiro-adachi added this to the 4.1.x milestone Jun 22, 2022
@xuelian311 xuelian311 modified the milestones: 4.1.x, 4.2.x May 8, 2023
@xuelian311 xuelian311 modified the milestones: 4.2.x, 4.2.3 Jul 7, 2023
@dotani1111 dotani1111 modified the milestones: 4.2.3, 4.2.x Nov 10, 2023
@ji-eunsoo ji-eunsoo changed the base branch from 4.1 to 4.3 March 27, 2024 07:18
@ji-eunsoo ji-eunsoo modified the milestones: 4.2.x, 4.3.0 Mar 27, 2024
@nanasess nanasess self-assigned this Mar 27, 2024
@nanasess
Copy link
Contributor

nanasess commented Apr 2, 2024

当方でユニットテストを作成し、動作確認してみます

@nanasess
Copy link
Contributor

nanasess commented Apr 3, 2024

以下のE2Eテストを追加して検証しています。
nanasess@4f7b522

本修正により、 Eccube\Entity\Master 配下の Entity に対して Schema Update が自動実行されないようです。
(手動で bin/console doctrine:schema:update --force を実行すれば、正常動作することは確認済み)
https://github.com/nanasess/ec-cube/actions/runs/8532978876/job/23374978428#step:13:72

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.

7 participants