Merge 4372962e1d into 61bede748a

[ticket/17508] Fix PHP warning on custom profile fields edit

phpBB/develop/adjust_avatars.php

@@ -4,6 +4,9 @@
 *
 * You should make a backup from your users table and the avatar directory in case something goes wrong
 */
+
+use phpbb\storage\provider\local;
+
 die("Please read the first lines of this script for instructions on how to enable it");
 
 set_time_limit(0);
@@ -30,7 +33,7 @@ if (!isset($config['avatar_salt']))
 	die('database not up to date');
 }
 
-if (!isset($config['storage\\avatar\\config\\path']) || $config['storage\\avatar\\config\\path'] !== 'phpbb\\storage\\provider\\local')
+if (!isset($config['storage\\attachment\\provider']) || $config['storage\\attachment\\provider'] !== local::class)
 {
 	die('use local provider');
 }

phpBB/includes/acp/acp_profile.php

@@ -475,6 +475,41 @@ class acp_profile
 					$cp->vars[$key] = $var;
 				}
 
+				// step 3 - all arrays
+				if ($action == 'edit')
+				{
+					// Get language entries
+					$sql = 'SELECT *
+						FROM ' . PROFILE_FIELDS_LANG_TABLE . '
+						WHERE lang_id <> ' . $this->edit_lang_id . "
+							AND field_id = $field_id
+						ORDER BY option_id ASC";
+					$result = $db->sql_query($sql);
+
+					$l_lang_options = [];
+					while ($row = $db->sql_fetchrow($result))
+					{
+						$l_lang_options[$row['lang_id']][$row['option_id']] = $row['lang_value'];
+					}
+					$db->sql_freeresult($result);
+
+					$sql = 'SELECT lang_id, lang_name, lang_explain, lang_default_value
+						FROM ' . PROFILE_LANG_TABLE . '
+						WHERE lang_id <> ' . $this->edit_lang_id . "
+							AND field_id = $field_id
+						ORDER BY lang_id ASC";
+					$result = $db->sql_query($sql);
+
+					$l_lang_name = $l_lang_explain = $l_lang_default_value = [];
+					while ($row = $db->sql_fetchrow($result))
+					{
+						$l_lang_name[$row['lang_id']] = $row['lang_name'];
+						$l_lang_explain[$row['lang_id']] = $row['lang_explain'];
+						$l_lang_default_value[$row['lang_id']] = $row['lang_default_value'];
+					}
+					$db->sql_freeresult($result);
+				}
+
 				foreach ($exclude[3] as $key)
 				{
 					$cp->vars[$key] = $request->variable($key, array(0 => ''), true);

phpBB/phpbb/search/backend/base.php

@@ -112,10 +112,16 @@ abstract class base implements search_backend_interface
 				}
 			}
 
-			// If the sort direction differs from the direction in the cache, then reverse the ids array
+			// If the sort direction differs from the direction in the cache, then recalculate array keys
 			if ($reverse_ids)
 			{
-				$stored_ids = array_reverse($stored_ids);
+				$keys = array_keys($stored_ids);
+				array_walk($keys, function (&$value, $key) use ($result_count)
+					{
+						$value = ($value >= 0) ? $result_count - $value - 1 : $value;
+					}
+				);
+				$stored_ids = array_combine($keys, $stored_ids);
 			}
 
 			for ($i = $start, $n = $start + $per_page; ($i < $n) && ($i < $result_count); $i++)
@@ -166,6 +172,8 @@ abstract class base implements search_backend_interface
 		}
 
 		$store_ids = array_slice($id_ary, 0, $length);
+		$id_range = range($start, $start + $length - 1);
+		$store_ids = array_combine($id_range, $store_ids);
 
 		// create a new resultset if there is none for this search_key yet
 		// or add the ids to the existing resultset
@@ -200,29 +208,26 @@ abstract class base implements search_backend_interface
 			$this->db->sql_query($sql);
 
 			$store = array(-1 => $result_count, -2 => $sort_dir);
