Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors a complex SQL query method in the DoctrineSpeakerRepository to use Common Table Expressions (CTEs) for better readability and maintainability. The refactoring transforms a deeply nested query with multiple UNION operations into a cleaner CTE-based approach.
- Replaced complex nested queries with CTEs (QualifiedEvents, SpeakerIds, SpeakerRows)
- Changed IFNULL to COALESCE for better SQL standard compliance
- Improved code formatting and added comprehensive inline documentation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public function getSpeakersBySummitAndOnSchedule( | ||
| Summit $summit, | ||
| PagingInfo $paging_info, | ||
| Filter $filter = null, | ||
| Order $order = null | ||
| ) { |
There was a problem hiding this comment.
The method is missing its PHPDoc comment. This is a public API method that should have proper documentation describing its parameters and return type.
| (F.PresentationSpeakerID IS NOT NULL) AS Featured | ||
| FROM (SELECT DISTINCT SpeakerID FROM SpeakerIds) ds | ||
| JOIN PresentationSpeaker S ON S.ID = ds.SpeakerID | ||
| LEFT JOIN Member M ON M.ID = S.MemberID | ||
| LEFT JOIN SpeakerRegistrationRequest R ON R.SpeakerID = S.ID | ||
| LEFT JOIN Summit_FeaturedSpeakers F | ||
| ON F.PresentationSpeakerID = S.ID AND F.SummitID = {$summitId} |
There was a problem hiding this comment.
The Featured column logic has changed from using EXISTS to checking if a JOIN result is NOT NULL. This could potentially affect performance and behavior if there are multiple matching records in Summit_FeaturedSpeakers table, unlike the original EXISTS which was guaranteed to return boolean.
smarcet
force-pushed
the
main
branch
4 times, most recently
from
c6ecdd0 to
728ae67
Compare
1 participant
No description provided.