Ticket #1296 (closed バグ指摘: 修正済)

Opened 9 years ago

Last modified 8 years ago

SC_* のコンストラクタの拡張が無視される

Reported by: Seasoft Owned by: shutta
Priority: Milestone: EC-CUBE2.12.0
Component: その他 Version: 2.11.1
Keywords: Cc:
修正済み: yes

Description (last modified by Seasoft) (diff)

課題

  • 拡張ページクラスで親クラスのコンストラクタをコールするように改訂すると、(EC-CUBE の標準実装の開発として) コンストラクタの引数を変更する場合に2つのクラスの同期を保証する必要があり、メンテナンスコストが上がる。
    • PHP の場合、「関数処理」関数で回避する方法も考えられそう。しかし、この場合実行コストがどの程度上がるか把握していない。また、ソースの連続性がなくなり、ソース分析が面倒になる (多分、ツール類では構造分析できない)。
  • PHP の言語仕様 (コンストラクタは継承しない) との整合は無視するのか?

Change History

comment:1 Changed 9 years ago by Seasoft

  • Owner changed from somebody to Seasoft

comment:2 Changed 9 years ago by Seasoft

  • Status changed from new to closed
  • Resolution set to 無効
  • Summary changed from SC_View のコンストラクタの拡張が無視される to SC_* のコンストラクタの拡張が無視される

親クラスのコンストラクタを呼び出す実装をプロトタイピングしましたが、EC-CUBE 本体の開発に対する保守性が悪くなりそうなので破棄します。

カスタマイズの際は、拡張クラスを使用せずに親クラスを書き換えるのが妥当なんですかね。

comment:3 follow-up: ↓ 4 Changed 9 years ago by shutta

  • Status changed from closed to reopened
  • Resolution 無効 deleted

個人的には、拡張クラスがある以上は、現状ではカスタマイズは拡張クラスを使用するのを想定するのが本来だと思っています。

そのため、2.11の際に、ひと通りclass_extendsに対応させましたが、 SC_View周りを r20306 で、拡張クラス対応した際にコンストラクタ部分まで考慮できていませんでした。

本チケットは、閉じられてしまってますが、構造に関して再度議論できればと思い、復活させて頂きます。

comment:4 in reply to: ↑ 3 Changed 9 years ago by Seasoft

  • Owner Seasoft deleted
  • Status changed from reopened to new
  • Description modified (diff)

個人的には、拡張クラスを次バージョンあたりで削除するのが理想と考えているので破棄したのですが、2.11 系としての対応としては、本来はバグとして修正するのが正しいと思います。

そのため、2.11の際に、ひと通りclass_extendsに対応させましたが、 SC_View周りを r20306 で、拡張クラス対応した際にコンストラクタ部分まで考慮できていませんでした。

SC_View に限らず、SC_* と SC_Helper_* 全般での対応が必要になると思います。

参考までに、私が検討した際の課題を説明欄に記載しておきますね。

comment:5 Changed 9 years ago by kajiwara

  • Milestone changed from EC-CUBE2.11.2 to EC-CUBE2.11.3(仮)

一旦次期開発に移行します。

comment:6 Changed 9 years ago by AMUAMU

次期開発に向けて・・・ 個人的にはshuttaさんと同様にclass_extends以下を使用する方向を支持したい方なので コンストラクタを使わない実装など構造検討出来ると良いと思います。

PHP の場合、「関数処理」関数で回避する方法も考えられそう。しかし、この場合実行コストがどの程度上がるか把握していない。

PHP5.2以降は実行コストはそこまで上がらなかったはずです

comment:7 Changed 9 years ago by kotani

  • Milestone changed from EC-CUBE2.11.3 to EC-CUBE2.11.4(仮)

comment:8 Changed 9 years ago by kotani

  • Milestone changed from EC-CUBE2.11.5 to EC-CUBE2.12.0alpha

comment:9 follow-up: ↓ 10 Changed 9 years ago by shutta

2.12リリースが近いですし、安直な方法として、EC-CUBE2.12からはPHP4縛りが外れるので、__construct()を使用するよう書き換えるのは、どうでしょうか?

EC-CUBE 2.12.0 システム要件
 http://xoops.ec-cube.net/modules/newbb/viewtopic.php?viewmode=thread&topic_id=9755&forum=14&post_id=46921

コンストラクタを継承する場合は、parent::__construct()を呼べば済みますし、必要なければ拡張クラス側ではコンストラクタを定義しないままで良いですし。

comment:10 in reply to: ↑ 9 Changed 9 years ago by Seasoft

本件を抜きとしても、construct() への置換に賛成です。

comment:11 Changed 9 years ago by shutta

  • Owner set to shutta
  • Status changed from new to assigned

r21765 にて、手始めにSC_*Viewクラスを弄ってみました。

孫クラスまであって、かつ途中でコンストラクタの形式が変更されていたりと、面倒なことになっていましたが、(私なりに)手早く改良できる形で改修してみました。

こんな感じで、どんなもんでしょうか?

comment:12 Changed 9 years ago by shutta

残りのその他のクラスは、機械的に変更すれば良かったので特に困りませんでした。(r21767)

comment:13 Changed 9 years ago by shutta

r21767 にコミット漏れしていました。(r21768)

comment:14 Changed 9 years ago by shutta

SC_Graph_Barクラスでは、親クラスのコンストラクタ(SC_Graph_Line)をオーバーライドしている変な実装だったので漏れていた。 (r21769)

comment:15 Changed 8 years ago by shutta

  • 修正済み set

comment:16 Changed 8 years ago by kajiwara

  • Status changed from assigned to closed
  • Resolution set to 修正済

こちらでクローズとさせていただきます。ご対応ありがとうございました。

Note: See TracTickets for help on using tickets.