-			$id_range = range($start, $start + $length - 1);
 		}
 		else
 		{
 			// we use one set of results for both sort directions so we have to calculate the indizes
-			// for the reversed array and we also have to reverse the ids themselves
+			// for the reversed array
 			if ($store[-2] != $sort_dir)
 			{
-				$store_ids = array_reverse($store_ids);
+				$keys = array_keys($store_ids);
-				$id_range = range($store[-1] - $start - $length, $store[-1] - $start - 1);
+				array_walk($keys, function (&$value, $key) use ($store) {
-			}
+					$value = $store[-1] - $value - 1;
-			else
+				});
-			{
+				$store_ids = array_combine($keys, $store_ids);
-				$id_range = range($start, $start + $length - 1);
 			}
 		}
 
-		$store_ids = array_combine($id_range, $store_ids);
-
 		// append the ids
 		if (is_array($store_ids))
 		{
 			$store += $store_ids;
+			ksort($store);
 
 			// if the cache is too big
 			if (count($store) - 2 > 20 * $this->config['search_block_size'])

phpBB/phpbb/search/backend/fulltext_mysql.php

@@ -550,7 +550,6 @@ class fulltext_mysql extends base implements search_backend_interface
 		}
 		$this->db->sql_freeresult($result);
 
-		$id_ary = array_unique($id_ary);
 		// if the total result count is not cached yet, retrieve it from the db
 		if (!$result_count && count($id_ary))
 		{
@@ -576,9 +575,9 @@ class fulltext_mysql extends base implements search_backend_interface
 				$id_ary[] = (int) $row[$field];
 			}
 			$this->db->sql_freeresult($result);
+		}
 
 		$id_ary = array_unique($id_ary);
-		}
 
 		// store the ids, from start on then delete anything that isn't on the current page because we only need ids for one page
 		$this->save_ids($search_key, implode(' ', $this->split_words), $author_ary, $result_count, $id_ary, $start, $sort_dir);
@@ -758,6 +757,8 @@ class fulltext_mysql extends base implements search_backend_interface
 		// Build the query for really selecting the post_ids
 		if ($type == 'posts')
 		{
+			// For sorting by non-unique columns, add unique sort key to avoid duplicated rows in results
+			$sql_sort .= ', p.post_id' . (($sort_dir == 'a') ? ' ASC' : ' DESC');
 			$sql = "SELECT $sql_select
 				FROM " . $sql_sort_table . POSTS_TABLE . ' p' . (($firstpost_only) ? ', ' . TOPICS_TABLE . ' t ' : ' ') . "
 				WHERE $sql_author
@@ -821,9 +822,9 @@ class fulltext_mysql extends base implements search_backend_interface
 				$id_ary[] = (int) $row[$field];
 			}
 			$this->db->sql_freeresult($result);
+		}
 
 		$id_ary = array_unique($id_ary);
-		}
 
 		if (count($id_ary))
 		{

phpBB/phpbb/search/backend/fulltext_native.php

@@ -1004,6 +1004,8 @@ class fulltext_native extends base implements search_backend_interface
 			$this->db->sql_freeresult($result);
 		}
 
+		$id_ary = array_unique($id_ary);
+
 		// store the ids, from start on then delete anything that isn't on the current page because we only need ids for one page
 		$this->save_ids($search_key, $this->search_query, $author_ary, $total_results, $id_ary, $start, $sort_dir);
 		$id_ary = array_slice($id_ary, 0, (int) $per_page);
@@ -1225,6 +1227,8 @@ class fulltext_native extends base implements search_backend_interface
 		// Build the query for really selecting the post_ids
 		if ($type == 'posts')
 		{
+			// For sorting by non-unique columns, add unique sort key to avoid duplicated rows in results
+			$sql_sort .= ', p.post_id' . (($sort_dir == 'a') ? ' ASC' : ' DESC');
 			$sql = "SELECT $select
 				FROM " . $sql_sort_table . POSTS_TABLE . ' p' . (($firstpost_only) ? ', ' . TOPICS_TABLE . ' t' : '') . "
 				WHERE $sql_author
@@ -1289,6 +1293,8 @@ class fulltext_native extends base implements search_backend_interface
 			$this->db->sql_freeresult($result);
 		}
 
