[ticket/14285] Move response variable and small improvements

PHPBB3-14285

phpBB/phpbb/storage/controller/attachment.php

@@ -26,7 +26,9 @@ use phpbb\storage\storage;
 use phpbb\user;
 use Symfony\Component\HttpFoundation\Request as symfony_request;
 use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\ResponseHeaderBag;
+use Symfony\Component\HttpFoundation\StreamedResponse;
 
 /**
  * Controller for /download/attachment/{id} routes
@@ -85,9 +87,9 @@ class attachment extends controller
 	/**
 	 * {@inheritdoc}
 	 */
-	public function handle($id)
+	public function handle(string $file): Response
 	{
-		$attach_id = (int) $id;
+		$attach_id = (int) $file;
 		$thumbnail = $this->request->variable('t', false);
 
 		$this->language->add_lang('viewtopic');
@@ -246,13 +248,15 @@ class attachment extends controller
 		// TODO: The next lines should go better in prepare, also the mimetype is handled by the storage table
 		// so probably can be removed
 
+		$response = new StreamedResponse();
+
 		// Content-type header
-		$this->response->headers->set('Content-Type', $attachment['mimetype']);
+		$response->headers->set('Content-Type', $attachment['mimetype']);
 
 		// Display images in browser and force download for other file types
 		if (strpos($attachment['mimetype'], 'image') !== false)
 		{
-			$disposition = $this->response->headers->makeDisposition(
+			$disposition = $response->headers->makeDisposition(
 				ResponseHeaderBag::DISPOSITION_INLINE,
 				$attachment['real_filename'],
 				$this->filenameFallback($attachment['real_filename'])
@@ -260,18 +264,18 @@ class attachment extends controller
 		}
 		else
 		{
-			$disposition = $this->response->headers->makeDisposition(
+			$disposition = $response->headers->makeDisposition(
 				ResponseHeaderBag::DISPOSITION_ATTACHMENT,
 				$attachment['real_filename'],
 				$this->filenameFallback($attachment['real_filename'])
 			);
 		}
 
-		$this->response->headers->set('Content-Disposition', $disposition);
+		$response->headers->set('Content-Disposition', $disposition);
 
 		// Set expires header for browser cache
 		$time = new \Datetime();
-		$this->response->setExpires($time->modify('+1 year'));
+		$response->setExpires($time->modify('+1 year'));
 
 		return parent::handle($attachment['physical_filename']);
 	}
@@ -289,11 +293,11 @@ class attachment extends controller
 	/**
 	 * {@inheritdoc}
 	 */
-	protected function prepare($file)
+	protected function prepare(StreamedResponse $response, string $file): void
 	{
-		$this->response->setPrivate();	// But default should be private, but make sure of it
+		$response->setPrivate();	// By default should be private, but make sure of it
 
-		parent::prepare($file);
+		parent::prepare($response, $file);
 	}
 
 	/**
@@ -305,7 +309,7 @@ class attachment extends controller
 	 * @throws http_exception If attachment is not found
 	 *                        If user don't have permission to download the attachment
 	 */
-	protected function phpbb_download_handle_forum_auth($topic_id): void
+	protected function phpbb_download_handle_forum_auth(int $topic_id): void
 	{
 		$sql_array = array(
 			'SELECT'	=> 't.topic_visibility, t.forum_id, f.forum_name, f.forum_password, f.parent_id',
@@ -404,20 +408,15 @@ class attachment extends controller
 	/**
 	 * Increments the download count of all provided attachments
 	 *
-	 * @param array|int $ids The attach_id of each attachment
+	 * @param int $id The attach_id of the attachment
 	 *
 	 * @return void
 	 */
-	protected function phpbb_increment_downloads($ids): void
+	protected function phpbb_increment_downloads(int $id): void
 	{
-		if (!is_array($ids))
-		{
-			$ids = array($ids);
-		}
-
 		$sql = 'UPDATE ' . ATTACHMENTS_TABLE . '
 			SET download_count = download_count + 1
-			WHERE ' . $this->db->sql_in_set('attach_id', $ids);
+			WHERE attach_id = ' . $id;
 		$this->db->sql_query($sql);
 	}
 

phpBB/phpbb/storage/controller/avatar.php

@@ -18,7 +18,9 @@ use phpbb\config\config;
 use phpbb\db\driver\driver_interface;
 use phpbb\storage\storage;
 use Symfony\Component\HttpFoundation\Request as symfony_request;
+use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\ResponseHeaderBag;
+use Symfony\Component\HttpFoundation\StreamedResponse;
 
 /**
  * Controller for /download/avatar/{file} routes
@@ -50,7 +52,7 @@ class avatar extends controller
 	/**
 	 * {@inheritdoc}
 	 */
-	public function handle($file)
+	public function handle(string $file): Response
 	{
 		$file = $this->decode_filename($file);
 
@@ -60,22 +62,22 @@ class avatar extends controller
 	/**
 	 * {@inheritdoc}
 	 */
-	protected function is_allowed($file)
+	protected function is_allowed(string $file): bool
 	{
 		$ext = substr(strrchr($file, '.'), 1);
 
 		// If filename have point and have an allowed extension
-		return strpos($file, '.') && in_array($ext, $this->allowed_extensions);
+		return strpos($file, '.') && in_array($ext, $this->allowed_extensions, true);
 	}
 
 	/**
 	 * Decode avatar filename
 	 *
-	 * @param string		$file		Filename
+	 * @param string $file		Filename
 	 *
 	 * @return string Filename in filesystem
 	 */
-	protected function decode_filename($file)
+	protected function decode_filename(string $file): string
 	{
 		$avatar_group = false;
 
@@ -94,20 +96,20 @@ class avatar extends controller
 	/**
 	 * {@inheritdoc}
 	 */
-	protected function prepare($file)
+	protected function prepare(StreamedResponse $response, string $file): void
 	{
-		$this->response->setPublic();
+		$response->setPublic();
 
-		$disposition = $this->response->headers->makeDisposition(
+		$disposition = $response->headers->makeDisposition(
 			ResponseHeaderBag::DISPOSITION_INLINE,
 			rawurlencode($file)
 		);
 
-		$this->response->headers->set('Content-Disposition', $disposition);
+		$response->headers->set('Content-Disposition', $disposition);
 
 		$time = new \Datetime();
-		$this->response->setExpires($time->modify('+1 year'));
+		$response->setExpires($time->modify('+1 year'));
 
-		parent::prepare($file);
+		parent::prepare($response, $file);
 	}
 }

phpBB/phpbb/storage/controller/controller.php

@@ -16,8 +16,10 @@ namespace phpbb\storage\controller;
 use phpbb\cache\service;
 use phpbb\db\driver\driver_interface;
 use phpbb\exception\http_exception;
+use phpbb\storage\exception\exception;
 use phpbb\storage\storage;
 use Symfony\Component\HttpFoundation\Request as symfony_request;
+use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpFoundation\StreamedResponse;
 
 /**
@@ -34,9 +36,6 @@ class controller
 	/** @var storage */
 	protected $storage;
 
-	/** @var StreamedResponse */
-	protected $response;
-
 	/** @var symfony_request */
 	protected $symfony_request;
 
@@ -54,48 +53,50 @@ class controller
 		$this->db = $db;
 		$this->storage = $storage;
 		$this->symfony_request = $symfony_request;
-		$this->response = new StreamedResponse();
 	}
 
 	/**
 	 * Handler
 	 *
-	 * @param string		$file		File path
+	 * @param string $file		File path
+	 *
+	 * @return Response a Symfony response object
 	 *
 	 * @throws http_exception when can't access $file
-	 *
-	 * @return StreamedResponse a Symfony response object
+	 * @throws exception when there is an error reading the file
 	 */
-	public function handle($file)
+	public function handle(string $file): Response
 	{
-		if (!$this->is_allowed($file))
+		$response = new StreamedResponse();
+
+		if (!static::is_allowed($file))
 		{
 			throw new http_exception(403, 'Forbidden');
 		}
 
-		if (!$this->file_exists($file))
+		if (!static::file_exists($file))
 		{
 			throw new http_exception(404, 'Not Found');
 		}
 
-		$this->prepare($file);
+		static::prepare($response, $file);
 
 		if (headers_sent())
 		{
 			throw new http_exception(500, 'Headers already sent');
 		}
 
-		return $this->response;
+		return $response;
 	}
 
 	/**
 	 * If the user is allowed to download the file
 	 *
-	 * @param string		$file		File path
+	 * @param string $file		File path
 	 *
 	 * @return bool
 	 */
-	protected function is_allowed($file)
+	protected function is_allowed(string $file): bool
 	{
 		return true;
 	}
@@ -103,11 +104,11 @@ class controller
 	/**
 	 * Check if file exists
 	 *
-	 * @param string		$file		File path
+	 * @param string $file		File path
 	 *
 	 * @return bool
 	 */
-	protected function file_exists($file)
+	protected function file_exists(string $file): bool
 	{
 		return $this->storage->exists($file);
 	}
@@ -115,33 +116,39 @@ class controller
 	/**
 	 * Prepare response
 	 *
-	 * @param string		$file		File path
+	 * @param StreamedResponse $response
+	 * @param string $file File path
+	 *
+	 * @return void
+	 * @throws exception when there is an error reading the file
 	 */
-	protected function prepare($file)
+	protected function prepare(StreamedResponse $response, string $file): void
 	{
 		$file_info = $this->storage->file_info($file);
 
-		if (!$this->response->headers->has('Content-Type'))
+		// Add Content-Type header
+		if (!$response->headers->has('Content-Type'))
 		{
 			try
 			{
 				$content_type = $file_info->get('mimetype');
 			}
-			catch (\phpbb\storage\exception\exception $e)
+			catch (exception $e)
 			{
 				$content_type = 'application/octet-stream';
 			}
 
-			$this->response->headers->set('Content-Type', $content_type);
+			$response->headers->set('Content-Type', $content_type);
 		}
 
-		if (!$this->response->headers->has('Content-Length'))
+		// Add Content-Length header if we have the file size
+		if (!$response->headers->has('Content-Length'))
 		{
 			try
 			{
-				$this->response->headers->set('Content-Length', $file_info->get('size'));
+				$response->headers->set('Content-Length', $file_info->get('size'));
 			}
-			catch (\phpbb\storage\exception\exception $e)
+			catch (exception $e)
 			{
 				// Just don't send this header
 			}
@@ -156,7 +163,7 @@ class controller
 
 		$output = fopen('php://output', 'w+b');
 
-		$this->response->setCallback(function () use ($fp, $output) {
+		$response->setCallback(function () use ($fp, $output) {
 			stream_copy_to_stream($fp, $output);
 			fclose($fp);
 			fclose($output);
@@ -167,19 +174,15 @@ class controller
 			exit;
 		});
 
-		$this->response->isNotModified($this->symfony_request);
+		$response->isNotModified($this->symfony_request);
 	}
 
 	/**
 	* Garbage Collection
 	*/
-	protected function file_gc()
+	protected function file_gc(): void
 	{
-		if (!empty($this->cache))
-		{
-			$this->cache->unload();
-		}
-
+		$this->cache->unload(); // Equivalent to $this->cache->get_driver()->unload();
 		$this->db->sql_close();
 	}
 }