From d4325c10fb27a8c7e00f747d9e02ebf204cd1b6c Mon Sep 17 00:00:00 2001 From: Emma Date: Sun, 8 Apr 2018 04:33:54 +0200 Subject: [PATCH] completely redo the way submissions are fetched adds: - keyset pagination (fast!) - ability to limit to forums/users, and exclude forums/users - reintroduced top/controversial sort modes - adds a new 'most commented' sort mode removes: - previous button (no one cared anyway) --- config/app_routes/forum.yaml | 24 +- config/app_routes/forum_category.yaml | 6 +- config/app_routes/front.yaml | 43 +-- config/services.yaml | 1 + src/Controller/AbstractController.php | 6 + src/Controller/ForumCategoryController.php | 9 +- src/Controller/ForumController.php | 41 +-- src/Controller/FrontController.php | 81 ++++-- src/Entity/Submission.php | 4 + .../Submission/NoSubmissionsException.php | 19 ++ src/Repository/Submission/SubmissionPager.php | 109 ++++++++ src/Repository/SubmissionRepository.php | 245 +++++++++++------- templates/_includes/meta_pagination.html.twig | 12 +- templates/_includes/pagination.html.twig | 4 +- templates/_macros/feed_macros.xml.twig | 6 +- templates/front/base.html.twig | 8 +- templates/submission/_macros.html.twig | 4 +- tests/ApplicationAvailabilityTest.php | 67 +++-- translations/messages.en.yml | 1 + 19 files changed, 478 insertions(+), 212 deletions(-) create mode 100644 src/Repository/Submission/NoSubmissionsException.php create mode 100644 src/Repository/Submission/SubmissionPager.php diff --git a/config/app_routes/forum.yaml b/config/app_routes/forum.yaml index adc578d..90915fc 100644 --- a/config/app_routes/forum.yaml +++ b/config/app_routes/forum.yaml @@ -1,24 +1,30 @@ multi: controller: App\Controller\ForumController::multi - defaults: { sortBy: hot, page: 1 } - path: /f/{names}/{sortBy}/{page} + defaults: { sortBy: hot } + path: /f/{names}/{sortBy} requirements: names: '(?:\w{3,25}\+){1,70}\w{3,25}' - sortBy: hot|new + sortBy: "%submission_sort_modes%" forum: controller: App\Controller\ForumController::front - defaults: { sortBy: hot, page: 1 } - path: /f/{forum_name}/{sortBy}/{page} + defaults: { sortBy: hot } + path: /f/{forum_name}/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } forum_feed: controller: App\Controller\ForumController::feed - defaults: { sortBy: hot, page: 1, _format: xml } - path: /f/{forum_name}/{sortBy}/{page}.atom + defaults: { sortBy: hot, _format: xml } + path: /f/{forum_name}/{sortBy}.atom methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } + +forum_feed_legacy_redirect: + controller: FrameworkBundle:Redirect:redirect + defaults: { route: forum_feed, ignoreAttributes: [page] } + path: /f/{forum_name}/{sortBy}/{page}.atom + requirements: { page: \d+ } edit_forum: controller: App\Controller\ForumController::editForum diff --git a/config/app_routes/forum_category.yaml b/config/app_routes/forum_category.yaml index 795d12f..184bf0d 100644 --- a/config/app_routes/forum_category.yaml +++ b/config/app_routes/forum_category.yaml @@ -6,10 +6,10 @@ manage_forum_categories: forum_category: controller: App\Controller\ForumCategoryController::category - defaults: { sortBy: hot, page: 1 } - path: /c/{name}/{sortBy}/{page} + defaults: { sortBy: hot } + path: /c/{name}/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } create_forum_category: controller: App\Controller\ForumCategoryController::create diff --git a/config/app_routes/front.yaml b/config/app_routes/front.yaml index 625c6ab..bd4d02d 100644 --- a/config/app_routes/front.yaml +++ b/config/app_routes/front.yaml @@ -1,41 +1,48 @@ front: controller: App\Controller\FrontController::front - defaults: { sortBy: hot, page: 1 } - path: /{sortBy}/{page} + defaults: { sortBy: hot } + path: /{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } featured: controller: App\Controller\FrontController::featured - defaults: { sortBy: hot, page: 1} - path: /featured/{sortBy}/{page} + defaults: { sortBy: hot } + path: /featured/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } subscribed: controller: App\Controller\FrontController::subscribed - defaults: { sortBy: hot, page: 1 } - path: /subscribed/{sortBy}/{page} + defaults: { sortBy: hot } + path: /subscribed/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } all: controller: App\Controller\FrontController::all - defaults: { sortBy: hot, page: 1 } - path: /all/{sortBy}/{page} + defaults: { sortBy: hot } + path: /all/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } moderated: controller: App\Controller\FrontController::moderated - defaults: { sortBy: hot, page: 1 } - path: /moderated/{sortBy}/{page} + defaults: { sortBy: hot } + path: /moderated/{sortBy} methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } featured_feed: controller: App\Controller\FrontController::featuredFeed - defaults: { sortBy: hot, page: 1, _format: xml } - path: /featured/{sortBy}/{page}.atom + defaults: { sortBy: hot, _format: xml } + path: /featured/{sortBy}.atom methods: [GET] - requirements: { sortBy: hot|new, page: \d+ } + requirements: { sortBy: "%submission_sort_modes%" } + +featured_feed_legacy_redirect: + controller: FrameworkBundle:Redirect:redirect + defaults: { route: featured_feed, ignoreAttributes: true } + methods: [GET] + path: /featured/{sortBy}/{_page}.atom + requirements: { sortBy: "%submission_sort_modes%", page: \d+ } diff --git a/config/services.yaml b/config/services.yaml index 9d66a0b..96601f0 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -6,6 +6,7 @@ parameters: uuid_regex: '[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}' wiki_page_regex: '[A-Za-z][A-Za-z0-9_-]*(/[A-Za-z][A-Za-z0-9_-]*)*' env(APP_ENABLE_WEBHOOKS): false + submission_sort_modes: hot|new|top|controversial|most_commented services: _defaults: diff --git a/src/Controller/AbstractController.php b/src/Controller/AbstractController.php index ce358a8..4874233 100644 --- a/src/Controller/AbstractController.php +++ b/src/Controller/AbstractController.php @@ -2,10 +2,16 @@ namespace App\Controller; +use App\Repository\Submission\SubmissionPager; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController as BaseAbstractController; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; abstract class AbstractController extends BaseAbstractController { + protected function submissionPage(string $sortBy, Request $request): array { + return SubmissionPager::getParamsFromRequest($sortBy, $request); + } + protected function validateCsrf(string $id, string $token) { if (!$this->isCsrfTokenValid($id, $token)) { throw new BadRequestHttpException('Invalid CSRF token'); diff --git a/src/Controller/ForumCategoryController.php b/src/Controller/ForumCategoryController.php index 0af013c..b1f11c1 100644 --- a/src/Controller/ForumCategoryController.php +++ b/src/Controller/ForumCategoryController.php @@ -16,13 +16,16 @@ class ForumCategoryController extends AbstractController { public function category( ForumCategory $category, - string $sortBy, int $page, + string $sortBy, ForumRepository $fr, - SubmissionRepository $sr + SubmissionRepository $sr, + Request $request ): Response { $forums = $fr->findForumsInCategory($category); - $submissions = $sr->findFrontPageSubmissions($forums, $sortBy, $page); + $submissions = $sr->findSubmissions($sortBy, [ + 'forums' => array_keys($forums), + ], $this->submissionPage($sortBy, $request)); return $this->render('forum_category/category.html.twig', [ 'category' => $category, diff --git a/src/Controller/ForumController.php b/src/Controller/ForumController.php index 49efcce..8516aaf 100644 --- a/src/Controller/ForumController.php +++ b/src/Controller/ForumController.php @@ -34,27 +34,34 @@ * @Entity("user", expr="repository.findOneOrRedirectToCanonical(username, 'username')") */ final class ForumController extends AbstractController { + /** + * @var SubmissionRepository + */ + private $submissions; + /** * @var bool */ private $enableWebhooks; - public function __construct(bool $enableWebhooks) { + public function __construct(SubmissionRepository $submissions, bool $enableWebhooks) { + $this->submissions = $submissions; $this->enableWebhooks = $enableWebhooks; } /** * Show the front page of a given forum. * - * @param SubmissionRepository $sr - * @param Forum $forum - * @param string $sortBy - * @param int $page + * @param Forum $forum + * @param string $sortBy * * @return Response */ - public function front(SubmissionRepository $sr, Forum $forum, string $sortBy, int $page) { - $submissions = $sr->findForumSubmissions($forum, $sortBy, $page); + public function front(Forum $forum, string $sortBy, Request $request): Response { + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => [$forum->getId()], + 'stickies' => true, + ], $this->submissionPage($sortBy, $request)); return $this->render('forum/forum.html.twig', [ 'forum' => $forum, @@ -63,8 +70,7 @@ public function front(SubmissionRepository $sr, Forum $forum, string $sortBy, in ]); } - public function multi(ForumRepository $fr, SubmissionRepository $sr, - string $names, string $sortBy, int $page) { + public function multi(ForumRepository $fr, string $names, string $sortBy, Request $request) { $names = preg_split('/[^\w]+/', $names, -1, PREG_SPLIT_NO_EMPTY); $names = array_map(Forum::class.'::normalizeName', $names); $names = $fr->findForumNames($names); @@ -73,7 +79,9 @@ public function multi(ForumRepository $fr, SubmissionRepository $sr, throw $this->createNotFoundException('no such forums'); } - $submissions = $sr->findFrontPageSubmissions($names, $sortBy, $page); + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => array_keys($names), + ], $this->submissionPage($sortBy, $request)); return $this->render('forum/multi.html.twig', [ 'forums' => $names, @@ -148,17 +156,18 @@ public function editForum(Request $request, Forum $forum, EntityManager $em) { } /** - * @param Forum $forum - * @param SubmissionRepository $sr - * @param string $sortBy - * @param int $page + * @param Forum $forum + * @param string $sortBy + * @param Request $request * * @return Response */ - public function feed(Forum $forum, SubmissionRepository $sr, string $sortBy, int $page) { + public function feed(Forum $forum, string $sortBy, Request $request) { return $this->render('forum/feed.xml.twig', [ 'forum' => $forum, - 'submissions' => $sr->findForumSubmissions($forum, $sortBy, $page), + 'submissions' => $this->submissions->findSubmissions($sortBy, [ + 'forums' => [$forum->getId()], + ], $this->submissionPage($sortBy, $request)), ]); } diff --git a/src/Controller/FrontController.php b/src/Controller/FrontController.php index 549b7e3..34dc192 100644 --- a/src/Controller/FrontController.php +++ b/src/Controller/FrontController.php @@ -5,6 +5,8 @@ use App\Entity\User; use App\Repository\ForumRepository; use App\Repository\SubmissionRepository; +use App\Repository\Submission\SubmissionPager; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; /** @@ -25,7 +27,25 @@ * instead. */ final class FrontController extends AbstractController { - public function front(ForumRepository $fr, SubmissionRepository $sr, string $sortBy, int $page) { + /** + * @var ForumRepository + */ + private $forums; + + /** + * @var SubmissionRepository + */ + private $submissions; + + public function __construct( + ForumRepository $forums, + SubmissionRepository $submissions + ) { + $this->forums = $forums; + $this->submissions = $submissions; + } + + public function front(string $sortBy, Request $request): Response { $user = $this->getUser(); if (!$user instanceof User) { @@ -38,21 +58,24 @@ public function front(ForumRepository $fr, SubmissionRepository $sr, string $sor switch ($listing) { case User::FRONT_SUBSCRIBED: - return $this->subscribed($fr, $sr, $sortBy, $page); + return $this->subscribed($sortBy, $request); case User::FRONT_FEATURED: - return $this->featured($fr, $sr, $sortBy, $page); + return $this->featured($sortBy, $request); case User::FRONT_ALL: - return $this->all($sr, $sortBy, $page); + return $this->all($sortBy, $request); case User::FRONT_MODERATED: - return $this->moderated($fr, $sr, $sortBy, $page); + return $this->moderated($sortBy, $request); default: throw new \InvalidArgumentException('bad front page selection'); } } - public function featured(ForumRepository $fr, SubmissionRepository $sr, string $sortBy, int $page) { - $forums = $fr->findFeaturedForumNames(); - $submissions = $sr->findFrontPageSubmissions($forums, $sortBy, $page); + public function featured(string $sortBy, Request $request): Response { + $forums = $this->forums->findFeaturedForumNames(); + + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => array_keys($this->forums->findFeaturedForumNames()), + ], $this->submissionPage($sortBy, $request)); return $this->render('front/featured.html.twig', [ 'forums' => $forums, @@ -62,17 +85,19 @@ public function featured(ForumRepository $fr, SubmissionRepository $sr, string $ ]); } - public function subscribed(ForumRepository $fr, SubmissionRepository $sr, string $sortBy, int $page) { + public function subscribed(string $sortBy, Request $request): Response { $this->denyAccessUnlessGranted('ROLE_USER'); - $forums = $fr->findSubscribedForumNames($this->getUser()); - $hasSubscriptions = count($forums) > 0; + $forums = $this->forums->findSubscribedForumNames($this->getUser()); + $hasSubscriptions = \count($forums) > 0; if (!$hasSubscriptions) { - $forums = $fr->findFeaturedForumNames(); + $forums = $this->forums->findFeaturedForumNames(); } - $submissions = $sr->findFrontPageSubmissions($forums, $sortBy, $page); + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => array_keys($forums), + ], $this->submissionPage($sortBy, $request)); return $this->render('front/subscribed.html.twig', [ 'forums' => $forums, @@ -83,15 +108,9 @@ public function subscribed(ForumRepository $fr, SubmissionRepository $sr, string ]); } - /** - * @param SubmissionRepository $sr - * @param string $sortBy - * @param int $page - * - * @return Response - */ - public function all(SubmissionRepository $sr, string $sortBy, int $page) { - $submissions = $sr->findAllSubmissions($sortBy, $page); + public function all(string $sortBy, Request $request): Response { + $submissions = $this->submissions->findSubmissions($sortBy, [], + $this->submissionPage($sortBy, $request)); return $this->render('front/all.html.twig', [ 'listing' => 'all', @@ -100,11 +119,14 @@ public function all(SubmissionRepository $sr, string $sortBy, int $page) { ]); } - public function moderated(ForumRepository $fr, SubmissionRepository $sr, string $sortBy, int $page) { + public function moderated(string $sortBy, Request $request): Response { $this->denyAccessUnlessGranted('ROLE_USER'); - $forums = $fr->findModeratedForumNames($this->getUser()); - $submissions = $sr->findFrontPageSubmissions($forums, $sortBy, $page); + $forums = $this->forums->findModeratedForumNames($this->getUser()); + + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => array_keys($forums), + ], $this->submissionPage($sortBy, $request)); return $this->render('front/moderated.html.twig', [ 'forums' => $forums, @@ -114,9 +136,12 @@ public function moderated(ForumRepository $fr, SubmissionRepository $sr, string ]); } - public function featuredFeed(ForumRepository $fr, SubmissionRepository $sr, string $sortBy, int $page = 1) { - $forums = $fr->findFeaturedForumNames(); - $submissions = $sr->findFrontPageSubmissions($forums, $sortBy, $page); + public function featuredFeed(string $sortBy, Request $request): Response { + $forums = $this->forums->findFeaturedForumNames(); + + $submissions = $this->submissions->findSubmissions($sortBy, [ + 'forums' => array_keys($forums), + ], $this->submissionPage($sortBy, $request)); return $this->render('front/featured.xml.twig', [ 'forums' => $forums, diff --git a/src/Entity/Submission.php b/src/Entity/Submission.php index 7f82fb4..388160a 100644 --- a/src/Entity/Submission.php +++ b/src/Entity/Submission.php @@ -215,6 +215,10 @@ public function getComments(): Collection { return $this->comments; } + public function getCommentCount(): int { + return \count($this->comments); + } + /** * Get top-level comments, ordered by descending net score. * diff --git a/src/Repository/Submission/NoSubmissionsException.php b/src/Repository/Submission/NoSubmissionsException.php new file mode 100644 index 0000000..67b498a --- /dev/null +++ b/src/Repository/Submission/NoSubmissionsException.php @@ -0,0 +1,19 @@ +query->get('next_'.$column); + $type = SubmissionRepository::SORT_COLUMN_TYPES[$column]; + + if ($value === null || !self::valueIsOfType($type, $value)) { + // missing columns - no pagination + return []; + } + + $params[$column] = $value; + } + + // complete pager params + return $params; + } + + /** + * @param Submission[]|iterable $submissions List of submissions, including + * one more than $maxPerPage to + * tell if there's a next page + * @param int $maxPerPage + * @param string $sortBy property to use for pagination + */ + public function __construct(iterable $submissions, int $maxPerPage, string $sortBy) { + if (!isset(SubmissionRepository::SORT_COLUMN_MAP[$sortBy])) { + throw new \InvalidArgumentException("Invalid sort mode '$sortBy'"); + } + + $count = 0; + + foreach ($submissions as $submission) { + if (++$count > $maxPerPage) { + foreach (SubmissionRepository::SORT_COLUMN_MAP[$sortBy] as $column) { + $accessor = $this->columnNameToAccessor($column); + $value = $submission->{$accessor}(); + + $this->nextPageParams['next_'.$column] = $value; + } + + break; + } + + $this->submissions[] = $submission; + } + } + + public function getIterator() { + return new \ArrayIterator($this->submissions); + } + + public function hasNextPage(): bool { + return (bool) $this->nextPageParams; + } + + /** + * @throws \BadMethodCallException if there is no next page + */ + public function getNextPageParams(): array { + if (!$this->hasNextPage()) { + throw new \BadMethodCallException('There is no next page'); + } + + return $this->nextPageParams; + } + + private function columnNameToAccessor(string $columnName): string { + return 'get'.str_replace('_', '', ucwords($columnName, '_')); + } + + private static function valueIsOfType(string $type, $value): bool { + switch ($type) { + case 'integer': + return ctype_digit($value) && \is_int(+$value) && + $value >= 0x80000000 && $value <= 0x7fffffff; + case 'bigint': + // if this causes problems on 32-bit systems, the site operators + // deserved it. + return ctype_digit($value) && \is_int(+$value); + default: + throw new \InvalidArgumentException("Unexpected type '$type'"); + } + } +} diff --git a/src/Repository/SubmissionRepository.php b/src/Repository/SubmissionRepository.php index 5f2eeb6..e52ded7 100644 --- a/src/Repository/SubmissionRepository.php +++ b/src/Repository/SubmissionRepository.php @@ -2,114 +2,196 @@ namespace App\Repository; -use App\Entity\Forum; use App\Entity\Submission; -use App\Utils\PrependOrderBy; +use App\Repository\Submission\NoSubmissionsException; +use App\Repository\Submission\SubmissionPager; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\Common\Persistence\ManagerRegistry; -use Doctrine\ORM\QueryBuilder; -use Pagerfanta\Adapter\DoctrineORMAdapter; -use Pagerfanta\Pagerfanta; +use Doctrine\DBAL\Query\QueryBuilder; class SubmissionRepository extends ServiceEntityRepository { - const MAX_PER_PAGE = 25; + public const SORT_HOT = 'hot'; + public const SORT_NEW = 'new'; + public const SORT_TOP = 'top'; + public const SORT_CONTROVERSIAL = 'controversial'; + public const SORT_MOST_COMMENTED = 'most_commented'; + + /** + * `$sortBy` -> ordered column name mapping + * + * @var array[] + */ + public const SORT_COLUMN_MAP = [ + self::SORT_HOT => ['ranking', 'id'], + self::SORT_NEW => ['id'], + self::SORT_TOP => ['net_score', 'id'], + self::SORT_CONTROVERSIAL => ['downvotes', 'id'], + self::SORT_MOST_COMMENTED => ['comment_count', 'id'], + ]; + + public const SORT_COLUMN_TYPES = [ + 'ranking' => 'bigint', + 'id' => 'bigint', + 'net_score' => 'integer', + 'downvotes' => 'integer', + 'comment_count' => 'integer', + ]; + + private const MAX_PER_PAGE = 25; + + private const NET_SCORE_JOIN = '('. + 'SELECT submission_id, '. + 'COUNT(*) FILTER (WHERE upvote = TRUE) - '. + 'COUNT(*) FILTER (WHERE upvote = FALSE) AS net_score '. + 'FROM submission_votes '. + 'GROUP BY submission_id'. + ')'; + + // TODO: implement actually useful controversy metric + private const CONTROVERSIAL_JOIN = '('. + 'SELECT submission_id, COUNT(*) AS downvotes '. + 'FROM submission_votes '. + 'WHERE NOT upvote = FALSE '. + 'GROUP BY submission_id'. + ')'; + + private const COMMENT_COUNT_JOIN = '('. + 'SELECT submission_id, COUNT(*) AS comment_count '. + 'FROM comments '. + 'GROUP BY submission_id'. + ')'; public function __construct(ManagerRegistry $registry) { parent::__construct($registry, Submission::class); } /** - * @param string[] $forums array where keys are forum IDs - * @param string $sortBy - * @param int $page + * The amazing submission finder. + * + * @param string $sortBy One of SORT_* constants + * @param array $options An array with the following keys: + * + * @param array $pager + * + * @return Submission[]|SubmissionPager * - * @return Pagerfanta|Submission[] + * @throws \InvalidArgumentException if $sortBy is bad + * @throws NoSubmissionsException if there are no submissions */ - public function findFrontPageSubmissions(array $forums, string $sortBy, int $page = 1) { - if (isset($forums[0])) { - // make sure $forums is id => forum_name array - throw new \InvalidArgumentException('Keys in $forums must be IDs'); - } + public function findSubmissions(string $sortBy, array $options = [], array $pager = []) { + $maxPerPage = $options['max_per_page'] ?? self::MAX_PER_PAGE; - $qb = $this->findSortedQb($sortBy) - ->where('IDENTITY(s.forum) IN (:forums)') - ->setParameter(':forums', array_keys($forums)); + $rsm = $this->createResultSetMappingBuilder('s'); - $submissions = $this->paginate($qb, $page); + $qb = $this->_em->getConnection()->createQueryBuilder() + ->select($rsm->generateSelectClause()) + ->from('submissions', 's') + ->setMaxResults($maxPerPage + 1); - $this->hydrateAssociations($submissions); + switch ($sortBy) { + case self::SORT_HOT: + case self::SORT_NEW: + break; + case self::SORT_TOP: + $qb->join('s', self::NET_SCORE_JOIN, 'ns', 's.id = ns.submission_id'); + break; + case self::SORT_CONTROVERSIAL: + $qb->join('s', self::CONTROVERSIAL_JOIN, 'cn', 's.id = cn.submission_id'); + break; + case self::SORT_MOST_COMMENTED: + $qb->join('s', self::COMMENT_COUNT_JOIN, 'cc', 's.id = cc.submission_id'); + break; + default: + throw new \InvalidArgumentException("Sort mode '$sortBy' not implemented"); + } - return $submissions; - } + if (!$pager && !empty($options['stickies'])) { + // FIXME: won't work if there are >= $maxPerPage stickies (lol) + $qb->orderBy('sticky', 'DESC'); + } - /** - * @param Forum $forum - * @param string $sortBy - * @param int $page - * - * @return Pagerfanta|Submission[] - */ - public function findForumSubmissions(Forum $forum, string $sortBy, int $page = 1) { - $qb = $this->findSortedQb($sortBy) - ->andWhere('s.forum = :forum') - ->setParameter('forum', $forum); + foreach (self::SORT_COLUMN_MAP[$sortBy] as $column) { + $qb->addOrderBy($column, 'DESC'); + } + + if ($pager) { + $qb->andWhere(sprintf('(%s) <= (:next_%s)', + implode(', ', self::SORT_COLUMN_MAP[$sortBy]), + implode(', :next_', self::SORT_COLUMN_MAP[$sortBy]) + )); - if ($sortBy === 'hot') { - PrependOrderBy::prepend($qb, 's.sticky', 'DESC'); + foreach (self::SORT_COLUMN_MAP[$sortBy] as $column) { + $qb->setParameter('next_'.$column, $pager[$column]); + } } - $submissions = $this->paginate($qb, $page); + self::filterQuery($qb, $options); - $this->hydrateAssociations($submissions); + $results = $this->_em + ->createNativeQuery($qb->getSQL(), $rsm) + ->setParameters($qb->getParameters()) + ->execute(); - return $submissions; - } + if ($pager && \count($results) === 0) { + throw new NoSubmissionsException(); + } - /** - * @param string $sortBy - * @param int $page - * - * @return Pagerfanta|Submission[] - */ - public function findAllSubmissions(string $sortBy, int $page = 1) { - $submissions = $this->paginate($this->findSortedQb($sortBy), $page); + $submissions = new SubmissionPager($results, $maxPerPage, $sortBy); $this->hydrateAssociations($submissions); return $submissions; } - /** - * @param string $sortType one of 'hot' or 'new' - * - * @return QueryBuilder - */ - public function findSortedQb($sortType) { - $qb = $this->createQueryBuilder('s'); - - switch ($sortType) { - case 'hot': - $this->sortByHot($qb); - break; - case 'new': - $this->sortByNewest($qb); - break; - case 'top': - throw new \InvalidArgumentException('Sorting by "top" is no longer supported'); - case 'controversial': - throw new \InvalidArgumentException('Sorting by "controversial" is no longer supported'); - default: - throw new \InvalidArgumentException('Bad sort type'); + private static function filterQuery(QueryBuilder $qb, array $options): void { + if (!empty($options['forums'])) { + /* @noinspection NotOptimalIfConditionsInspection */ + if (!empty($options['excluded_forums'])) { + $options['forums'] = array_diff( + $options['forums'], + $options['excluded_forums'] + ); + } + + $qb->andWhere('s.forum_id IN (:forum_ids)'); + $qb->setParameter('forum_ids', $options['forums']); + } elseif (!empty($options['excluded_forums'])) { + $qb->andWhere('s.forum_id NOT IN (:forum_ids)'); + $qb->setParameter('forum_ids', $options['excluded_forums']); } - return $qb; + if (!empty($options['users'])) { + /* @noinspection NotOptimalIfConditionsInspection */ + if (!empty($options['excluded_users'])) { + $options['users'] = array_diff( + $options['users'], + $options['excluded_users'] + ); + } + + $qb->andWhere('s.user_id IN (:user_ids)'); + $qb->setParameter('user_ids', $options['users']); + } elseif (!empty($options['excluded_users'])) { + $qb->andWhere('s.user_id NOT IN (:user_ids)'); + $qb->setParameter('user_ids', $options['excluded_users']); + } } - public function hydrateAssociations($submissions) { + private function hydrateAssociations(iterable $submissions): void { if ($submissions instanceof \Traversable) { $submissions = iterator_to_array($submissions); - } elseif (!is_array($submissions)) { - throw new \InvalidArgumentException('$submissions must be iterable'); } $this->_em->createQueryBuilder() @@ -144,23 +226,4 @@ public function hydrateAssociations($submissions) { ->getQuery() ->getResult(); } - - private function sortByHot(QueryBuilder $qb) { - $qb->addOrderBy('s.ranking', 'DESC'); - $qb->addOrderBy('s.id', 'DESC'); - } - - private function sortByNewest(QueryBuilder $qb) { - $qb->addOrderBy('s.id', 'DESC'); - } - - private function paginate($query, int $page): Pagerfanta { - // I don't think we need to fetch-join when joined entities aren't - // included in the result. - $pager = new Pagerfanta(new DoctrineORMAdapter($query, false, false)); - $pager->setMaxPerPage(self::MAX_PER_PAGE); - $pager->setCurrentPage($page); - - return $pager; - } } diff --git a/templates/_includes/meta_pagination.html.twig b/templates/_includes/meta_pagination.html.twig index a298d0e..f0327eb 100644 --- a/templates/_includes/meta_pagination.html.twig +++ b/templates/_includes/meta_pagination.html.twig @@ -1,9 +1,15 @@ {% with {attr: app.request.attributes, get: app.request.query.all} %} - {% if pager.hasPreviousPage %} - + {% if pager.hasPreviousPage ?? false %} + {% endif %} {% if pager.hasNextPage %} - + {% endif %} {% endwith %} diff --git a/templates/_includes/pagination.html.twig b/templates/_includes/pagination.html.twig index f903444..9a3f5d4 100644 --- a/templates/_includes/pagination.html.twig +++ b/templates/_includes/pagination.html.twig @@ -1,7 +1,7 @@ {% with { attr: app.request.attributes, get: app.request.query.all, - hasPrev: pager.hasPreviousPage, + hasPrev: pager.hasPreviousPage ?? false, hasNext: pager.hasNextPage, } %} {% if hasPrev or hasNext %} @@ -16,7 +16,7 @@ {% endif %} {% if hasNext %} diff --git a/templates/_macros/feed_macros.xml.twig b/templates/_macros/feed_macros.xml.twig index da7c31d..62258f5 100644 --- a/templates/_macros/feed_macros.xml.twig +++ b/templates/_macros/feed_macros.xml.twig @@ -1,11 +1,7 @@ {% macro pagination(pager) %} {% with { route: app.request.attributes.get('_route'), params: app.request.attributes.get('_route_params') ?? {} } %} {% if pager.hasNextPage %} - - {% endif %} - - {% if pager.hasPreviousPage %} - + {% endif %} {% endwith %} {% endmacro %} diff --git a/templates/front/base.html.twig b/templates/front/base.html.twig index 595ef00..cc5fd93 100644 --- a/templates/front/base.html.twig +++ b/templates/front/base.html.twig @@ -33,7 +33,7 @@ {%- macro submission_filter(choice, sort_by) -%} {% with { active: choice == 'featured' } %}
  • - + {{- 'front.featured'|trans -}}
    • @@ -42,7 +42,7 @@ {% if is_granted('ROLE_USER') %} {% with { active: choice == 'subscribed' } %}
  • - + {{- 'front.subscribed'|trans -}}
    • @@ -51,7 +51,7 @@ {% with { active: choice == 'all' } %}
  • - + {{- 'front.all'|trans -}}
    • @@ -60,7 +60,7 @@ {% if choice == 'moderated' or app.user and app.user.moderatorTokens|length > 0 %} {% with { active: choice == 'moderated' } %}
  • - + {{- 'nav.moderated'|trans -}}
    • diff --git a/templates/submission/_macros.html.twig b/templates/submission/_macros.html.twig index dff6112..4c1b14a 100644 --- a/templates/submission/_macros.html.twig +++ b/templates/submission/_macros.html.twig @@ -10,9 +10,9 @@ {%- macro submission_sort(current) -%} {%- set attr = app.request.attributes -%} {%- spaceless -%} - {%- for type in ['hot', 'new'] -%} + {%- for type in ['hot', 'new', 'top', 'controversial', 'most_commented'] -%}
  • - + {{- ('submissions.sort_by_'~type)|trans -}}
    • diff --git a/tests/ApplicationAvailabilityTest.php b/tests/ApplicationAvailabilityTest.php index db35da9..e8a09ac 100644 --- a/tests/ApplicationAvailabilityTest.php +++ b/tests/ApplicationAvailabilityTest.php @@ -14,7 +14,7 @@ class ApplicationAvailabilityTest extends WebTestCase { * @param string $url */ public function testCanAccessPublicPages($url) { - $client = $this->createClient(); + $client = self::createClient(); $client->request('GET', $url); $this->assertTrue($client->getResponse()->isSuccessful()); @@ -26,7 +26,7 @@ public function testCanAccessPublicPages($url) { * @param string $url */ public function testCanAccessPagesThatNeedAuthentication($url) { - $client = $this->createClient([], [ + $client = self::createClient([], [ 'PHP_AUTH_USER' => 'emma', 'PHP_AUTH_PW' => 'goodshit', ]); @@ -41,7 +41,7 @@ public function testCanAccessPagesThatNeedAuthentication($url) { * @param string $url */ public function testCannotAccessPagesThatNeedAuthenticationWhenNotAuthenticated($url) { - $client = $this->createClient(); + $client = self::createClient(); $client->request('GET', $url); $this->assertTrue($client->getResponse()->isRedirect()); @@ -55,10 +55,12 @@ public function testCannotAccessPagesThatNeedAuthenticationWhenNotAuthenticated( * @param string $url */ public function testRedirectedUrlsGoToExpectedLocation($expectedLocation, $url) { - $client = $this->createClient(); + $client = self::createClient(); $client->followRedirects(); $client->request('GET', $url); + $this->assertTrue($client->getResponse()->isSuccessful()); + $this->assertEquals( "http://localhost{$expectedLocation}", $client->getCrawler()->getUri() @@ -73,29 +75,37 @@ public function publicUrlProvider() { yield ['/']; yield ['/hot']; yield ['/new']; - yield ['/hot/1']; - yield ['/new/1']; + yield ['/top']; + yield ['/controversial']; + yield ['/most_commented']; yield ['/all/hot']; yield ['/all/new']; - yield ['/all/hot/1']; - yield ['/all/new/1']; + yield ['/all/top']; + yield ['/all/controversial']; + yield ['/all/most_commented']; yield ['/featured/hot']; yield ['/featured/new']; - yield ['/featured/hot/1']; - yield ['/featured/new/1']; - yield ['/featured/hot/1.atom']; - yield ['/featured/new/1.atom']; + yield ['/featured/top']; + yield ['/featured/controversial']; + yield ['/featured/most_commented']; + yield ['/featured/hot.atom']; + yield ['/featured/new.atom']; + yield ['/featured/top.atom']; + yield ['/featured/controversial.atom']; + yield ['/featured/most_commented.atom']; yield ['/f/news/hot']; yield ['/f/news/new']; - yield ['/f/news/hot/1']; - yield ['/f/news/new/1']; - yield ['/f/news/hot/1.atom']; - yield ['/f/news/new/1.atom']; - yield ['/f/news/1']; + yield ['/f/news/top']; + yield ['/f/news/controversial']; + yield ['/f/news/most_commented']; + yield ['/f/news/hot.atom']; + yield ['/f/news/new.atom']; + yield ['/f/news/top.atom']; + yield ['/f/news/controversial.atom']; + yield ['/f/news/most_commented.atom']; yield ['/f/news/1/comment/1']; yield ['/f/news/bans']; yield ['/f/news/moderation_log']; - yield ['/f/cats/2']; yield ['/forums']; yield ['/forums/by_name']; yield ['/forums/by_title']; @@ -112,16 +122,15 @@ public function publicUrlProvider() { } public function redirectUrlProvider() { - yield ['/f/cats/2', '/f/cats/2/']; yield ['/f/cats', '/f/cats/']; - yield ['/f/cats/2', '/f/CATS/2/']; - yield ['/f/cats/2', '/f/CATS/2']; yield ['/f/news', '/f/NeWs/hot']; yield ['/f/news/new', '/f/NeWs/new']; - yield ['/f/news', '/f/NeWs/hot/1']; - yield ['/f/news/new', '/f/NeWs/new/1']; - yield ['/f/news/1', '/f/NeWs/1']; + yield ['/f/news/top', '/f/NeWs/top']; + yield ['/f/news/controversial', '/f/NeWs/controversial']; + yield ['/f/news/most_commented', '/f/NeWs/most_commented']; yield ['/f/news/1/comment/1', '/f/NeWs/1/comment/1']; + yield ['/f/news/hot.atom', '/f/news/hot/1.atom']; + yield ['/f/news/new.atom', '/f/news/new/1.atom']; } /** @@ -130,12 +139,14 @@ public function redirectUrlProvider() { public function authUrlProvider() { yield ['/subscribed/hot']; yield ['/subscribed/new']; - yield ['/subscribed/hot/1']; - yield ['/subscribed/new/1']; + yield ['/subscribed/top']; + yield ['/subscribed/controversial']; + yield ['/subscribed/most_commented']; yield ['/moderated/hot']; yield ['/moderated/new']; - yield ['/moderated/hot/1']; - yield ['/moderated/new/1']; + yield ['/moderated/top']; + yield ['/moderated/controversial']; + yield ['/moderated/most_commented']; yield ['/create_forum']; yield ['/f/news/edit']; yield ['/f/news/appearance']; diff --git a/translations/messages.en.yml b/translations/messages.en.yml index 552675e..0d39e93 100644 --- a/translations/messages.en.yml +++ b/translations/messages.en.yml @@ -418,6 +418,7 @@ submissions: sort_by_new: New sort_by_top: Top sort_by_controversial: Controversial + sort_by_most_commented: Most commented total_votes: '{1} %count% point|[0,Inf[ %count% points' vote_stats: (+%up%, −%down%) edit_info: (edited %edited_at%)