From 9389f7a368fddea5f395bdef70e26da209fdfd99 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 9 Aug 2026 23:30:11 +0200 Subject: [PATCH 1/2] Gives each post one quick-moderation checkbox, and no empty submit A topic draws two of these strips, one for the page and one for the mobile menu, and both instances append their own checkbox to the same container. So every post carried two of them side by side, and ticking one left the other strip's count wrong. Reuse whatever checkbox is already in the container and just listen to it as well. 2.1 built the Remove/Restore/Split buttons on the first checkbox click. 3.0 moved that block into init(), which left them on screen from page load, with nothing selected and no count. Pressing one then submitted the form with no msgs[] at all, and quickmod2 read $_REQUEST['msgs'] unguarded: a blank page, an undefined key and array_map() on null. Show the buttons from the same place that already counts, so they start hidden, and let the action redirect back to the topic if it is handed nothing. The move also left the else branch assigning to oNewDiv, which is not declared anywhere. Signed-off-by: Mathias Papenbrock Signed-off-by: albertlast --- Sources/Actions/QuickModerationInTopic.php | 10 ++- Themes/default/scripts/topic.js | 77 +++++++++++++++------- 2 files changed, 61 insertions(+), 26 deletions(-) diff --git a/Sources/Actions/QuickModerationInTopic.php b/Sources/Actions/QuickModerationInTopic.php index 9d7edfbddf1..920134cda4c 100644 --- a/Sources/Actions/QuickModerationInTopic.php +++ b/Sources/Actions/QuickModerationInTopic.php @@ -62,6 +62,14 @@ public function execute(): void // Check the session = get or post. User::$me->checkSession('request'); + // Nothing ticked, nothing to do. Every branch below assumes at least + // one message: splitting takes the min() of them, and the other two + // hand the list to an {array_int:...}, which will not accept an empty + // one. + if (empty($this->messages)) { + Utils::redirectexit('topic=' . Topic::$topic_id . '.' . ($_REQUEST['start'] ?? 0)); + } + if (isset($_REQUEST['restore_selected'])) { $this->restore(); } elseif (isset($_REQUEST['split_selection'])) { @@ -80,7 +88,7 @@ public function execute(): void */ protected function __construct() { - $this->messages = array_map('intval', $_REQUEST['msgs']); + $this->messages = array_map('intval', (array) ($_REQUEST['msgs'] ?? [])); } /** diff --git a/Themes/default/scripts/topic.js b/Themes/default/scripts/topic.js index 6b2cd75c57e..59e546b06c4 100755 --- a/Themes/default/scripts/topic.js +++ b/Themes/default/scripts/topic.js @@ -875,17 +875,31 @@ InTopicModeration.prototype.init = function() // Add checkboxes to all the messages. for (var i = 0, n = this.opt.aMessageIds.length; i < n; i++) { - // Create the checkbox. - var oCheckbox = document.createElement('input'); - oCheckbox.type = 'checkbox'; - oCheckbox.className = this.opt.sButtonStrip + '_check'; - oCheckbox.name = 'msgs[]'; - oCheckbox.value = this.opt.aMessageIds[i]; - oCheckbox.onclick = this.handleClick.bind(this, oCheckbox); - // Append it to the container var oCheckboxContainer = document.getElementById(this.opt.sCheckboxContainerMask + this.opt.aMessageIds[i]); - oCheckboxContainer.appendChild(oCheckbox); + + /* + * The topic draws two of these strips - one for the page and one for + * the mobile menu - and they share the containers, so the second + * instance finds a checkbox already sitting here. Take that one and + * listen to it as well, rather than putting a second checkbox beside + * every post. + */ + var oCheckbox = oCheckboxContainer.querySelector('input[name="msgs[]"]'); + + if (!oCheckbox) + { + // Create the checkbox. + oCheckbox = document.createElement('input'); + oCheckbox.type = 'checkbox'; + oCheckbox.className = this.opt.sButtonStrip + '_check'; + oCheckbox.name = 'msgs[]'; + oCheckbox.value = this.opt.aMessageIds[i]; + + oCheckboxContainer.appendChild(oCheckbox); + } + + oCheckbox.addEventListener('click', this.handleClick.bind(this, oCheckbox)); oCheckboxContainer.style.display = ''; } @@ -898,8 +912,8 @@ InTopicModeration.prototype.init = function() else { oButtonStripDisplay = document.createElement('div'); - oNewDiv.id = this.opt.sButtonStripDisplay; - oNewDiv.className = this.opt.sButtonStripClass || 'buttonlist floatbottom'; + oButtonStripDisplay.id = this.opt.sButtonStripDisplay; + oButtonStripDisplay.className = this.opt.sButtonStripClass || 'buttonlist floatbottom'; oButtonStrip.appendChild(oButtonStripDisplay); } @@ -936,6 +950,14 @@ InTopicModeration.prototype.init = function() ['click', this.handleSubmit.bind(this, 'split')] ] }); + + /* + * Nothing is selected yet, so put the buttons in the state that says so. + * They used to be built on the first click instead of here, which is why + * nothing hid them to begin with - and pressing one with an empty + * selection submits the form with no msgs[] at all. + */ + this.updateButtons(); } InTopicModeration.prototype.handleClick = function(oCheckbox) @@ -944,23 +966,28 @@ InTopicModeration.prototype.handleClick = function(oCheckbox) // Keep stats on how many items were selected. this.iNumSelected += oCheckbox.checked ? 1 : -1; - // Show the number of messages selected in each of the buttons. - if (this.opt.bCanRemove && !this.opt.bUseImageButton) - { - this.oRemoveButton.innerHTML = this.opt.sRemoveButtonLabel + ' [' + this.iNumSelected + ']'; - this.oRemoveButton.style.display = this.iNumSelected < 1 ? "none" : ""; - } + this.updateButtons(); +} - if (this.opt.bCanRestore && !this.opt.bUseImageButton) - { - this.oRestoreButton.innerHTML = this.opt.sRestoreButtonLabel + ' [' + this.iNumSelected + ']'; - this.oRestoreButton.style.display = this.iNumSelected < 1 ? "none" : ""; - } +// Show the number of messages selected in each of the buttons, and hide them +// while that number is zero. +InTopicModeration.prototype.updateButtons = function() +{ + var aButtons = [ + [this.opt.bCanRemove, this.oRemoveButton, this.opt.sRemoveButtonLabel], + [this.opt.bCanRestore, this.oRestoreButton, this.opt.sRestoreButtonLabel], + [this.opt.bCanSplit, this.oSplitButton, this.opt.sSplitButtonLabel] + ]; - if (this.opt.bCanSplit && !this.opt.bUseImageButton) + for (var i = 0; i < aButtons.length; i++) { - this.oSplitButton.innerHTML = this.opt.sSplitButtonLabel + ' [' + this.iNumSelected + ']'; - this.oSplitButton.style.display = this.iNumSelected < 1 ? "none" : ""; + if (!aButtons[i][0] || !aButtons[i][1]) + continue; + + if (!this.opt.bUseImageButton) + aButtons[i][1].innerHTML = aButtons[i][2] + ' [' + this.iNumSelected + ']'; + + aButtons[i][1].style.display = this.iNumSelected < 1 ? "none" : ""; } } From 4a254979095d74f95c63d0d399cd1ec1782a2419 Mon Sep 17 00:00:00 2001 From: albertlast Date: Mon, 10 Aug 2026 16:12:08 +0200 Subject: [PATCH 2/2] Collects the existing checkboxes in one pass Review asked for the selector to come out of the loop. The scoped call could not move as it stood, since the container it looks inside is a different element on every pass, so this queries the document once and indexes what it finds by the message id each checkbox carries. This file is the only thing that emits a msgs[] input, so an unscoped query has nothing else to pick up. Signed-off-by: albertlast --- Themes/default/scripts/topic.js | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/Themes/default/scripts/topic.js b/Themes/default/scripts/topic.js index 59e546b06c4..99d00937c3c 100755 --- a/Themes/default/scripts/topic.js +++ b/Themes/default/scripts/topic.js @@ -872,20 +872,27 @@ function InTopicModeration(oOptions) InTopicModeration.prototype.init = function() { + /* + * The topic draws two of these strips - one for the page and one for the + * mobile menu - and they share the containers, so the second instance + * finds the checkboxes the first one already made. Collect those once, + * under the message id each one carries, and listen to them as well + * rather than putting a second checkbox beside every post. This file is + * the only thing that emits a msgs[] input, so there is nothing else on + * the page for this to pick up. + */ + var oExisting = {}; + var aCheckboxes = document.querySelectorAll('input[name="msgs[]"]'); + + for (var j = 0, m = aCheckboxes.length; j < m; j++) + oExisting[aCheckboxes[j].value] = aCheckboxes[j]; + // Add checkboxes to all the messages. for (var i = 0, n = this.opt.aMessageIds.length; i < n; i++) { // Append it to the container var oCheckboxContainer = document.getElementById(this.opt.sCheckboxContainerMask + this.opt.aMessageIds[i]); - - /* - * The topic draws two of these strips - one for the page and one for - * the mobile menu - and they share the containers, so the second - * instance finds a checkbox already sitting here. Take that one and - * listen to it as well, rather than putting a second checkbox beside - * every post. - */ - var oCheckbox = oCheckboxContainer.querySelector('input[name="msgs[]"]'); + var oCheckbox = oExisting[this.opt.aMessageIds[i]]; if (!oCheckbox) {