ENHANCEMENT: moved 404 page out to a separate template and altered DocumentationViewer to throw 404s on pages which do not resolve to anything. Fixes: #6616

code/DocumentationService.php

@@ -335,9 +335,12 @@ class DocumentationService {
 
 			if($modules) {
 				foreach($modules as $key => $module) {
-					if(is_dir(BASE_PATH .'/'. $module) && !in_array($module, self::get_ignored_files(), true)) {
+					$dir = is_dir(Controller::join_links(BASE_PATH, $module));
+					$ignored = in_array($module, self::get_ignored_files(), true);
+					
+					if($dir && !$ignored) {
 						// check to see if it has docs
-						$docs = BASE_PATH .'/'. $module .'/docs/';
+						$docs = Controller::join_links($dir, 'docs');
 	
 						if(is_dir($docs)) {
 							self::register($module, $docs, '', $module, true);
@@ -355,7 +358,13 @@ class DocumentationService {
 	 * @param String $code code
 	 */
 	public static function get_language_title($lang) {
-		return (isset(self::$language_mapping[$lang])) ? _t("DOCUMENTATIONSERVICE.LANG-$lang", self::$language_mapping[$lang]) : $lang;
+		$map = self::$language_mapping;
+		
+		if(isset($map[$lang])) { 
+			return _t("DOCUMENTATIONSERVICE.LANG-$lang", $map[$lang]);
+		}
+		
+		return $lang;
 	}
 	
 	

code/controllers/DocumentationViewer.php

@@ -48,7 +48,7 @@ class DocumentationViewer extends Controller {
 	protected static $link_base = 'dev/docs/';
 	
 	/**
-	 * @var String|array Optional permssion check
+	 * @var String|array Optional permission check
 	 */
 	static $check_permission = 'ADMIN';
 	
@@ -59,12 +59,8 @@ class DocumentationViewer extends Controller {
 
 		// javascript
 		Requirements::javascript(THIRDPARTY_DIR .'/jquery/jquery.js');
-		Requirements::javascript('sapphiredocs/thirdparty/syntaxhighlighter/scripts/shCore.js');
-		Requirements::javascript('sapphiredocs/thirdparty/syntaxhighlighter/scripts/shBrushJScript.js');
-		Requirements::javascript('sapphiredocs/thirdparty/syntaxhighlighter/scripts/shBrushPhp.js');
-		Requirements::javascript('sapphiredocs/thirdparty/syntaxhighlighter/scripts/shBrushXml.js');
-		Requirements::javascript('sapphiredocs/thirdparty/syntaxhighlighter/scripts/shBrushCss.js');
 		Requirements::javascript('sapphiredocs/javascript/shBrushSS.js');
+		
 		Requirements::combine_files(
 			'syntaxhighlighter.js',
 			array(
@@ -97,11 +93,11 @@ class DocumentationViewer extends Controller {
 	}
 
 	/**
-	 * Overloaded to avoid "action doesnt exist" errors - all URL parts in this
+	 * Overloaded to avoid "action doesn't exist" errors - all URL parts in this
 	 * controller are virtual and handled through handleRequest(), not controller methods.
 	 */
 	public function handleAction($request) {
-		try{
+		try {
 			$response = parent::handleAction($request);
 		} catch(SS_HTTPResponse_Exception $e) {
 			if(strpos($e->getMessage(), 'does not exist') !== FALSE) {
@@ -141,6 +137,7 @@ class DocumentationViewer extends Controller {
 			// allow assets
 			if($firstParam == "assets") return parent::handleRequest($request);
 			
+			// check for permalinks
 			if($link = DocumentationPermalinks::map($firstParam)) {
 				// the first param is a shortcode for a page so redirect the user to
 				// the short code.
@@ -151,8 +148,14 @@ class DocumentationViewer extends Controller {
 				
 			}
 
+			// check to see if the module is a valid module. If it isn't, then we
+			// need to throw a 404.
+			if(!DocumentationService::is_registered_module($firstParam)) {
+				return $this->throw404();
+			}
+			
 			$this->module = $firstParam;
-			$this->lang = $secondParam;
+			$this->language = $secondParam;
 			
 			if(isset($thirdParam) && (is_numeric($thirdParam) || in_array($thirdParam, array('master', 'trunk')))) {
 				$this->version = $thirdParam;	
@@ -166,27 +169,43 @@ class DocumentationViewer extends Controller {
 		}
 		
 		// 'current' version mapping
-		$module = DocumentationService::is_registered_module($this->module, null, $this->getLang());
+		$entity = DocumentationService::is_registered_module($this->module, null, $this->getLang());
 
-		if($module) {
+		if($entity) {
-			$current = $module->getLatestVersion();
+			$current = $entity->getLatestVersion();
 			$version = $this->getVersion();
 			
-			if(!$version || $version == '') {
+			if(!$version) {
 				$this->version = $current;
 			}
 			
 			// Check if page exists, otherwise return 404
 			if(!$this->locationExists()) {
-				$body = $this->renderWith(get_class($this));
+				return $this->throw404();
+			}
+			
+			
+			return parent::handleRequest($request);
+		}
+		
+		return $this->throw404();
+	}
+		
+	
+	/**
+	 * Helper function for throwing a 404 error from the {@link handleRequest}
+	 * method.
+	 *
+	 * @return HttpResponse
+	 */
+	function throw404() {
+		$class = get_class($this);
+		
+		$body = $this->renderWith(array("{$class}_error", $class));
 		$this->response = new SS_HTTPResponse($body, 404);
 		
 		return $this->response;
 	}
-		}
-
-		return parent::handleRequest($request);
-	}
 	
 	/**
 	 * Custom templates for each of the sections. 
@@ -404,6 +423,7 @@ class DocumentationViewer extends Controller {
 		$module = $this->getModule();
 		
 		if($module) {
+			
 			$has_dir = is_dir(Controller::join_links(
 				$module->getPath($this->getVersion(), $this->getLang()), 
 				implode('/', $this->Remaining)
@@ -567,7 +587,7 @@ class DocumentationViewer extends Controller {
 		if($page) {
 			return DBField::create("HTMLText", $page->getHTML($this->getVersion(), $this->getLang()));
 		}
-		else {
+		
 		// If no page found then we may want to get the listing of the folder.
 		// In case no folder exists, show a "not found" page.
 		$module = $this->getModule();
@@ -582,21 +602,19 @@ class DocumentationViewer extends Controller {
 				$this->getLang()
 			);
 			
-				// If no pages are found, the 404 is handled in the same template
 			return $this->customise(array(
+				'Content' => false,
 				'Title' => DocumentationService::clean_page_name(array_pop($url)),
 				'Pages' => $pages
 			))->renderWith('DocFolderListing');
 		}
 		else {
-				// get all available modules and show a table of contents.
-				
 			return $this->customise(array(
+				'Content' => false,
 				'Title' => _t('DocumentationViewer.MODULES', 'Modules'),
 				'Pages' => $this->getModules()
 			))->renderWith('DocFolderListing');
 		}
-		}
 	
 		return false;
 	}

templates/DocFolderListing.ss

@@ -1,13 +1,13 @@
-<% if Pages %>
+<div id="folder-listing">
-	<div id="folder-listing">
 	<h2>$Title</h2>
 	
+	<% if Pages %>
 		<ul>
 			<% control Pages %>
 				<li><a href="$Link">$Title</a></li>
 			<% end_control %>
 		</ul>
-	</div>
+	<% else %>
-<% else %>
+		<p>No documentation pages found for $Title. If you need help writing documentation please consult the README.</p>
-	<% include DocNotFound %>
+	<% end_if %>
-<% end_if %>
+</div>

templates/Includes/DocNotFound.ss

@@ -1,24 +0,0 @@
-<div class="warningBox" id="pageNotFoundWarning">
-	<div class="warningBoxTop">
-		<h1>We're sorry&#8230;</h1>
-		<p>The page you are looking for does not exist, or might have moved.
-		<h5>Perhaps you could&#8230;</h5>
-		<p>Return to the <a href="$BaseHref">homepage</a> or try searching for the page (see input box on the top right).</p>
-
-		<!-- 
-			<h5>Search results for similar/related phrases</h5>
-			<ul class="resetStyle">
-				<li><a href="#">Installing</a></li>
-				<li><a href="#">Upgrading</a></li>
-				<li><a href="#">Server-requirements</a></li>
-				<li><a href="#">Suggested-web-hosts</a></li>
-			</ul> 
-		-->
-		</div>
-		<!-- <div class="warningBoxBottom">
-			<ul>
-				<li><a href="#">Report a missing link</a></li>
-				<li><a href="#">Report a bug or a concren</a></li>
-			</ul>
-	</div> -->
-</div>

templates/Layout/DocumentationViewer.ss

@@ -1,10 +1,6 @@
 <div id="documentation-page">
 	<div id="left-column">
-		<% if Content %>
 		$Content
-		<% else %>
-			<% include DocNotFound %>
-		<% end_if %>
 	</div>
 
 	<% if Content %>

templates/Layout/DocumentationViewer_error.ss

@@ -0,0 +1,13 @@
+<div id="documentation-page" class="documentation-error-page">
+	<div class="warningBox" id="pageNotFoundWarning">
+		<div class="warningBoxTop">
+			<h1>We're sorry&#8230;</h1>
+			<p>The page you are looking for does not exist, or might have moved.
+			<h5>Perhaps you could&#8230;</h5>
+			<ul>
+				<li>Return to the <a href="$BaseHref">homepage</a></li> 
+				<li>Try searching for the page (see input box on the top right).</li>
+			</ul>
+		</div>
+	</div>
+</div>

tests/DocumentationViewerTest.php

@@ -52,7 +52,6 @@ class DocumentationViewerTest extends FunctionalTest {
 		$response = $this->get('dev/docs/DocumentationViewerTests/en/2.4/');		
 		$this->assertEquals($response->getStatusCode(), 200, 'Existing base folder');		
 		
-		
 		$response = $this->get('dev/docs/DocumentationViewerTests/en/2.3/nonexistant-subfolder');
 		$this->assertEquals($response->getStatusCode(), 404, 'Nonexistant subfolder');
 		
@@ -79,8 +78,13 @@ class DocumentationViewerTest extends FunctionalTest {
 		
 		$response = $this->get('dev/docs/DocumentationViewerTests/en/3.0/test/');
 		$this->assertEquals($response->getStatusCode(), 404, 'Missing page');
-	}
 		
+		$response = $this->get('dev/docs/en');
+		$this->assertEquals($response->getStatusCode(), 404, 'Must include a module');
+		
+		$response = $this->get('dev/docs/DocumentationViewerTests/dk/');;
+		$this->assertEquals($response->getStatusCode(), 404, 'Access a language that doesn\'t exist');
+	}
 	
 	function testRouting() {
 		$response = $this->get('dev/docs/DocumentationViewerTests/en/2.4');