Validate the UCP location and number preferences

event/ucp_listener.php

@@ -28,6 +28,16 @@
  */
 class ucp_listener implements EventSubscriberInterface
 {
+	/** Index-page block locations a user may choose */
+	const LOCATIONS = ['RT_TOP', 'RT_BOTTOM', 'RT_SIDE'];
+
+	/** Forum-view block locations a user may choose; there is no side column there */
+	const VIEWFORUM_LOCATIONS = ['RT_TOP', 'RT_BOTTOM'];
+
+	/** Range for the number of topics per page, matching the UCP form's max */
+	const NUMBER_MIN = 1;
+	const NUMBER_MAX = 999;
+
 	/**
 	* @var auth
 	*/
@@ -120,9 +130,17 @@ public function ucp_prefs_get_data($event)
 		$event['data'] = array_merge(
 			$event['data'], array(
 			'rt_enable'          => $this->request->variable('rt_enable', (int) $this->user->data['user_rt_enable']),
-			'rt_location'        => $this->request->variable('rt_location', $this->user->data['user_rt_location']),
-			'rt_viewforum_location' => $this->request->variable('rt_viewforum_location', $this->user->data['user_rt_viewforum_location']),
-			'rt_number'          => $this->request->variable('rt_number', (int) $this->user->data['user_rt_number']),
+			'rt_location'        => $this->valid_location(
+				$this->request->variable('rt_location', $this->user->data['user_rt_location']),
+				self::LOCATIONS, $this->user->data['user_rt_location'], $this->config['rt_location']
+			),
+			'rt_viewforum_location' => $this->valid_location(
+				$this->request->variable('rt_viewforum_location', $this->user->data['user_rt_viewforum_location']),
+				self::VIEWFORUM_LOCATIONS, $this->user->data['user_rt_viewforum_location'], $this->config['rt_viewforum_location']
+			),
+			'rt_number'          => max(self::NUMBER_MIN, min(self::NUMBER_MAX,
+				$this->request->variable('rt_number', (int) $this->user->data['user_rt_number'])
+			)),
 			'rt_sort_start_time' => $this->request->variable('rt_sort_start_time', (int) $this->user->data['user_rt_sort_start_time']),
 			'rt_unread_only'     => $this->request->variable('rt_unread_only', (int) $this->user->data['user_rt_unread_only']),
 			)
@@ -274,6 +292,30 @@ public function ucp_prefs_set_data($event)
 		$event['sql_ary'] = array_merge($event['sql_ary'], $sql_ary);
 	}
 
+	/**
+	 * Return a submitted block location if it is one of the allowed options (issue #198).
+	 *
+	 * Otherwise keep the user's stored location, or use the board default if that is not valid either.
+	 *
+	 * @param  string $submitted Location from the request
+	 * @param  array  $allowed   Valid locations for this setting
+	 * @param  string $stored    The user's current location
+	 * @param  string $default   The board-wide default location
+	 * @return string
+	 */
+	private function valid_location($submitted, array $allowed, $stored, $default)
+	{
+		foreach ([$submitted, $stored, $default] as $location)
+		{
+			if (in_array($location, $allowed, true))
+			{
+				return $location;
+			}
+		}
+
+		return $allowed[0];
+	}
+
 	/**
 	 * set a newly registered account's Recent Topics preferences from default.
 	 *

styles/all/template/event/ucp_prefs_view_select_menu_append.html

@@ -36,7 +36,7 @@
 	{% if A_RT_NUMBER %}
 	<dl>
 		<dt><label for="rt_number">{{ lang('RT_NUMBER') }}{{ lang('COLON') }}</label><br /><span>{{ lang('RT_NUMBER_EXP') }}</span></dt>
-		<dd><input type="number" id="rt_number" name="rt_number" size="6" min="0" max="999" value="{{ RT_NUMBER }}" class="inputbox autowidth" /></dd>
+		<dd><input type="number" id="rt_number" name="rt_number" size="6" min="1" max="999" value="{{ RT_NUMBER }}" class="inputbox autowidth" /></dd>
 	</dl>
 	{% endif %}
 

tests/event/ucp_listener_test.php

@@ -237,6 +237,80 @@ public function test_ucp_prefs_get_data_no_submit()
 		$this->assertEquals(5, $event['data']['rt_number']);
 	}
 
+	/**
+	 * Submitted location / number preferences, and what must reach $event['data'] (#198).
+	 * Stored user values: location RT_BOTTOM, viewforum location RT_TOP, number 5.
+	 */
+	public function submitted_preferences_data()
+	{
+		return array(
+			'valid values pass through'           => array('RT_SIDE', 'RT_BOTTOM', 20, 'RT_SIDE', 'RT_BOTTOM', 20),
+			'unknown location keeps stored value' => array('RT_EVIL', 'RT_TOP', 5, 'RT_BOTTOM', 'RT_TOP', 5),
+			'side is not a viewforum location'    => array('RT_TOP', 'RT_SIDE', 5, 'RT_TOP', 'RT_TOP', 5),
+			'number above 999 is clamped'         => array('RT_TOP', 'RT_TOP', 100000, 'RT_TOP', 'RT_TOP', 999),
+			'zero is raised to 1'                 => array('RT_TOP', 'RT_TOP', 0, 'RT_TOP', 'RT_TOP', 1),
+			'negative number is raised to 1'      => array('RT_TOP', 'RT_TOP', -5, 'RT_TOP', 'RT_TOP', 1),
+		);
+	}
+
+	/**
+	 * @dataProvider submitted_preferences_data
+	 */
+	public function test_submitted_preferences_are_validated($location, $vf_location, $number, $expected_location, $expected_vf_location, $expected_number)
+	{
+		$this->user->data = array(
+			'user_rt_enable'             => 1,
+			'user_rt_location'           => 'RT_BOTTOM',
+			'user_rt_viewforum_location' => 'RT_TOP',
+			'user_rt_number'             => 5,
+			'user_rt_sort_start_time'    => 0,
+			'user_rt_unread_only'        => 0,
+		);
+
+		$submitted = array('rt_location' => $location, 'rt_viewforum_location' => $vf_location, 'rt_number' => $number);
+		$this->request->method('variable')
+			->willReturnCallback(function ($var, $default) use ($submitted) {
+				return $submitted[$var] ?? $default;
+			});
+
+		$this->set_listener();
+
+		$event = new \phpbb\event\data(array('data' => array(), 'submit' => true));
+		$this->listener->ucp_prefs_get_data($event);
+
+		$this->assertSame($expected_location, $event['data']['rt_location']);
+		$this->assertSame($expected_vf_location, $event['data']['rt_viewforum_location']);
+		$this->assertSame($expected_number, $event['data']['rt_number']);
+	}
+
+	/**
+	 * A stored location that is itself invalid falls back to the board default (#198).
+	 */
+	public function test_invalid_stored_location_falls_back_to_board_default()
+	{
+		$this->config['rt_location'] = 'RT_SIDE';
+		$this->user->data = array(
+			'user_rt_enable'             => 1,
+			'user_rt_location'           => 'GARBAGE',
+			'user_rt_viewforum_location' => 'RT_TOP',
+			'user_rt_number'             => 5,
+			'user_rt_sort_start_time'    => 0,
+			'user_rt_unread_only'        => 0,
+		);
+
+		$this->request->method('variable')
+			->willReturnCallback(function ($var, $default) {
+				return $var === 'rt_location' ? 'RT_EVIL' : $default;
+			});
+
+		$this->set_listener();
+
+		$event = new \phpbb\event\data(array('data' => array(), 'submit' => true));
+		$this->listener->ucp_prefs_get_data($event);
+
+		$this->assertSame('RT_SIDE', $event['data']['rt_location']);
+	}
+
 	public function test_ucp_prefs_get_data_on_submit()
 	{
 		$this->user->data = array(

tests/tests.md

@@ -88,7 +88,7 @@ It creates the listener with mocked dependencies — a `config` object with real
 `event/ucp_listener.php` lets registered users customise their own Recent Topics experience. They can choose how many topics to show, where to position the block, whether to show only unread topics, and so on. The listener hooks into the UCP display-preferences page to show those settings and save them.
 
 There are three handlers:
-- `ucp_prefs_get_data` — runs on page load AND on form submit; builds the data array and (only on page load) renders the UCP template block
+- `ucp_prefs_get_data` — runs on page load AND on form submit; builds the data array, keeping the locations within their allowed options and the number within 1–999, and (only on page load) renders the UCP template block
 - `ucp_prefs_set_data` — maps the submitted form fields to the SQL column names used in `phpbb_users`
 - `ucp_register_set_data` — runs on `core.ucp_register_register_after`, after the new account is inserted, and writes the global config defaults to that user's row
 
@@ -100,6 +100,8 @@ Creates the listener with mocks for auth, config, request, template, user, langu
 | `test_getSubscribedEvents` | Verifies the 3 expected event subscriptions | Accidentally removed subscription stops the UCP page from working |
 | `test_ucp_prefs_set_data` | Submits 5 preference fields | Each `data['rt_*']` field must map to the correct `sql_ary['user_rt_*']` column; a mismatch means preferences silently fail to save |
 | `test_ucp_prefs_get_data_no_submit` | Page load (submit = false) | Must: merge user DB values into `data`, call `add_lang()`, call `template->assign_vars()` |
+| `test_submitted_preferences_are_validated` | Data provider: valid values; unknown location; `RT_SIDE` as viewforum location; numbers 100000, 0 and -5 | Locations outside the allowed options fall back to the user's stored value; the number is clamped to 1–999 (#198) |
+| `test_invalid_stored_location_falls_back_to_board_default` | Submitted and stored locations both invalid | The board default `rt_location` is used (#198) |
 | `test_ucp_prefs_get_data_on_submit` | Form submit (submit = true) | Must: merge data; must NOT call `template->assign_vars()` — template must only be touched on page load, not on form processing |
 | `test_register_defaults_run_after_the_account_exists` | Event map | `ucp_register_set_data` must be on `core.ucp_register_register_after` (fires after `user_add()`, carries `user_id`) and not on `core.ucp_register_data_after` (fires during validation, no `user_id`, so the UPDATE hit `user_id = 0`; #196) |
 | `test_ucp_register_set_data` | New user registration (user_id = 3) | Must call `sql_build_array('UPDATE', ...)` with all 5 default values, then `sql_query()` with `WHERE user_id = 3` — verified by mock expectations on the db object |