+		$id_ary = array_unique($id_ary);
+
 		if (count($id_ary))
 		{
 			$this->save_ids($search_key, '', $author_ary, $total_results, $id_ary, $start, $sort_dir);

phpBB/phpbb/search/backend/fulltext_postgres.php

@@ -478,8 +478,6 @@ class fulltext_postgres extends base implements search_backend_interface
 		}
 		$this->db->sql_freeresult($result);
 
-		$id_ary = array_unique($id_ary);
-
 		// if the total result count is not cached yet, retrieve it from the db
 		if (!$result_count)
 		{
@@ -509,9 +507,9 @@ class fulltext_postgres extends base implements search_backend_interface
 				$id_ary[] = $row[$field];
 			}
 			$this->db->sql_freeresult($result);
+		}
 
 		$id_ary = array_unique($id_ary);
-		}
 
 		// store the ids, from start on then delete anything that isn't on the current page because we only need ids for one page
 		$this->save_ids($search_key, implode(' ', $this->split_words), $author_ary, $result_count, $id_ary, $start, $sort_dir);
@@ -683,6 +681,8 @@ class fulltext_postgres extends base implements search_backend_interface
 		// Build the query for really selecting the post_ids
 		if ($type == 'posts')
 		{
+			// For sorting by non-unique columns, add unique sort key to avoid duplicated rows in results
+			$sql_sort .= ', p.post_id' . (($sort_dir == 'a') ? ' ASC' : ' DESC');
 			$sql = "SELECT p.post_id
 				FROM " . $sql_sort_table . POSTS_TABLE . ' p' . (($firstpost_only) ? ', ' . TOPICS_TABLE . ' t ' : ' ') . "
 				WHERE $sql_author
@@ -775,9 +775,9 @@ class fulltext_postgres extends base implements search_backend_interface
 				$id_ary[] = (int) $row[$field];
 			}
 			$this->db->sql_freeresult($result);
+		}
 
 		$id_ary = array_unique($id_ary);
