Merge pull request #14689 from owncloud/better-missing-resource-handling

Log errors and create 404 in network list when a css or js is missing

lib/private/template/cssresourcelocator.php

@@ -9,7 +9,10 @@
 namespace OC\Template;
 
 class CSSResourceLocator extends ResourceLocator {
-	public function doFind( $style ) {
+	/**
+	 * @param string $style
+	 */
+	public function doFind($style) {
 		if (strpos($style, '3rdparty') === 0
 			&& $this->appendIfExist($this->thirdpartyroot, $style.'.css')
 			|| $this->appendIfExist($this->serverroot, $style.'.css')
@@ -21,14 +24,13 @@ class CSSResourceLocator extends ResourceLocator {
 		$style = substr($style, strpos($style, '/')+1);
 		$app_path = \OC_App::getAppPath($app);
 		$app_url = \OC_App::getAppWebPath($app);
-		if ($this->appendIfExist($app_path, $style.'.css', $app_url)
-		) {
-			return;
-		}
-		throw new \Exception('css file not found: style:'.$style);
+		$this->append($app_path, $style.'.css', $app_url);
 	}
 
-	public function doFindTheme( $style ) {
+	/**
+	 * @param string $style
+	 */
+	public function doFindTheme($style) {
 		$theme_dir = 'themes/'.$this->theme.'/';
 		$this->appendIfExist($this->serverroot, $theme_dir.'apps/'.$style.'.css')
 			|| $this->appendIfExist($this->serverroot, $theme_dir.$style.'.css')

lib/private/template/jsresourcelocator.php

@@ -9,7 +9,10 @@
 namespace OC\Template;
 
 class JSResourceLocator extends ResourceLocator {
-	public function doFind( $script ) {
+	/**
+	 * @param string $script
+	 */
+	public function doFind($script) {
 		$theme_dir = 'themes/'.$this->theme.'/';
 		if (strpos($script, '3rdparty') === 0
 			&& $this->appendIfExist($this->thirdpartyroot, $script.'.js')
@@ -25,16 +28,18 @@ class JSResourceLocator extends ResourceLocator {
 		$script = substr($script, strpos($script, '/')+1);
 		$app_path = \OC_App::getAppPath($app);
 		$app_url = \OC_App::getAppWebPath($app);
-		if ($this->appendIfExist($app_path, $script.'.js', $app_url)) {
-			return;
-		}
+
 		// missing translations files fill be ignored
-		if (strpos($script, "l10n/") === 0) {
+		if (strpos($script, 'l10n/') === 0) {
+			$this->appendIfExist($app_path, $script . '.js', $app_url);
 			return;
 		}
-		throw new \Exception('js file not found: script:'.$script);
+		$this->append($app_path, $script . '.js', $app_url);
 	}
 
-	public function doFindTheme( $script ) {
+	/**
+	 * @param string $script
+	 */
+	public function doFindTheme($script) {
 	}
 }

lib/private/template/resourcelocator.php

@@ -18,10 +18,17 @@ abstract class ResourceLocator {
 
 	protected $resources = array();
 
+	/** @var \OCP\ILogger */
+	protected $logger;
+
 	/**
+	 * @param \OCP\ILogger $logger
 	 * @param string $theme
+	 * @param array $core_map
+	 * @param array $party_map
 	 */
-	public function __construct( $theme, $core_map, $party_map ) {
+	public function __construct(\OCP\ILogger $logger, $theme, $core_map, $party_map) {
+		$this->logger = $logger;
 		$this->theme = $theme;
 		$this->mapping = $core_map + $party_map;
 		$this->serverroot = key($core_map);
@@ -29,41 +36,82 @@ abstract class ResourceLocator {
 		$this->webroot = $this->mapping[$this->serverroot];
 	}
 
-	abstract public function doFind( $resource );
-	abstract public function doFindTheme( $resource );
+	/**
+	 * @param string $resource
+	 */
+	abstract public function doFind($resource);
 
-	public function find( $resources ) {
-		try {
-			foreach($resources as $resource) {
+	/**
+	 * @param string $resource
+	 */
+	abstract public function doFindTheme($resource);
+
+	/**
+	 * Finds the resources and adds them to the list
+	 *
+	 * @param array $resources
+	 */
+	public function find($resources) {
+		foreach ($resources as $resource) {
+			try {
 				$this->doFind($resource);
+			} catch (ResourceNotFoundException $e) {
+				$resourceApp = substr($resource, 0, strpos($resource, '/'));
+				$this->logger->error('Could not find resource file "' . $e->getResourcePath() . '"', ['app' => $resourceApp]);
 			}
-			if (!empty($this->theme)) {
-				foreach($resources as $resource) {
+		}
+		if (!empty($this->theme)) {
+			foreach ($resources as $resource) {
+				try {
 					$this->doFindTheme($resource);
+				} catch (ResourceNotFoundException $e) {
+					$resourceApp = substr($resource, 0, strpos($resource, '/'));
+					$this->logger->error('Could not find resource file "' . $e->getResourcePath() . '"', ['app' => $resourceApp]);
 				}
 			}
-		} catch (\Exception $e) {
-			throw new \Exception($e->getMessage().' serverroot:'.$this->serverroot);
 		}
 	}
 
-	/*
+	/**
 	 * append the $file resource if exist at $root
+	 *
 	 * @param string $root path to check
 	 * @param string $file the filename
-	 * @param string|null $webroot base for path, default map $root to $webroot
+	 * @param string|null $webRoot base for path, default map $root to $webRoot
+	 * @return bool True if the resource was found, false otherwise
 	 */
-	protected function appendIfExist($root, $file, $webroot = null) {
+	protected function appendIfExist($root, $file, $webRoot = null) {
 		if (is_file($root.'/'.$file)) {
-			if (!$webroot) {
-				$webroot = $this->mapping[$root];
-			}
-			$this->resources[] = array($root, $webroot, $file);
+			$this->append($root, $file, $webRoot, false);
 			return true;
 		}
 		return false;
 	}
 
+	/**
+	 * append the $file resource at $root
+	 *
+	 * @param string $root path to check
+	 * @param string $file the filename
+	 * @param string|null $webRoot base for path, default map $root to $webRoot
+	 * @param bool $throw Throw an exception, when the route does not exist
+	 * @throws ResourceNotFoundException Only thrown when $throw is true and the resource is missing
+	 */
+	protected function append($root, $file, $webRoot = null, $throw = true) {
+		if (!$webRoot) {
+			$webRoot = $this->mapping[$root];
+		}
+		$this->resources[] = array($root, $webRoot, $file);
+
+		if ($throw && !is_file($root . '/' . $file)) {
+			throw new ResourceNotFoundException($file, $webRoot);
+		}
+	}
+
+	/**
+	 * Returns the list of all resources that should be loaded
+	 * @return array
+	 */
 	public function getResources() {
 		return $this->resources;
 	}

lib/private/template/resourcenotfoundexception.php

@@ -0,0 +1,35 @@
+<?php
+/**
+ * ownCloud
+ *
+ * @author Joas Schilling
+ * @copyright 2015 Joas Schilling nickvergessen@owncloud.com
+ *
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace OC\Template;
+
+class ResourceNotFoundException extends \LogicException {
+	protected $resource;
+	protected $webPath;
+
+	/**
+	 * @param string $resource
+	 * @param string $webPath
+	 */
+	public function __construct($resource, $webPath) {
+		parent::__construct('Resource not found');
+		$this->resource = $resource;
+		$this->webPath = $webPath;
+	}
+
+	/**
+	 * @return string
+	 */
+	public function getResourcePath() {
+		return $this->resource . '/' . $this->webPath;
+	}
+}

lib/private/templatelayout.php

@@ -135,7 +135,9 @@ class OC_TemplateLayout extends OC_Template {
 		// Read the selected theme from the config file
 		$theme = OC_Util::getTheme();
 
-		$locator = new \OC\Template\CSSResourceLocator( $theme,
+		$locator = new \OC\Template\CSSResourceLocator(
+			OC::$server->getLogger(),
+			$theme,
 			array( OC::$SERVERROOT => OC::$WEBROOT ),
 			array( OC::$THIRDPARTYROOT => OC::$THIRDPARTYWEBROOT ));
 		$locator->find($styles);
@@ -150,7 +152,9 @@ class OC_TemplateLayout extends OC_Template {
 		// Read the selected theme from the config file
 		$theme = OC_Util::getTheme();
 
-		$locator = new \OC\Template\JSResourceLocator( $theme,
+		$locator = new \OC\Template\JSResourceLocator(
+			OC::$server->getLogger(),
+			$theme,
 			array( OC::$SERVERROOT => OC::$WEBROOT ),
 			array( OC::$THIRDPARTYROOT => OC::$THIRDPARTYWEBROOT ));
 		$locator->find($scripts);

tests/lib/template/resourcelocator.php

@@ -7,13 +7,20 @@
  */
 
 class Test_ResourceLocator extends \Test\TestCase {
+	/** @var PHPUnit_Framework_MockObject_MockObject */
+	protected $logger;
+
+	protected function setUp() {
+		parent::setUp();
+		$this->logger = $this->getMock('OCP\ILogger');
+	}
 
 	/**
 	 * @param string $theme
 	 */
 	public function getResourceLocator( $theme, $core_map, $party_map, $appsroots ) {
 		return $this->getMockForAbstractClass('OC\Template\ResourceLocator',
-			array( $theme, $core_map, $party_map, $appsroots ),
+			array($this->logger, $theme, $core_map, $party_map, $appsroots ),
 			'', true, true, true, array());
 	}
 
@@ -30,7 +37,7 @@ class Test_ResourceLocator extends \Test\TestCase {
 
 	public function testFind() {
 		$locator = $this->getResourceLocator('theme',
-			array('core'=>'map'), array('3rd'=>'party'), array('foo'=>'bar'));
+			array('core' => 'map'), array('3rd' => 'party'), array('foo' => 'bar'));
 		$locator->expects($this->once())
 			->method('doFind')
 			->with('foo');
@@ -38,18 +45,22 @@ class Test_ResourceLocator extends \Test\TestCase {
 			->method('doFindTheme')
 			->with('foo');
 		$locator->find(array('foo'));
+	}
 
+	public function testFindNotFound() {
 		$locator = $this->getResourceLocator('theme',
 			array('core'=>'map'), array('3rd'=>'party'), array('foo'=>'bar'));
 		$locator->expects($this->once())
 			->method('doFind')
 			->with('foo')
-			->will($this->throwException(new Exception('test')));
-		try {
-			$locator->find(array('foo'));
-		} catch (\Exception $e) {
-			$this->assertEquals('test serverroot:core', $e->getMessage());
-		}
+			->will($this->throwException(new \OC\Template\ResourceNotFoundException('foo', 'map')));
+		$locator->expects($this->once())
+			->method('doFindTheme')
+			->with('foo')
+			->will($this->throwException(new \OC\Template\ResourceNotFoundException('foo', 'map')));
+		$this->logger->expects($this->exactly(2))
+			->method('error');
+		$locator->find(array('foo'));
 	}
 
 	public function testAppendIfExist() {