COmanage Developers List

[Benn Oshrin <>]

On 3/27/13 2:52 AM, Marie Huynh wrote:
> On Mar 26, 2013 6:43 AM, "Benn Oshrin" 
> <
> <mailto:>>
>  wrote:
>  >
>  > On 3/26/13 11:19 AM, Marie Huynh wrote:
>  >>
>  >> To get the search function working, I put some of the code in index() in
>  >> StandardController.php.  It seems reusable should search be implemented
>  >> for other models.  Problem is, index() is already a pretty big function
>  >> and it would make more sense shifted into paginationConditions.
>  >
>  > What does the code that you're putting in StandardController do?
>
> It modifies the pagination conditions to filter the items in index.

So one thing to keep in mind is that paginationConditions() is intended 
to be overridden by other controllers. There are three that do this. One 
option is for those to call parent::paginationConditions(), but we 
generally haven't been following that approach for overrideable methods 
in standardController, and I'm not sure we should start now.

> (I agree that search is a reusable concept, but there's a lot of crap in
> AppController and StandardContrller that probably shouldn't have been
> there in the first place.)
>  >
>  >> The problem with paginationConditions() is that it is only called if not
>  >> $this->requires_person.  Why?  HistoryRecordsController.php
>  >> and MVPAController.php (and subclasses) will skip
>  >> paginationConditions().  Subclasses include:
>  >>
>  >> AddressesController.php:class AddressesController extends
> MVPAController {
>  >> EmailAddressesController.php:class EmailAddressesController extends
>  >> MVPAController {
>  >> IdentifiersController.php:class IdentifiersController extends
>  >> MVPAController {
>  >> TelephoneNumbersController.php:class TelephoneNumbersController extends
>  >> MVPAController {
>  >>
>  >> Is there any reason why we wouldn't bother searching these, or can I go
>  >> ahead and have paginationConditions() run for all extensions of
>  >> Standardcontroller so I can shift my code there?
>  >
>  >
>  > I think this falls into the "crap" category, but I think we should
> first confirm where the search code goes before cleaning up this stuff.
> (This would also confirm if it should go in paginationConditions.)
>
> I'm pretty sure it should go in paginationConditions. The real question
> was if there's anything that would break that I need to watch out for in
> popping the call to it out of the else.

Before paginationConditions() was added, StandardController determined 
what conditions to set based on the type of the request coming in. This 
"old logic" was left in place using a $this->requires_person test with 
the intention of eventually switching everything over to using 
paginationConditions() at some point in the future.

That is, the code within if($this->requires_person) should eventually be 
put into model specific overrides of paginationConditions(), or perhaps 
into a single MVPAController override.

-Benn-