fix(previews): Cleanup Movie provider code and remove deprecated static varsfix/fix-movie-preview-construct

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

3 files changed, 47 insertions, 52 deletions

diff --git a/lib/private/Preview/Movie.php b/lib/private/Preview/Movie.php
index 9b5510ca72b..46462dfa592 100644
--- a/lib/private/Preview/Movie.php
+++ b/lib/private/Preview/Movie.php
@@ -5,32 +5,21 @@
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OC\Preview;
 
 use OCP\Files\File;
 use OCP\Files\FileInfo;
 use OCP\IConfig;
 use OCP\IImage;
+use OCP\ITempManager;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
 
 class Movie extends ProviderV2 {
 	private IConfig $config;
 
-	/**
-	 * @deprecated 23.0.0 pass option to \OCP\Preview\ProviderV2
-	 * @var string
-	 */
-	public static $avconvBinary;
-
-	/**
-	 * @deprecated 23.0.0 pass option to \OCP\Preview\ProviderV2
-	 * @var string
-	 */
-	public static $ffmpegBinary;
-
-	/** @var string */
-	private $binary;
+	private ?string $binary = null;
 
 	public function __construct(array $options = []) {
 		parent::__construct($options);
@@ -45,14 +34,9 @@ class Movie extends ProviderV2 {
 	 * {@inheritDoc}
 	 */
 	public function isAvailable(FileInfo $file): bool {
-		// TODO: remove when avconv is dropped
 		if (is_null($this->binary)) {
 			if (isset($this->options['movieBinary'])) {
 				$this->binary = $this->options['movieBinary'];
-			} elseif (is_string(self::$avconvBinary)) {
-				$this->binary = self::$avconvBinary;
-			} elseif (is_string(self::$ffmpegBinary)) {
-				$this->binary = self::$ffmpegBinary;
 			}
 		}
 		return is_string($this->binary);
@@ -89,14 +73,11 @@ class Movie extends ProviderV2 {
 				return null;
 			}
 
-			$result = null;
-			if (is_string($absPath)) {
-				$result = $this->generateThumbNail($maxX, $maxY, $absPath, 5);
+			$result = $this->generateThumbNail($maxX, $maxY, $absPath, 5);
+			if ($result === null) {
+				$result = $this->generateThumbNail($maxX, $maxY, $absPath, 1);
 				if ($result === null) {
-					$result = $this->generateThumbNail($maxX, $maxY, $absPath, 1);
-					if ($result === null) {
-						$result = $this->generateThumbNail($maxX, $maxY, $absPath, 0);
-					}
+					$result = $this->generateThumbNail($maxX, $maxY, $absPath, 0);
 				}
 			}
 
@@ -137,7 +118,15 @@ class Movie extends ProviderV2 {
 	}
 
 	private function generateThumbNail(int $maxX, int $maxY, string $absPath, int $second): ?IImage {
-		$tmpPath = \OC::$server->getTempManager()->getTemporaryFile();
+		$tmpPath = Server::get(ITempManager::class)->getTemporaryFile();
+
+		if ($tmpPath === false) {
+			Server::get(LoggerInterface::class)->error(
+				'Failed to get local file to generate thumbnail for: ' . $absPath,
+				['app' => 'core']
+			);
+			return null;
+		}
 
 		$binaryType = substr(strrchr($this->binary, '/'), 1);
 
@@ -190,7 +179,7 @@ class Movie extends ProviderV2 {
 		}
 
 		if ($second === 0) {
-			$logger = \OC::$server->get(LoggerInterface::class);
+			$logger = Server::get(LoggerInterface::class);
 			$logger->info('Movie preview generation failed Output: {output}', ['app' => 'core', 'output' => $output]);
 		}
 
diff --git a/lib/private/Preview/ProviderV2.php b/lib/private/Preview/ProviderV2.php
index 7251dd70d17..556d1099d2d 100644
--- a/lib/private/Preview/ProviderV2.php
+++ b/lib/private/Preview/ProviderV2.php
@@ -6,27 +6,23 @@ declare(strict_types=1);
  * SPDX-FileCopyrightText: 2019 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
+
 namespace OC\Preview;
 
 use OCP\Files\File;
 use OCP\Files\FileInfo;
 use OCP\IImage;
+use OCP\ITempManager;
 use OCP\Preview\IProviderV2;
+use OCP\Server;
+use Psr\Log\LoggerInterface;
 
 abstract class ProviderV2 implements IProviderV2 {
-	/** @var array */
-	protected $options;
-
-	/** @var array */
-	protected $tmpFiles = [];
+	protected array $tmpFiles = [];
 
-	/**
-	 * Constructor
-	 *
-	 * @param array $options
-	 */
-	public function __construct(array $options = []) {
-		$this->options = $options;
+	public function __construct(
+		protected array $options = [],
+	) {
 	}
 
 	/**
@@ -50,7 +46,7 @@ abstract class ProviderV2 implements IProviderV2 {
 	 * @param File $file
 	 * @param int $maxX The maximum X size of the thumbnail. It can be smaller depending on the shape of the image
 	 * @param int $maxY The maximum Y size of the thumbnail. It can be smaller depending on the shape of the image
-	 * @return null|\OCP\IImage false if no preview was generated
+	 * @return null|\OCP\IImage null if no preview was generated
 	 * @since 17.0.0
 	 */
 	abstract public function getThumbnail(File $file, int $maxX, int $maxY): ?IImage;
@@ -63,12 +59,19 @@ abstract class ProviderV2 implements IProviderV2 {
 	 * Get a path to either the local file or temporary file
 	 *
 	 * @param File $file
-	 * @param int $maxSize maximum size for temporary files
-	 * @return string|false
+	 * @param ?int $maxSize maximum size for temporary files
 	 */
-	protected function getLocalFile(File $file, ?int $maxSize = null) {
+	protected function getLocalFile(File $file, ?int $maxSize = null): string|false {
 		if ($this->useTempFile($file)) {
-			$absPath = \OC::$server->getTempManager()->getTemporaryFile();
+			$absPath = Server::get(ITempManager::class)->getTemporaryFile();
+
+			if ($absPath === false) {
+				Server::get(LoggerInterface::class)->error(
+					'Failed to get local file to generate thumbnail for: ' . $file->getPath(),
+					['app' => 'core']
+				);
+				return false;
+			}
 
 			$content = $file->fopen('r');
 			if ($content === false) {
diff --git a/tests/lib/Preview/MovieTest.php b/tests/lib/Preview/MovieTest.php
index d41b242945b..b8946ad4c5e 100644
--- a/tests/lib/Preview/MovieTest.php
+++ b/tests/lib/Preview/MovieTest.php
@@ -7,6 +7,9 @@
 
 namespace Test\Preview;
 
+use OCP\IBinaryFinder;
+use OCP\Server;
+
 /**
  * Class MovieTest
  *
@@ -16,20 +19,20 @@ namespace Test\Preview;
  */
 class MovieTest extends Provider {
 	protected function setUp(): void {
-		$avconvBinary = \OC_Helper::findBinaryPath('avconv');
-		$ffmpegBinary = ($avconvBinary) ? null : \OC_Helper::findBinaryPath('ffmpeg');
+		$binaryFinder = Server::get(IBinaryFinder::class);
+		$movieBinary = $binaryFinder->findBinaryPath('avconv');
+		if (!is_string($movieBinary)) {
+			$movieBinary = $binaryFinder->findBinaryPath('ffmpeg');
+		}
 
-		if ($avconvBinary || $ffmpegBinary) {
+		if (is_string($movieBinary)) {
 			parent::setUp();
 
-			\OC\Preview\Movie::$avconvBinary = $avconvBinary;
-			\OC\Preview\Movie::$ffmpegBinary = $ffmpegBinary;
-
 			$fileName = 'testimage.mp4';
 			$this->imgPath = $this->prepareTestFile($fileName, \OC::$SERVERROOT . '/tests/data/' . $fileName);
 			$this->width = 560;
 			$this->height = 320;
-			$this->provider = new \OC\Preview\Movie;
+			$this->provider = new \OC\Preview\Movie(['movieBinary' => $movieBinary]);
 		} else {
 			$this->markTestSkipped('No Movie provider present');
 		}