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

Doctrine ORM3.0ではサポートされないので、flushにエンティティを指定している処理を修正。 #4925

Merged
merged 10 commits into from
Mar 16, 2021

Conversation

kurozumi
Copy link
Contributor

@kurozumi kurozumi commented Feb 17, 2021

概要(Overview・Refs Issue)

Doctrine ORM3.0ではサポートされないので、
flushにエンティティを指定している処理部分をpersistまたはremoveでエンティティを指定してflushするよう変更しました。

https://github.com/doctrine/orm/blob/ebae57eb9637acd8252b398df3121b120688ed5c/lib/Doctrine/ORM/EntityManager.php#L367-L372

doctrine/orm#8459

※EC-CUBE4.0ではDoctrine ORM3.0をサポートしない気がするのですがいかがでしょうか?

方針(Policy)

実装に関する補足(Appendix)

テスト(Test)

相談(Discussion)

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

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

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@nanasess
Copy link
Contributor

※EC-CUBE4.0ではDoctrine ORM3.0をサポートしない気がするのですがいかがでしょうか?

#4528 でも ORM2.8 にしているので、 ORM 3.0 への対応は当分先になるかもしれません。
が、この PR を取り込められれば、 #4528 での ORM 3.0 のサポートが前進しそうです。

@okazy okazy added the improvement 機能改善 label Feb 17, 2021
@okazy okazy added this to the 4.0.x milestone Feb 17, 2021
Copy link
Contributor

@matsuoshi matsuoshi left a comment

Choose a reason for hiding this comment

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

ありがとうございます、内容問題ないと思います

@okazy
Copy link
Contributor

okazy commented Mar 10, 2021

こちらのテストが落ちていました。
テストを再実行してみます。

1) EF03OrderCest: Ef0303-uc01-t01_購入フローでログインしたタイミングで複数カートになったらログイン後にカート画面に戻す
 Test  codeception/acceptance/EF03OrderCest.php:order_ログイン後に複数カートになればカートに戻す
 Step  See "同時購入できない商品がカートに含まれています",".ec-alert-warning__text"
 Fail  Element located either by name, CSS or XPath element with '.ec-alert-warning__text' was not found.

@okazy
Copy link
Contributor

okazy commented Mar 10, 2021

また以下でテストが落ちました。

EF03OrderCest: Ef0303-uc01-t01_購入フローでログインしたタイミングで複数カートになったらログイン後にカート画面に戻す

原因を調査してから取り込みたいです。

Copy link
Contributor

@okazy okazy left a comment

Choose a reason for hiding this comment

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

解決策は調査中ですが、このままでは取り込めないのでレビューステータスを変えておきます。

}
$this->entityManager->flush($Cart);
$this->entityManager->persist($Cart);
$this->entityManager->flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

ログイン時にカートが別れる場合にエラーが発生します。

image

@okazy
Copy link
Contributor

okazy commented Mar 16, 2021

@kurozumi
テストが落ちていたので修正のプルリクをお送りしました。
kurozumi#6
ご確認とマージをお願いできますでしょうか。

@codecov-io
Copy link

codecov-io commented Mar 16, 2021

Codecov Report

Attention: Patch coverage is 82.35294% with 9 lines in your changes missing coverage. Please review.

Please upload report for BASE (4.0@6e33056). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../Eccube/Controller/Admin/Order/OrderController.php 63.63% 4 Missing ⚠️
...Eccube/Controller/Admin/Content/PageController.php 33.33% 2 Missing ⚠️
src/Eccube/Service/CartService.php 71.42% 2 Missing ⚠️
src/Eccube/Service/PluginService.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             4.0    #4925   +/-   ##
======================================
  Coverage       ?   76.19%           
  Complexity     ?     6124           
======================================
  Files          ?      437           
  Lines          ?    20744           
  Branches       ?        0           
======================================
  Hits           ?    15806           
  Misses         ?     4938           
  Partials       ?        0           
Flag Coverage Δ
tests 76.19% <82.35%> (?)

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

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

@kurozumi
Copy link
Contributor Author

@okazy

ありがとうございます。反映しました。

Copy link
Contributor

@okazy okazy left a comment

Choose a reason for hiding this comment

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

ありがとうございます!

@matsuoshi matsuoshi merged commit b0bc5ef into EC-CUBE:4.0 Mar 16, 2021
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.

6 participants