[ticket/16470] Revert session_time sync back

This reverts changes made by last 2 commits as they seem to break things.

PHPBB3-16470

phpBB/includes/acp/acp_main.php

@@ -403,6 +403,7 @@ class acp_main
 								'session_forum_id'		=> $user->page['forum'],
 								'session_user_id'		=> (int) $user->data['user_id'],
 								'session_start'			=> (int) $user->data['session_start'],
+								'session_last_visit'	=> (int) $user->data['session_last_visit'],
 								'session_time'			=> (int) $user->time_now,
 								'session_browser'		=> (string) trim(substr($user->browser, 0, 149)),
 								'session_forwarded_for'	=> (string) $user->forwarded_for,

phpBB/includes/functions.php

@@ -3816,7 +3816,7 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
 	}
 
 	// Last visit date/time
-	$s_last_visit = ($user->data['user_id'] != ANONYMOUS) ? $user->format_date($user->data['user_lastvisit']) : '';
+	$s_last_visit = ($user->data['user_id'] != ANONYMOUS) ? $user->format_date($user->data['session_last_visit']) : '';
 
 	// Get users online list ... if required
 	$l_online_users = $online_userlist = $l_online_record = $l_online_time = '';
@@ -3854,10 +3854,10 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
 	{
 		if ($user->data['user_new_privmsg'])
 		{
-			if (!$user->data['user_last_privmsg'] || $user->data['user_last_privmsg'] > $user->data['user_lastvisit'])
+			if (!$user->data['user_last_privmsg'] || $user->data['user_last_privmsg'] > $user->data['session_last_visit'])
 			{
 				$sql = 'UPDATE ' . USERS_TABLE . '
-					SET user_last_privmsg = ' . $user->data['user_lastvisit'] . '
+					SET user_last_privmsg = ' . $user->data['session_last_visit'] . '
 					WHERE user_id = ' . $user->data['user_id'];
 				$db->sql_query($sql);
 

phpBB/includes/functions_display.php

@@ -1594,14 +1594,21 @@ function phpbb_show_profile($data, $user_notes_enabled = false, $warn_user_enabl
 	if ($config['load_onlinetrack'])
 	{
 		$update_time = $config['load_online_time'] * 60;
-		$online = (time() - $update_time < $data['user_lastvisit']) && (!empty($data['session_viewonline']) || $auth->acl_get('u_viewonline'));
+		$online = (time() - $update_time < $data['session_time'] && ((isset($data['session_viewonline']) && $data['session_viewonline']) || $auth->acl_get('u_viewonline'))) ? true : false;
 	}
 	else
 	{
 		$online = false;
 	}
 
-	$last_active = ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline')) ? $data['user_lastvisit'] : '';
+	if ($data['user_allow_viewonline'] || $auth->acl_get('u_viewonline'))
+	{
+		$last_active = (!empty($data['session_time'])) ? $data['session_time'] : $data['user_lastvisit'];
+	}
+	else
+	{
+		$last_active = '';
+	}
 
 	$age = '';
 

phpBB/memberlist.php

@@ -663,13 +663,14 @@ switch ($mode)
 
 		if ($config['load_onlinetrack'])
 		{
-			$sql = 'SELECT MIN(session_viewonline) AS session_viewonline
+			$sql = 'SELECT MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline
 				FROM ' . SESSIONS_TABLE . "
 				WHERE session_user_id = $user_id";
 			$result = $db->sql_query($sql);
 			$row = $db->sql_fetchrow($result);
 			$db->sql_freeresult($result);
 
+			$member['session_time'] = (isset($row['session_time'])) ? $row['session_time'] : 0;
 			$member['session_viewonline'] = (isset($row['session_viewonline'])) ? $row['session_viewonline'] : 0;
 			unset($row);
 		}
@@ -1632,8 +1633,8 @@ switch ($mode)
 		// So, did we get any users?
 		if (count($user_list))
 		{
-			// Get recent session viewonline flags
+			// Session time?! Session time...
-			$sql = 'SELECT session_user_id, MIN(session_viewonline) AS session_viewonline
+			$sql = 'SELECT session_user_id, MAX(session_time) AS session_time, MIN(session_viewonline) AS session_viewonline
 				FROM ' . SESSIONS_TABLE . '
 				WHERE session_time >= ' . (time() - $config['session_length']) . '
 					AND ' . $db->sql_in_set('session_user_id', $user_list) . '
@@ -1644,6 +1645,7 @@ switch ($mode)
 			while ($row = $db->sql_fetchrow($result))
 			{
 				$session_ary[$row['session_user_id']] = [
+					'session_time' => $row['session_time'],
 					'session_viewonline' => $row['session_viewonline'],
 				];
 			}
@@ -1709,8 +1711,9 @@ switch ($mode)
 			$id_cache = array();
 			while ($row = $db->sql_fetchrow($result))
 			{
+				$row['session_time'] = $session_ary[$row['user_id']]['session_time'] ?? 0;
 				$row['session_viewonline'] = $session_ary[$row['user_id']]['session_viewonline'] ?? 0;
-				$row['last_visit'] = $row['user_lastvisit'];
+				$row['last_visit'] = (!empty($row['session_time'])) ? $row['session_time'] : $row['user_lastvisit'];
 
 				$id_cache[$row['user_id']] = $row;
 			}

phpBB/phpbb/db/migration/data/v33x/update_user_lastvisit_data.php

@@ -25,11 +25,6 @@ class update_user_lastvisit_data extends \phpbb\db\migration\migration
 	public function update_data()
 	{
 		return [
-			'drop_columns'	=> [
-				$this->table_prefix . 'sessions' => [
-					'session_last_visit',
-				],
-			],
 			['custom', [[$this, 'update_user_lastvisit_fields']]],
 		];
 	}

phpBB/phpbb/session.php

@@ -441,9 +441,9 @@ class session
 						$this->check_ban_for_current_session($config);
 
 						// Update user last visit time accordingly, but in a minute or so
-						if ((int) $this->time_now - (int) $this->data['user_lastvisit'] > 60)
+						if ((int) $this->data['session_time'] - (int) $this->data['user_lastvisit'] > 60)
 						{
-							$this->update_user_lastvisit((int) $this->time_now);
+							$this->update_user_lastvisit();
 						}
 
 						return true;
@@ -643,6 +643,15 @@ class session
 			$db->sql_freeresult($result);
 		}
 
+		if ($this->data['user_id'] != ANONYMOUS && !$bot)
+		{
+			$this->data['session_last_visit'] = (isset($this->data['session_time']) && $this->data['session_time']) ? $this->data['session_time'] : (($this->data['user_lastvisit']) ? $this->data['user_lastvisit'] : time());
+		}
+		else
+		{
+			$this->data['session_last_visit'] = $this->time_now;
+		}
+
 		// Force user id to be integer...
 		$this->data['user_id'] = (int) $this->data['user_id'];
 
@@ -681,11 +690,11 @@ class session
 			{
 				$this->session_id = $this->data['session_id'];
 
-				// Only sync user last visit time in a minute or so or if the page changes
+				// Only sync user last visit time in a minute or so after last session data update or if the page changes
-				if ((int) $this->time_now - (int) $this->data['user_lastvisit'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page']))
+				if ((int) $this->data['session_time'] - (int) $this->data['user_lastvisit'] > 60 || ($this->update_session_page && $this->data['session_page'] != $this->page['page']))
 				{
 					// Update the last visit time
-					$this->update_user_lastvisit((int) $this->time_now);
+					$this->update_user_lastvisit();
 				}
 
 				$SID = '?sid=';
@@ -706,6 +715,7 @@ class session
 		$sql_ary = array(
 			'session_user_id'		=> (int) $this->data['user_id'],
 			'session_start'			=> (int) $this->time_now,
+			'session_last_visit'	=> (int) $this->data['session_last_visit'],
 			'session_time'			=> (int) $this->time_now,
 			'session_browser'		=> (string) trim(substr($this->browser, 0, 149)),
 			'session_forwarded_for'	=> (string) $this->forwarded_for,
@@ -815,14 +825,14 @@ class session
 		}
 		else
 		{
-			$this->data['session_time'] = $this->data['user_lastvisit'] = $this->time_now;
+			$this->data['session_time'] = $this->data['session_last_visit'] = $this->time_now;
 
 			$SID = '?sid=';
 			$_SID = '';
 		}
 
 		// Update the last visit time
-		$this->update_user_lastvisit($this->time_now);
+		$this->update_user_lastvisit();
 
 		$session_data = $sql_ary;
 		/**
@@ -933,16 +943,82 @@ class session
 	/**
 	* Session garbage collection
 	*
-	* Effectively delete any sessions, autologin keys and login attempts data
+	* This looks a lot more complex than it really is. Effectively we are
-	* older than an admin definable limits.
+	* deleting any sessions older than an admin definable limit. Due to the
-	*
+	* way in which we maintain session data we have to ensure we update user
-	* @return void
+	* data before those sessions are destroyed. In addition this method
+	* removes autologin key information that is older than an admin defined
+	* limit.
 	*/
 	function session_gc()
 	{
 		global $db, $config, $phpbb_container, $phpbb_dispatcher;
 
-		$this->time_now = $this->time_now ?: time();
+		if (!$this->time_now)
+		{
+			$this->time_now = time();
+		}
+
+		/**
+		 * Get most recent session for each registered user to sync user last visit with it
+		 * Inner SELECT gets most recent sessions for each unique session_user_id
+		 * Outer SELECT gets data for them
+		 */
+		$sql_select = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time
+			FROM ' . SESSIONS_TABLE . ' AS s1
+			INNER JOIN (
+				SELECT session_user_id, MAX(session_time) AS recent_time
+				FROM ' . SESSIONS_TABLE . '
+				WHERE session_user_id <> ' . ANONYMOUS . '
+				GROUP BY session_user_id
+			) AS s2
+			ON s1.session_user_id = s2.session_user_id
+				AND s1.session_time = s2.recent_time';
+
+		switch ($db->get_sql_layer())
+		{
+			case 'sqlite3':
+				if (phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>='))
+				{
+					// For SQLite versions 3.8.3+ which support Common Table Expressions (CTE)
+					$sql = "WITH s3 (session_page, session_user_id, session_time) AS ($sql_select)
+						UPDATE " . USERS_TABLE . '
+						SET (user_lastpage, user_lastvisit) = (SELECT session_page, session_time FROM s3 WHERE session_user_id = user_id)
+						WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)';
+					$db->sql_query($sql);
+
+					break;
+				}
+
+			// No break, for SQLite versions prior to 3.8.3 and Oracle
+			case 'oracle':
+				$result = $db->sql_query($sql_select);
+				while ($row = $db->sql_fetchrow($result))
+				{
+					$sql = 'UPDATE ' . USERS_TABLE . '
+						SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "'
+						WHERE user_id = " . (int) $row['session_user_id'];
+					$db->sql_query($sql);
+				}
+				$db->sql_freeresult($result);
+			break;
+
+			case 'mysqli':
+				$sql = 'UPDATE ' . USERS_TABLE . " u,
+					($sql_select) s3
+					SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page
+					WHERE u.user_id = s3.session_user_id";
+				$db->sql_query($sql);
+			break;
+
+			default:
+				$sql = 'UPDATE ' . USERS_TABLE . "
+					SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page
+					FROM ($sql_select) s3
+					WHERE user_id = s3.session_user_id";
+				$db->sql_query($sql);
+			break;
+		}
 
 		// Delete all expired sessions
 		$sql = 'DELETE FROM ' . SESSIONS_TABLE . '
@@ -1724,20 +1800,19 @@ class session
 	/**
 	 * Update user last visit time
 	 *
-	 * @param int	$user_lastvisit	Timestamp to update user_lastvisit field to
 	 * @return bool
 	 */
-	public function update_user_lastvisit(int $user_lastvisit): bool
+	public function update_user_lastvisit(): bool
 	{
 		global $db;
 
-		if (empty($this->data['user_id']) || empty($user_lastvisit))
+		if (!isset($this->data['session_time'], $this->data['user_id']))
 		{
 			return false;
 		}
 
 		$sql = 'UPDATE ' . USERS_TABLE . '
-			SET user_lastvisit = ' . (int) $user_lastvisit . '
+			SET user_lastvisit = ' . (int) $this->data['session_time'] . '
 			WHERE user_id = ' . (int) $this->data['user_id'];
 		$db->sql_query($sql);
 

tests/session/garbage_collection_test.php

@@ -60,6 +60,22 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
 			'Before test, should get recent expired sessions only.'
 		);
 
+		$this->check_user_session_data(
+			[
+				[
+					'username_clean' => 'bar',
+					'user_lastvisit' => 1400000000,
+					'user_lastpage' => 'oldpage_user_bar.php',
+				],
+				[
+					'username_clean' => 'foo',
+					'user_lastvisit' => 1400000000,
+					'user_lastpage' => 'oldpage_user_foo.php',
+				],
+			],
+			'Before test, users session data is not updated yet.'
+		);
+
 		// There is an error unless the captcha plugin is set
 		$config['captcha_plugin'] = 'core.captcha.plugins.nogd';
 		$this->session->session_gc();
@@ -67,6 +83,22 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
 			[],
 			'After garbage collection, all expired sessions should be removed.'
 		);
+
+		$this->check_user_session_data(
+			[
+				[
+					'username_clean' => 'bar',
+					'user_lastvisit' => '1500000000',
+					'user_lastpage' => 'newpage_user_bar.php',
+				],
+				[
+					'username_clean' => 'foo',
+					'user_lastvisit' => '1500000000',
+					'user_lastpage' => 'newpage_user_foo.php',
+				],
+			],
+			'After garbage collection, users session data should be updated to the recent expired sessions data.'
+		);
 	}
 
 	public function test_cleanup_all()