DatabaseExceptionWrapper::decodeException() depends on an optional previous Exception

#1: 2003654-1-fatal-error-for-DatabaseExceptionWrapper.diff queued for re-testing.

+1

https://gist.github.com/glennpratt/5666118

Minor nit

+++ b/core/lib/Drupal/Core/Controller/ExceptionController.phpundefined
@@ -329,7 +329,22 @@ protected function decodeException(FlattenException $exception) {
+        // The first element in the stack is the call, the second element gives us the caller.
+        // We skip calls that occurred in one of the classes of the database layer

Should wrap at 80 chars

Any way you can replicate this in a test?

I like this approach

Some minor phpcs nitpicks

+++ b/core/lib/Drupal/Core/Utility/WrapperException.phpundefined
@@ -0,0 +1,37 @@
+ * Definition of Drupal\Core\Utility\WrapperException

new syntax is Contains \Drupal\Core\Utility\WrapperException

+++ b/core/lib/Drupal/Core/Utility/WrapperException.phpundefined
@@ -0,0 +1,37 @@
+use RuntimeException;

no need to use root-level objects, use \RuntimeException instead

+++ b/core/lib/Drupal/Core/Utility/WrapperException.phpundefined
@@ -0,0 +1,37 @@
+class WrapperException extends RuntimeException {

\RuntimeException once use statement is gone

+++ b/core/modules/system/lib/Drupal/system/Tests/Utility/WrapperExceptionTest.phpundefined
@@ -0,0 +1,54 @@
+ * Contains Drupal\system\Tests\Utility\WrapperExceptionTest.

needs leading \

I think the answer might be no, but I know the question will be asked - can we use UnitTestCase instead of UnitTestBase aka Php unit here instead for the test? Would require mock objects.

+++ b/core/tests/Drupal/Tests/Core/Utility/WrapperExceptionTest.phpundefined
@@ -0,0 +1,38 @@
+    $this->assertEquals('Exception', get_class($stub->getPrevious()));

is it worth making it new \Exception('Something went wrong') and then adding another assert such as

$this->assertEquals($stub->getPrevious()->getMessage(), 'Something went wrong');

?

Other than that RTBC imo

+++ b/core/tests/Drupal/Tests/Core/Utility/WrapperExceptionTest.phpundefined
@@ -0,0 +1,38 @@
+    $stub = $this->getMockBuilder('\Drupal\Core\Utility\WrapperException')
+      ->setConstructorArgs(array('Test exception', 0, new \Exception()))

is there a reason why we can just go $stub = new WrapperException('Test Exception', 0, new \Exception');?
Would need a use at the top of the file obviously - I don't think we need the mock in this case because its a simple class?

I know you're probably getting tired of my feedback........but

+++ b/core/lib/Drupal/Core/Database/DatabaseExceptionWrapper.phpundefined
@@ -7,13 +7,13 @@
+class DatabaseExceptionWrapper extends WrapperException implements DatabaseException {

Is there a reason why we need the extra class (WrapperException) can't we just make the $previous argument required on DatabaseExceptionWrapper? Then we can write a unit test for decodeException method, mocking FlattenException - but that would require changing use of check_plain something else.

I was just mucking about with Views trying to track down an exposed filter problem when I got:

PHP Fatal error: Call to a member function getTrace() on a non-object in /DRUPAL8/core/lib/Drupal/Core/Controller/ExceptionController.php on line 451

By trying to access /admin/content

Really, this should never happen with a D8 Core install. You should have to extend it somehow before seeing stuff like this. That's why I'm marking this major.

This is just a re-roll, but it didn't seem to resolve the problem, just seemed to change it.

PHP Fatal error: Class 'Drupal\Core\Database\WrapperException' not found in /DRUPAL8/core/lib/Drupal/Core/Database/DatabaseExceptionWrapper.php on line 15

core/lib/Drupal/Core/Utility/WrapperException.php already exists.

Other than that just a re-roll

hmm...

The ExceptionController was removed/refactored in #2323759: Modularize kernel exception handling, not sure if this is still relevant.

Thanks everyone who contributed to this issue.

Based on #2323759: Modularize kernel exception handling and after consulting with xtfer we have determined this issue is no longer relevant.