Fix up internal 404s and consolidate via trait

docs/Error/ErrorHandler.md

@@ -63,7 +63,12 @@ class Application extends BaseApplication {
     }
 ```
 
-### Tips
-You should also set up a monitor to check for internally caused 404s (referrer is a page on the own site) and alert (via email or alike).
+Note that internally caused 404s (referrer is a page on the own site) are not transferred into the 404 log.
 In that case you are having invalid links in your pages somewhere, which should be fixed.
+So those are considered actual errors here.
+
+### Tips
+
+You can also set up a monitor to check for the internal 404s and alert (via email or alike).
+Those should also be fixed rather soon because more and more people click on those and end up on the error page.
 All other 404s are caused from the outside (often times crawlers and bots) and are usually not too relevant.

src/Error/ErrorHandler.php

@@ -5,7 +5,6 @@ namespace Tools\Error;
 use Cake\Error\ErrorHandler as CoreErrorHandler;
 use Cake\Log\Log;
 use Exception;
-use Tools\Error\Middleware\ErrorHandlerMiddleware;
 
 /**
  * Custom ErrorHandler to not mix the 404 exceptions with the rest of "real" errors in the error.log file.
@@ -31,6 +30,8 @@ use Tools\Error\Middleware\ErrorHandlerMiddleware;
  */
 class ErrorHandler extends CoreErrorHandler {
 
+	use ErrorHandlerTrait;
+
 	/**
 	 * Handles exception logging
 	 *
@@ -38,11 +39,7 @@ class ErrorHandler extends CoreErrorHandler {
 	 * @return bool
 	 */
 	protected function _logException(Exception $exception) {
-		$blacklist = ErrorHandlerMiddleware::$blacklist;
-		if (isset($this->_options['log404'])) {
-			$blacklist = $this->_options['log404'];
-		}
-		if ($blacklist && in_array(get_class($exception), (array)$blacklist)) {
+		if ($this->is404($exception)) {
 			$level = LOG_ERR;
 			Log::write($level, $this->_getMessage($exception), ['404']);
 			return false;

src/Error/ErrorHandlerTrait.php

@@ -0,0 +1,97 @@
+<?php
+
+namespace Tools\Error;
+
+use Cake\Controller\Exception\MissingActionException;
+use Cake\Core\Configure;
+use Cake\Datasource\Exception\InvalidPrimaryKeyException;
+use Cake\Datasource\Exception\RecordNotFoundException;
+use Cake\Network\Exception\BadRequestException;
+use Cake\Network\Exception\ConflictException;
+use Cake\Network\Exception\GoneException;
+use Cake\Network\Exception\InvalidCsrfTokenException;
+use Cake\Network\Exception\MethodNotAllowedException;
+use Cake\Network\Exception\NotAcceptableException;
+use Cake\Network\Exception\NotFoundException;
+use Cake\Network\Exception\UnauthorizedException;
+use Cake\Network\Exception\UnavailableForLegalReasonsException;
+use Cake\Routing\Exception\MissingControllerException;
+use Cake\Routing\Exception\MissingRouteException;
+use Cake\View\Exception\MissingViewException;
+
+/**
+ * @property array $_options
+ */
+trait ErrorHandlerTrait {
+
+	/**
+	 * @var array
+	 */
+	protected static $blacklist = [
+		InvalidPrimaryKeyException::class,
+		NotFoundException::class,
+		MethodNotAllowedException::class,
+		NotAcceptableException::class,
+		RecordNotFoundException::class,
+		BadRequestException::class,
+		GoneException::class,
+		ConflictException::class,
+		InvalidCsrfTokenException::class,
+		UnauthorizedException::class,
+		MissingControllerException::class,
+		MissingActionException::class,
+		MissingRouteException::class,
+		MissingViewException::class,
+		UnavailableForLegalReasonsException::class,
+	];
+
+	/**
+	 * @param \Exception $exception
+	 * @param \Psr\Http\Message\ServerRequestInterface|null $request
+	 * @return bool
+	 */
+	protected function is404($exception, $request = null) {
+		$blacklist = static::$blacklist;
+		if (isset($this->_options['log404'])) {
+			$blacklist = $this->_options['log404'];
+		}
+		if (!$blacklist) {
+			return false;
+		}
+
+		$class = get_class($exception);
+		if (!$this->isBlacklisted($class, (array)$blacklist)) {
+			return false;
+		}
+
+		$referer = $request->getHeaderLine('Referer');
+		$baseUrl = Configure::read('App.fullBaseUrl');
+		if (strpos($referer, $baseUrl) === 0) {
+			return false;
+		}
+
+		return true;
+	}
+
+	/**
+	 * @param string $class
+	 * @param array $blacklist
+	 * @return bool
+	 */
+	protected function isBlacklisted($class, array $blacklist) {
+		// Quick string comparison first
+		if (in_array($class, $blacklist, true)) {
+			return true;
+		}
+
+		// Deep instance of checking
+		foreach ($blacklist as $blacklistedClass) {
+			if ($class instanceof $blacklistedClass) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+}

src/Error/Middleware/ErrorHandlerMiddleware.php

@@ -2,23 +2,9 @@
 
 namespace Tools\Error\Middleware;
 
-use Cake\Controller\Exception\MissingActionException;
-use Cake\Datasource\Exception\InvalidPrimaryKeyException;
-use Cake\Datasource\Exception\RecordNotFoundException;
 use Cake\Error\Middleware\ErrorHandlerMiddleware as CoreErrorHandlerMiddleware;
 use Cake\Log\Log;
-use Cake\Network\Exception\BadRequestException;
-use Cake\Network\Exception\ConflictException;
-use Cake\Network\Exception\GoneException;
-use Cake\Network\Exception\InvalidCsrfTokenException;
-use Cake\Network\Exception\MethodNotAllowedException;
-use Cake\Network\Exception\NotAcceptableException;
-use Cake\Network\Exception\NotFoundException;
-use Cake\Network\Exception\UnauthorizedException;
-use Cake\Network\Exception\UnavailableForLegalReasonsException;
-use Cake\Routing\Exception\MissingControllerException;
-use Cake\Routing\Exception\MissingRouteException;
-use Cake\View\Exception\MissingViewException;
+use Tools\Error\ErrorHandlerTrait;
 
 /**
  * Error handling middleware.
@@ -44,26 +30,7 @@ use Cake\View\Exception\MissingViewException;
  */
 class ErrorHandlerMiddleware extends CoreErrorHandlerMiddleware {
 
-	/**
-	 * @var array
-	 */
-	public static $blacklist = [
-		InvalidPrimaryKeyException::class,
-		NotFoundException::class,
-		MethodNotAllowedException::class,
-		NotAcceptableException::class,
-		RecordNotFoundException::class,
-		BadRequestException::class,
-		GoneException::class,
-		ConflictException::class,
-		InvalidCsrfTokenException::class,
-		UnauthorizedException::class,
-		MissingControllerException::class,
-		MissingActionException::class,
-		MissingRouteException::class,
-		MissingViewException::class,
-		UnavailableForLegalReasonsException::class,
-	];
+	use ErrorHandlerTrait;
 
 	/**
 	 * @param string|callable|null $renderer The renderer or class name
@@ -83,11 +50,7 @@ class ErrorHandlerMiddleware extends CoreErrorHandlerMiddleware {
 	 * @return void
 	 */
 	protected function logException($request, $exception) {
-		$blacklist = static::$blacklist;
-		if (isset($this->_config['log404'])) {
-			$blacklist = $this->_config['log404'];
-		}
-		if ($blacklist && in_array(get_class($exception), (array)$blacklist)) {
+		if ($this->is404($exception, $request)) {
 			$level = LOG_ERR;
 			Log::write($level, $this->getMessage($request, $exception), ['404']);
 			return;

tests/TestCase/Controller/ControllerTest.php

@@ -7,8 +7,6 @@ use Cake\ORM\TableRegistry;
 use Tools\Controller\Controller;
 use Tools\TestSuite\TestCase;
 
-/**
- */
 class ControllerTest extends TestCase {
 
 	/**

tests/TestCase/Error/Middleware/ErrorHandlerMiddlewareTest.php

@@ -0,0 +1,63 @@
+<?php
+
+namespace Tools\Test\TestCase\Error\Middleware;
+
+use Cake\Core\Configure;
+use Cake\Network\Exception\NotFoundException;
+use Cake\Network\Http\Request;
+use Tools\Error\Middleware\ErrorHandlerMiddleware;
+use Tools\TestSuite\TestCase;
+use Tools\TestSuite\ToolsTestTrait;
+
+class ErrorHandlerMiddlewareTest extends TestCase {
+
+	use ToolsTestTrait;
+
+	/**
+	 * @var \Tools\Error\Middleware\ErrorHandlerMiddleware
+	 */
+	protected $errorHandlerMiddleware;
+
+	/**
+	 * @return void
+	 */
+	public function setUp() {
+		parent::setUp();
+
+		Configure::write('App.fullBaseUrl', 'http://foo.bar');
+
+		$this->errorHandlerMiddleware = new ErrorHandlerMiddleware();
+	}
+
+	/**
+	 * @return void
+	 */
+	public function tearDown() {
+		parent::tearDown();
+
+		unset($this->errorHandlerMiddleware);
+
+		Configure::delete('App.fullBaseUrl');
+	}
+
+	/**
+	 * @return void
+	 */
+	public function test404() {
+		$parameters = [
+			new NotFoundException(),
+			new Request(),
+		];
+		$result = $this->invokeMethod($this->errorHandlerMiddleware, 'is404', $parameters);
+		$this->assertTrue($result);
+
+		$request = new Request('http://foo.bar', Request::METHOD_GET, ['Referer' => 'http://foo.bar/baz']);
+		$parameters = [
+			new NotFoundException(),
+			$request,
+		];
+		$result = $this->invokeMethod($this->errorHandlerMiddleware, 'is404', $parameters);
+		$this->assertFalse($result);
+	}
+
+}