chore: refactor DoctrineSummitEventRepository::getAllByPage #396
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the getAllByPage method in DoctrineSummitEventRepository to optimize performance by implementing a two-phase query approach: first fetching IDs with filters/ordering, then retrieving full entities by ID.
- Replaced the original
getAllByPagemethod with a new implementation usinggetFastCountandgetAllIdsByPagehelper methods - Introduced a join catalog system with
ensureJoinandrequiredAliasesmethods for dynamic join management - Reorganized filter and order mappings for better maintainability
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!is_null($filter)) { | ||
| $filter->apply2Query($query, $this->getFilterMappings($filter)); | ||
|
|
||
| if($filter->hasFilter('actions')) { | ||
| // if actions filter is required, we should do an inner join with the allowed | ||
| // actions of the selection plan of the presentation | ||
| $query = $query->andWhere("allowed_at_type = at"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated filter application logic - the filter is applied twice on lines 624 and 628. This could cause incorrect query results or performance issues.
| $query = $this->applyExtraFilters($query); | ||
|
|
||
| if(!is_null($order)){ | ||
| $order->apply2Query($query, $this->getOrderMappings($filter)); |
There was a problem hiding this comment.
Inconsistent parameter usage for getOrderMappings() - sometimes called with $filter parameter and sometimes without. This should be standardized across all usages.
| $query = $this->applyExtraFilters($query); | ||
|
|
||
| if(!is_null($order)){ | ||
| $order->apply2Query($query, $this->getOrderMappings($filter)); |
There was a problem hiding this comment.
Inconsistent parameter usage for getOrderMappings() - sometimes called with $filter parameter and sometimes without. This should be standardized across all usages.
| $shouldPerformRandomOrderingByPage = true; | ||
| $order->removeOrder("page_random"); | ||
| } | ||
| $order->apply2Query($query, $this->getOrderMappings()); |
There was a problem hiding this comment.
Inconsistent parameter usage for getOrderMappings() - sometimes called with $filter parameter and sometimes without. This should be standardized across all usages.
| $order->apply2Query($query, $this->getOrderMappings()); | |
| $order->apply2Query($query, $this->getOrderMappings($filter)); |
smarcet
force-pushed
the
hotfix/event-repo-get-all
branch
9 times, most recently
from
039e63c to
17005e9
Compare
optimice performance
smarcet
force-pushed
the
hotfix/event-repo-get-all
branch
from
17005e9 to
c15ce73
Compare
ref https://app.clickup.com/t/86b6ycj3j
optimize performance