-		}
 
 		if (count($id_ary))
 		{

phpBB/phpbb/storage/controller/attachment.php

@@ -24,8 +24,11 @@ use phpbb\exception\http_exception;
 use phpbb\language\language;
 use phpbb\mimetype\extension_guesser;
 use phpbb\request\request;
+use phpbb\storage\provider\local;
 use phpbb\storage\storage;
 use phpbb\user;
+use Symfony\Component\HttpFoundation\BinaryFileResponse;
+use Symfony\Component\HttpFoundation\HeaderUtils;
 use Symfony\Component\HttpFoundation\Request as symfony_request;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Response;
@@ -35,26 +38,41 @@ use Symfony\Component\HttpFoundation\StreamedResponse;
 /**
  * Controller for /download/attachment/{id} routes
  */
-class attachment extends controller
+class attachment
 {
 	/** @var auth */
 	protected $auth;
 
+	/** @var service */
+	protected $cache;
+
 	/** @var config */
 	protected $config;
 
 	/** @var content_visibility */
 	protected $content_visibility;
 
+	/** @var driver_interface */
+	protected $db;
+
 	/** @var dispatcher_interface */
 	protected $dispatcher;
 
+	/** @var extension_guesser */
+	protected $extension_guesser;
+
 	/** @var language */
 	protected $language;
 
 	/** @var request */
 	protected $request;
 
+	/** @var storage */
+	protected $storage;
+
+	/** @var symfony_request */
+	protected $symfony_request;
+
 	/** @var user */
 	protected $user;
 
@@ -76,14 +94,17 @@ class attachment extends controller
 	 */
 	public function __construct(auth $auth, service $cache, config $config, content_visibility $content_visibility, driver_interface $db, dispatcher_interface $dispatcher, extension_guesser $extension_guesser, language $language, request $request, storage $storage, symfony_request $symfony_request, user $user)
 	{
-		parent::__construct($cache, $db, $extension_guesser, $storage, $symfony_request);
-
 		$this->auth = $auth;
+		$this->cache = $cache;
 		$this->config = $config;
 		$this->content_visibility = $content_visibility;
+		$this->db = $db;
 		$this->dispatcher = $dispatcher;
+		$this->extension_guesser = $extension_guesser;
 		$this->language = $language;
 		$this->request = $request;
+		$this->storage = $storage;
+		$this->symfony_request = $symfony_request;
 		$this->user = $user;
 	}
 
@@ -252,21 +273,19 @@ class attachment extends controller
 		);
 		extract($this->dispatcher->trigger_event('core.send_file_to_browser_before', compact($vars)));
 
-		// TODO: The next lines should go better in prepare, also the mimetype is handled by the storage table
+		// Correct the mime type - we force application/octet-stream for all files, except images
-		// so probably can be removed
+		if ($display_cat != attachment_category::IMAGE || !str_starts_with($attachment['mimetype'], 'image'))
-
+		{
-		$response = new StreamedResponse();
+			$attachment['mimetype'] = 'application/octet-stream';
-
+		}
-		// Content-type header
-		$response->headers->set('Content-Type', $attachment['mimetype']);
 
 		// Display file types in browser and force download for others
-		if (strpos($attachment['mimetype'], 'image') !== false
+		if (str_contains($attachment['mimetype'], 'image')
-			|| strpos($attachment['mimetype'], 'audio') !== false
+			|| str_contains($attachment['mimetype'], 'audio')
-			|| strpos($attachment['mimetype'], 'video') !== false
+			|| str_contains($attachment['mimetype'], 'video')
 		)
 		{
-			$disposition = $response->headers->makeDisposition(
+			$disposition = HeaderUtils::makeDisposition(
 				ResponseHeaderBag::DISPOSITION_INLINE,
 				$attachment['real_filename'],
 				$this->filenameFallback($attachment['real_filename'])
@@ -274,20 +293,56 @@ class attachment extends controller
 		}
 		else
 		{
-			$disposition = $response->headers->makeDisposition(
+			$disposition = HeaderUtils::makeDisposition(
 				ResponseHeaderBag::DISPOSITION_ATTACHMENT,
 				$attachment['real_filename'],
 				$this->filenameFallback($attachment['real_filename'])
 			);
 		}
 
+		if ($this->config['storage\\attachment\\provider'] === local::class)
+		{
+			$response = new BinaryFileResponse($this->config['storage\\attachment\\config\\path'] . '/' . $attachment['physical_filename']);
+		}
+		else
+		{
+			$response = new StreamedResponse();
+
+			$fp = $this->storage->read($attachment['physical_filename']);
+
+			$output = fopen('php://output', 'w+b');
+
+			$response->setCallback(function () use ($fp, $output) {
+				stream_copy_to_stream($fp, $output);
+				fclose($fp);
+				fclose($output);
+				flush();
+
+				// Terminate script to avoid the execution of terminate events
+				// This avoid possible errors with db connection closed
+				exit;
+			});
+		}
+
+		// Close db connection
+		$this->file_gc();
+
+		$response->setPrivate();
+
+		// Content-type header
+		$response->headers->set('Content-Type', $attachment['mimetype']);
+
 		$response->headers->set('Content-Disposition', $disposition);
 
+		$response->isNotModified($this->symfony_request);
+
 		// Set expires header for browser cache
 		$time = new \DateTime();
 		$response->setExpires($time->modify('+1 year'));
 
-		return parent::handle($attachment['physical_filename']);
+		@set_time_limit(0);
+
+		return $response;
 	}
 
 	/**
@@ -300,16 +355,6 @@ class attachment extends controller
 		return !empty($filename) ? $filename : 'File';
 	}
 
-	/**
-	 * {@inheritdoc}
-	 */
-	protected function prepare(StreamedResponse $response, string $file): void
-	{
-		$response->setPrivate();	// By default, should be private, but make sure of it
-
-		parent::prepare($response, $file);
-	}
-
 	/**
 	 * Handles authentication when downloading attachments from a post or topic
 	 *
@@ -539,4 +584,13 @@ class attachment extends controller
 
 		return $allowed;
 	}
+
+	/**
+	 * Garbage Collection
+	 */
+	protected function file_gc(): void
+	{
+		$this->cache->unload(); // Equivalent to $this->cache->get_driver()->unload();
+		$this->db->sql_close();
+	}
 }

tests/functional/acp_profile_field_test.php

@@ -69,4 +69,32 @@ class phpbb_functional_acp_profile_field_test extends phpbb_functional_test_case
 
 		$this->assertContainsLang('ADDED_PROFILE_FIELD', $crawler->text());
 	}
+
+	public function test_edit_profile_fields()
+	{
+		// Custom profile fields page
+		$crawler = self::request('GET', 'adm/index.php?i=acp_profile&mode=profile&sid=' . $this->sid);
+
+		// Get all profile fields edit URLs
+		$edits = $crawler->filter('td.actions a')
+			->reduce(
+				function ($node, $i) {
+					$url = $node->attr('href');
+					return ((bool) strpos($url, 'action=edit'));
+			})
+			->each(
+				function ($node, $i) {
+					$url = $node->attr('href');
+					return ($url);
+			});
+
+		foreach ($edits as $edit_url)
+		{
+			$crawler = self::request('GET', 'adm/' . $edit_url . '&sid=' . $this->sid);
+			$form = $crawler->selectButton('Save')->form();
+			$crawler= self::submit($form);
+
+			$this->assertContainsLang('CHANGED_PROFILE_FIELD', $crawler->text());
+		}
+	}
 }

tests/functional/search/base.php

@@ -229,6 +229,159 @@ abstract class phpbb_functional_search_base extends phpbb_functional_test_case
 		$this->delete_topic($topic_multiple_results_count2['topic_id']);
 	}
 
