search

smstw / windwalker-joomla-rad

Windwalker RAD framework for Joomla CMS
http://ventoviro.github.io/windwalker-rad-doc/
1 stars 2 forks source link

Added ListModel UnitTest #102

Closed LeoOnTheEarth closed 9 years ago

LeoOnTheEarth commented 9 years ago

https://github.com/smstw/windwalker-joomla-rad/blob/dev-2.1/src/Model/ListModel.php

asika32764 commented 9 years ago

我建議我們接下來養成一個習慣，Test 寫完順便附上原 class 連結方便 review https://github.com/smstw/windwalker-joomla-rad/blob/dev-2.1/src/Model/ListModel.php

LeoOnTheEarth commented 9 years ago

https://github.com/smstw/windwalker-joomla-rad/blob/cdbe361a47746c1e776bf0e946efd4938d6b1d59/src/Model/ListModel.php#L118

這邊在取得 container 時，Container name 帶的是 null 但是在 Model 的話，Container name 帶的是 $this->option 的值 (雖然 ListModel 繼承了 Model 之後，就變成 null 了) 這樣會不會有問題?

asika32764 commented 9 years ago

感覺上 option 也要事先在 117 行先 init 出來，因為 getContainer() 裡面是會用到 $this->option

asika32764 commented 9 years ago

如果是我會做個簡單的重構，把 guess prefix / name / option 的動作都移到 getPrefix() / getName() / getOption() 裡面並快取

然後 getContainer() 改用 getOption() 來帶入 name

這樣應該就可以讓流程穩定下來了

asika32764 commented 9 years ago

不過這樣子又要考慮到 config 也要暫存或 pass 進去這些method

所以不然還是在 117 行組一次 $this->option 就好了

LeoOnTheEarth commented 9 years ago

我先加了一個 getPrefix method 到 Model class 內

LeoOnTheEarth commented 9 years ago

這支寫好了