allow custom qbpager page parameter - #227
Conversation
|
|
||
| $this->_pager_id = $pager_id; | ||
| $this->_prefix = 'org_openpsa_qbpager_' . $pager_id . '_'; | ||
| $this->_prefix = $prefix ?? 'org_openpsa_qbpager_' . $pager_id . '_page'; |
There was a problem hiding this comment.
this I find a bit hard to read. Is it ($prefix ?? 'org_openpsa_qbpager_') . $pager_id . '_page' or is it $prefix ?? ('org_openpsa_qbpager_' . $pager_id . '_page')?
| $this->_current_page = max(1, (int) $_REQUEST[$page_var]); | ||
| } | ||
| $results_var = $this->_prefix . 'results'; | ||
| $results_var = 'org_openpsa_qbpager_' . $this->_pager_id . '_results'; |
There was a problem hiding this comment.
seems a bit inconsistent to not use the prefix here. Why does this need to change?
| private $total; | ||
|
|
||
| public function __construct(string $classname, string $pager_id) | ||
| public function __construct(string $classname, string $pager_id, ?string $prefix = null) |
There was a problem hiding this comment.
maybe as a more general comment: Is it really necessary to add a new parameter here? Shouldn't pager_id be enough to uniquely identify a pager instance?
There was a problem hiding this comment.
i couldn’t merge initialize() into __construct() because the parent constructor can’t be used here. it expects a DBA class and tries to resolve the classname through the MidCOM class loader. based on the tests midgard_style is already a raw MgdSchema class, so the conversion fails before the direct query builder can be created
There was a problem hiding this comment.
ah yeah, makes sense. Didn't notice that qbpager extends querybuilder
2 participants
No description provided.