From 0e583d312a7af9f0e69cff21cbe5ac5a4d825d3b Mon Sep 17 00:00:00 2001 From: albertlast Date: Wed, 9 Sep 2026 06:58:38 +0200 Subject: [PATCH] Limits a print page to one page of posts Every other view that shows a lot of a topic at once is capped. The "All" view refuses a topic longer than enableAllMessages, and a feed takes at most 255 items. The print view took the lot: one query for every message in the topic, each one parsed and held in memory, for anyone who could read the board. It now shows the same number of posts the admin is willing to show in the "All" view, and pages through the rest with a page index of its own. Forums that never show "All" get a limit of 250 rather than none, since 0 there means the link is off and not that a request may render anything it likes. The rest follows from paging: the page index and the text/images links carry the current start, the poll prints once with the first page rather than on top of every page, and an out of range start redirects the way a topic page does. The page links are plain text because the print page loads neither the theme's icon CSS nor its JavaScript, so the usual arrows and the expanding page list would both be dead there. Co-Authored-By: Claude Opus 5 Signed-off-by: albertlast --- Languages/en_US/Help.php | 2 +- Sources/Actions/TopicPrint.php | 90 ++++++++++- Themes/default/Printpage.template.php | 8 +- tests/Integration/Http/TopicPrintTest.php | 187 ++++++++++++++++++++++ tests/Unit/TopicPrintTest.php | 74 +++++++++ 5 files changed, 355 insertions(+), 6 deletions(-) create mode 100644 tests/Integration/Http/TopicPrintTest.php create mode 100644 tests/Unit/TopicPrintTest.php diff --git a/Languages/en_US/Help.php b/Languages/en_US/Help.php index 13354bf72bc..7e08895ac40 100644 --- a/Languages/en_US/Help.php +++ b/Languages/en_US/Help.php @@ -220,7 +220,7 @@ '; $helptxt['topicSummaryPosts'] = 'This allows you to set the number of previous posts shown in the topic summary on the reply page.'; -$helptxt['enableAllMessages'] = 'Set this to the maximum number of posts a topic can have to show the all link. Setting this lower than "Maximum messages to display in a topic page" will simply mean it never gets shown, and setting it too high could slow down your forum.'; +$helptxt['enableAllMessages'] = 'Set this to the maximum number of posts a topic can have to show the all link. Setting this lower than "Maximum messages to display in a topic page" will simply mean it never gets shown, and setting it too high could slow down your forum. The print view of a topic shows this many posts at a time as well, and falls back to a limit of its own when the all link is turned off here.'; $helptxt['allow_guestAccess'] = 'Unchecking this box will stop guests from doing anything but very basic actions on your forum - login, register, password reminder, etc. - on your forum. This is not the same as disallowing guest access to boards.'; $helptxt['userLanguage'] = 'Turning this setting on will allow users to select which language file they use. It will not affect the default selection.'; diff --git a/Sources/Actions/TopicPrint.php b/Sources/Actions/TopicPrint.php index 0d8b30cd2f5..9241161851d 100644 --- a/Sources/Actions/TopicPrint.php +++ b/Sources/Actions/TopicPrint.php @@ -24,6 +24,7 @@ use SMF\Db\DatabaseApi as Db; use SMF\ErrorHandler; use SMF\Lang; +use SMF\PageIndex; use SMF\Parser; use SMF\Poll; use SMF\Routable; @@ -43,6 +44,16 @@ class TopicPrint implements ActionInterface, Routable use ActionSuffixRouter; use ActionTrait; + /***************** + * Class constants + *****************/ + + /** + * The number of posts to show on one print page in forums that never show + * the "All" view. + */ + public const DEFAULT_MAX_POSTS = 250; + /**************** * Public methods ****************/ @@ -97,7 +108,56 @@ public function execute(): void $row = Db::$db->fetch_assoc($request); Db::$db->free_result($request); - if (!empty($row['id_poll'])) { + // Only the posts this user is allowed to see are printed or counted. + $approval_filter = Config::$modSettings['postmod_active'] && !User::$me->allowedTo('approve_posts') ? ' + AND (m.approved = {int:is_approved}' . (User::$me->is_guest ? '' : ' OR m.id_member = {int:current_member}') . ')' : ''; + + $request = Db::$db->query( + 'SELECT COUNT(*) + FROM {db_prefix}messages AS m + WHERE m.id_topic = {int:current_topic}' . $approval_filter, + [ + 'current_topic' => Topic::$topic_id, + 'is_approved' => 1, + 'current_member' => User::$me->id, + ], + ); + list($total_posts) = Db::$db->fetch_row($request); + Db::$db->free_result($request); + + $per_page = $this->getPostsPerPage(); + + Utils::$context['start'] = (int) $_REQUEST['start']; + + $page_index = new PageIndex( + Config::$scripturl . '?action=printpage;topic=' . Topic::$topic_id . '.%1$d' . (isset($_REQUEST['images']) ? ';images' : ''), + Utils::$context['start'], + (int) $total_posts, + $per_page, + true, + true, + // The print page carries its own styles and no scripts, so the + // icons and the expanding page list have to be plain text here. + [ + 'previous_page' => Lang::getTxt('prev', file: 'General'), + 'next_page' => Lang::getTxt('next', file: 'General'), + 'expand_pages' => ' ... ', + ], + ); + + // If the supplied start value was invalid, redirect to the correct one. + if ($_REQUEST['start'] != Utils::$context['start']) { + Utils::redirectexit(\sprintf($page_index->base_url, Utils::$context['start'])); + } + + // There is nothing to navigate when the whole topic fits on one page. + if ($total_posts > $per_page) { + Utils::$context['page_index'] = $page_index; + } + + // The poll belongs to the topic rather than to any of its posts, so it + // is printed once, with the first page. + if (!empty($row['id_poll']) && Utils::$context['start'] === 0) { $poll = Poll::load(Topic::$topic_id, Poll::LOAD_BY_TOPIC); Utils::$context['poll'] = $poll->format(['no_buttons' => true]); } @@ -120,13 +180,15 @@ public function execute(): void 'SELECT subject, poster_time, body, COALESCE(mem.real_name, poster_name) AS poster_name, id_msg FROM {db_prefix}messages AS m LEFT JOIN {db_prefix}members AS mem ON (mem.id_member = m.id_member) - WHERE m.id_topic = {int:current_topic}' . (Config::$modSettings['postmod_active'] && !User::$me->allowedTo('approve_posts') ? ' - AND (m.approved = {int:is_approved}' . (User::$me->is_guest ? '' : ' OR m.id_member = {int:current_member}') . ')' : '') . ' - ORDER BY m.id_msg', + WHERE m.id_topic = {int:current_topic}' . $approval_filter . ' + ORDER BY m.id_msg + LIMIT {int:per_page} OFFSET {int:start}', [ 'current_topic' => Topic::$topic_id, 'is_approved' => 1, 'current_member' => User::$me->id, + 'per_page' => $per_page, + 'start' => Utils::$context['start'], ], ); Utils::$context['posts'] = []; @@ -215,4 +277,24 @@ public function execute(): void // Set a canonical URL for this page. Utils::$context['canonical_url'] = Config::$scripturl . '?topic=' . Topic::$topic_id . '.0'; } + + /****************** + * Internal methods + ******************/ + + /** + * Works out how many posts belong on a single print page. + * + * A print page holds whole posts, parsed and in memory all at once, so it + * shows no more of them at a time than the admin is willing to show in the + * "All" view of a topic. + * + * @return int The maximum number of posts on one print page. + */ + protected function getPostsPerPage(): int + { + $per_page = (int) (Config::$modSettings['enableAllMessages'] ?? 0); + + return $per_page > 0 ? $per_page : self::DEFAULT_MAX_POSTS; + } } diff --git a/Themes/default/Printpage.template.php b/Themes/default/Printpage.template.php index 056916e9281..244e2667b3d 100644 --- a/Themes/default/Printpage.template.php +++ b/Themes/default/Printpage.template.php @@ -224,12 +224,18 @@ function template_print_below() */ function template_print_options() { - $url_text = Config::$scripturl . '?action=printpage;topic=' . Topic::$topic_id . '.0'; + $url_text = Config::$scripturl . '?action=printpage;topic=' . Topic::$topic_id . '.' . Utils::$context['start']; $url_images = $url_text . ';images'; echo '
'; + // Long topics are printed a page at a time. + if (isset(Utils::$context['page_index'])) { + echo ' +
', Utils::$context['page_index'], '
'; + } + // Which option is set, text or text&images if (isset($_REQUEST['images'])) { echo ' diff --git a/tests/Integration/Http/TopicPrintTest.php b/tests/Integration/Http/TopicPrintTest.php new file mode 100644 index 00000000000..b36bc2d92c4 --- /dev/null +++ b/tests/Integration/Http/TopicPrintTest.php @@ -0,0 +1,187 @@ +seedTopic(); + + $first = $this->fetch('?action=printpage;topic=' . $topic_id . '.0'); + + $this->assertSame( + self::PER_PAGE, + $first->xpath('//div[@class="postheader"]')->length, + 'the print page rendered the whole topic instead of one page of it', + ); + + $this->assertStringNotContainsString( + $this->bodyOfPost(self::POSTS), + $first->text(), + 'the last post of the topic is on the first print page', + ); + + $this->assertNoErrorsLogged('printing a topic logged something.' . "\n"); + } + + public function testTheRestOfTheTopicIsOnTheFollowingPages(): void + { + $topic_id = $this->seedTopic(); + + $last = $this->fetch('?action=printpage;topic=' . $topic_id . '.' . (self::POSTS - 1)); + + $this->assertStringContainsString( + $this->bodyOfPost(self::POSTS), + $last->text(), + 'the last post of the topic cannot be printed at all', + ); + + $this->assertNoErrorsLogged('printing the last page logged something.' . "\n"); + } + + /** + * Without these the rest of a long topic is unreachable, since the print + * page is the only thing that links to itself. + */ + public function testAPrintPageLinksToTheOtherPages(): void + { + $topic_id = $this->seedTopic(); + + $first = $this->fetch('?action=printpage;topic=' . $topic_id . '.0'); + + $this->assertGreaterThan( + 0, + $first->xpath( + '//a[contains(@href, "action=printpage")][contains(@href, "topic=' . $topic_id . '.' . self::PER_PAGE . '")]', + )->length, + 'the print page does not link to its second page', + ); + } + + /****************** + * Internal methods + ******************/ + + protected function setUp(): void + { + parent::setUp(); + + // A print page shows no more posts than the "All" view is allowed to, + // so this is what decides where it stops. + $this->previous_max = $this->rawSetting('enableAllMessages'); + + Config::updateModSettings(['enableAllMessages' => self::PER_PAGE]); + } + + protected function tearDown(): void + { + // Before the parent runs, while the connection is still ours. + Config::updateModSettings(['enableAllMessages' => (int) $this->previous_max]); + + if ($this->created_topics !== []) { + Topic::remove($this->created_topics); + + $this->created_topics = []; + } + + parent::tearDown(); + } + + /** + * Starts a topic holding more posts than one print page may show. + * + * @return int The new topic's id. + */ + private function seedTopic(): int + { + $this->actingAs($this->adminId()); + + $topic_id = 0; + + for ($i = 1; $i <= self::POSTS; $i++) { + $msgOptions = [ + 'subject' => ($i === 1 ? '' : 'Re: ') . 'Print pagination', + 'body' => $this->bodyOfPost($i), + 'send_notifications' => false, + ]; + $topicOptions = [ + 'id' => $topic_id, + 'board' => 1, + ]; + $posterOptions = [ + 'id' => $this->adminId(), + ]; + + Msg::create($msgOptions, $topicOptions, $posterOptions); + + $topic_id = (int) $topicOptions['id']; + + if ($this->created_topics === []) { + $this->created_topics[] = $topic_id; + } + } + + return $topic_id; + } + + /** + * The body of one of the seeded posts. + * + * @param int $number Which post. + * @return string Its body. + */ + private function bodyOfPost(int $number): string + { + return 'Print pagination post number ' . $number . '.'; + } +} diff --git a/tests/Unit/TopicPrintTest.php b/tests/Unit/TopicPrintTest.php new file mode 100644 index 00000000000..dcc6dd0ee8f --- /dev/null +++ b/tests/Unit/TopicPrintTest.php @@ -0,0 +1,74 @@ +assertSame(TopicPrint::DEFAULT_MAX_POSTS, $this->getPostsPerPage()); + } + + // Note: the section banner above must not be the first thing in this group when + // the first member carries an attribute. The SMF/section_comments fixer inserts + // the banner between the attribute and its method. + #[DataProvider('maxTopicSizeProvider')] + public function testGetPostsPerPage(mixed $enable_all_messages, int $expected): void + { + Config::$modSettings['enableAllMessages'] = $enable_all_messages; + + $this->assertSame($expected, $this->getPostsPerPage()); + } + + /*********************** + * Public static methods + ***********************/ + + /** + * @return array + */ + public static function maxTopicSizeProvider(): array + { + return [ + 'the admin sets the size' => ['50', 50], + 'an integer is accepted too' => [50, 50], + 'zero means the admin never shows all' => ['0', TopicPrint::DEFAULT_MAX_POSTS], + 'a negative size is meaningless' => ['-50', TopicPrint::DEFAULT_MAX_POSTS], + 'so is a non-numeric one' => ['lots', TopicPrint::DEFAULT_MAX_POSTS], + ]; + } + + /****************** + * Internal methods + ******************/ + + protected function tearDown(): void + { + unset(Config::$modSettings['enableAllMessages']); + } + + /** + * Calls the protected helper under test. + */ + private function getPostsPerPage(): int + { + $action = (new \ReflectionClass(TopicPrint::class))->newInstanceWithoutConstructor(); + + return (new \ReflectionMethod(TopicPrint::class, 'getPostsPerPage'))->invoke($action); + } +}