+	public function test_caching_search_results()
+	{
+		global $phpbb_root_path;
+
+		// Sphinx search doesn't use phpBB search results caching
+		if (strpos($this->search_backend, 'fulltext_sphinx'))
+		{
+			$this->markTestSkipped("Sphinx search doesn't use phpBB search results caching");
+		}
+
+		$this->purge_cache();
+		$this->login();
+		$this->admin_login();
+
+		$crawler = self::request('GET', 'search.php?author_id=2&sr=posts');
+		$posts_found_text = $crawler->filter('.searchresults-title')->text();
+
+		// Get total user's post count
+		preg_match('!(\d+)!', $posts_found_text, $matches);
+		$posts_count = (int) $matches[1];
+
+		$this->assertStringContainsString("Search found $posts_count matches", $posts_found_text, $this->search_backend);
+
+		// Set this value to cache less results than total count
+		$sql = 'UPDATE ' . CONFIG_TABLE . '
+			SET config_value = ' . floor($posts_count / 3) . "
+			WHERE config_name = '" . $this->db->sql_escape('search_block_size') . "'";
+		$this->db->sql_query($sql);
+
+		// Temporarily set posts_per_page to the value allowing to get several pages (4+)
+		$crawler = self::request('GET', 'adm/index.php?sid=' . $this->sid . '&i=acp_board&mode=post');
+		$form = $crawler->selectButton('Submit')->form();
+		$values = $form->getValues();
+		$current_posts_per_page = $values['config[posts_per_page]'];
+		$values['config[posts_per_page]'] = floor($posts_count / 10);
+		$form->setValues($values);
+		$crawler = self::submit($form);
+		$this->assertEquals(1, $crawler->filter('.successbox')->count(), $this->search_backend);
+
+		// Now actually test caching search results
+		$this->purge_cache();
+
+		// Default sort direction is 'd' (descending), browse  the 1st page
+		$crawler = self::request('GET', 'search.php?author_id=2&sr=posts');
+		$pagination = $crawler->filter('.pagination')->eq(0);
+		$posts_found_text = $pagination->text();
+
+		$this->assertStringContainsString("Search found $posts_count matches", $posts_found_text, $this->search_backend);
+
+		// Filter all search result page links on the 1st page
+		$pagination = $pagination->filter('li > a')->reduce(
+			function ($node, $i)
+			{
+				return ($node->attr('class') == 'button');
+			}
+		);
+
+		// Get last page number
+		$last_page = (int) $pagination->last()->text();
+
+		// Browse the last search page
+		$crawler = self::$client->click($pagination->selectLink($last_page)->link());
+		$pagination = $crawler->filter('.pagination')->eq(0);
+
+		// Filter all search result page links on the last page
+		$pagination = $pagination->filter('li > a')->reduce(
+			function ($node, $i)
+			{
+				return ($node->attr('class') == 'button');
+			}
+		);
+
+		// Now change sort direction to ascending
+		$form = $crawler->selectButton('sort')->form();
+		$values = $form->getValues();
+		$values['sd'] = 'a';
+		$form->setValues($values);
+		$crawler = self::submit($form);
+
+		$pagination = $crawler->filter('.pagination')->eq(0);
+
+		// Filter all search result page links on the 1st page with new sort direction
+		$pagination = $pagination->filter('li > a')->reduce(
+			function ($node, $i)
+			{
+				return ($node->attr('class') == 'button');
+			}
+		);
+
+		// Browse the rest of search results pages with new sort direction
+		$pages = range(2, $last_page);
+		foreach ($pages as $page_number)
+		{
+			$crawler = self::$client->click($pagination->selectLink($page_number)->link());
+			$pagination = $crawler->filter('.pagination')->eq(0);
+			$pagination = $pagination->filter('li > a')->reduce(
+				function ($node, $i)
+				{
+					return ($node->attr('class') == 'button');
+				}
+			);
+		}
+
+		// Get search results cache varname
+		$finder = new \Symfony\Component\Finder\Finder();
+		$finder
+			->name('data_search_results_*.php')
+			->files()
+			->in($phpbb_root_path . 'cache/' . PHPBB_ENVIRONMENT);
+		$iterator = $finder->getIterator();
+		$iterator->rewind();
+		$cache_filename = $iterator->current();
+		$cache_varname = substr($cache_filename->getBasename('.php'), 4);
+
+		// Get cached post ids data
+		$cache = $this->get_cache_driver();
+		$post_ids_cached = $cache->get($cache_varname);
+		
+		$cached_results_count = count($post_ids_cached) - 2; // Don't count '-1' and '-2' indexes
+
+		$post_ids_cached_backup = $post_ids_cached;
+		
+		// Cached data still should have initial 'd' sort direction
+		$this->assertTrue($post_ids_cached[-2] === 'd', $this->search_backend);
+
+		// Cached search results count should be equal to displayed on search results page
+		$this->assertEquals($posts_count, $post_ids_cached[-1], $this->search_backend);
+
+		// Actual cached data array count should be equal to displayed on search results page too
+		$this->assertEquals($posts_count, $cached_results_count, $this->search_backend);
+
+		// Cached data array shouldn't change after removing duplicates. That is, it shouldn't have any duplicates.
+		unset($post_ids_cached[-2], $post_ids_cached[-1]);
+		unset($post_ids_cached_backup[-2], $post_ids_cached_backup[-1]);
+		$post_ids_cached = array_unique($post_ids_cached);
+		$this->assertEquals($post_ids_cached_backup, $post_ids_cached, $this->search_backend);
+
+		// Restore this value to default
+		$sql = 'UPDATE ' . CONFIG_TABLE . "
+			SET config_value = 250
+			WHERE config_name = '" . $this->db->sql_escape('search_block_size') . "'";
+		$this->db->sql_query($sql);
+
+		// Restore posts_per_page value
+		$crawler = self::request('GET', 'adm/index.php?sid=' . $this->sid . '&i=acp_board&mode=post');
+		$form = $crawler->selectButton('Submit')->form();
+		$values = $form->getValues();
+		$values['config[posts_per_page]'] = $current_posts_per_page;
+		$form->setValues($values);
+		$crawler = self::submit($form);
+		$this->assertEquals(1, $crawler->filter('.successbox')->count(), $this->search_backend);
+	}
+
 	protected function create_search_index($backend = null)
 	{
 		$this->add_lang('acp/search');