From 5e7410a93011b751e98111c462eabf01af862149 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maur=C3=ADcio=20Meneghini=20Fauth?= <mauricio@fauth.dev>
Date: Sat, 29 Jan 2022 16:55:44 -0300
Subject: [PATCH] Improve Error message stacktrace style
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes #17326

Signed-off-by: Maurício Meneghini Fauth <mauricio@fauth.dev>
---
 libraries/classes/Error.php      | 42 ++++++++++++--------------------
 templates/error/get_display.twig | 23 +++++++----------
 test/classes/ErrorTest.php       | 16 +++++++++---
 3 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/libraries/classes/Error.php b/libraries/classes/Error.php
index ec6e3f6b53..b06b35a3ef 100644
--- a/libraries/classes/Error.php
+++ b/libraries/classes/Error.php
@@ -354,62 +354,52 @@ class Error extends Message
      */
     public function getBacktraceDisplay(): string
     {
-        return self::formatBacktrace(
-            $this->getBacktrace(),
-            "<br>\n",
-            "<br>\n"
-        );
+        return self::formatBacktrace($this->getBacktrace());
     }
 
     /**
      * return formatted backtrace field
      *
-     * @param array  $backtrace Backtrace data
-     * @param string $separator Arguments separator to use
-     * @param string $lines     Lines separator to use
+     * @param array $backtrace Backtrace data
      *
      * @return string formatted backtrace
      */
-    public static function formatBacktrace(
-        array $backtrace,
-        string $separator,
-        string $lines
-    ): string {
-        $retval = '';
+    public static function formatBacktrace(array $backtrace): string
+    {
+        $retval = '<ol class="list-group">';
 
         foreach ($backtrace as $step) {
+            $retval .= '<li class="list-group-item">';
             if (isset($step['file'], $step['line'])) {
-                $retval .= self::relPath($step['file'])
-                    . '#' . $step['line'] . ': ';
+                $retval .= self::relPath($step['file']) . '#' . $step['line'] . ': ';
             }
 
             if (isset($step['class'])) {
                 $retval .= $step['class'] . $step['type'];
             }
 
-            $retval .= self::getFunctionCall($step, $separator);
-            $retval .= $lines;
+            $retval .= self::getFunctionCall($step);
+            $retval .= '</li>';
         }
 
-        return $retval;
+        return $retval . '</ol>';
     }
 
     /**
      * Formats function call in a backtrace
      *
-     * @param array  $step      backtrace step
-     * @param string $separator Arguments separator to use
+     * @param array $step backtrace step
      */
-    public static function getFunctionCall(array $step, string $separator): string
+    public static function getFunctionCall(array $step): string
     {
         $retval = $step['function'] . '(';
         if (isset($step['args'])) {
             if (count($step['args']) > 1) {
-                $retval .= $separator;
+                $retval .= '<br>';
                 foreach ($step['args'] as $arg) {
                     $retval .= "\t";
                     $retval .= $arg;
-                    $retval .= ',' . $separator;
+                    $retval .= ',<br>';
                 }
             } elseif (count($step['args']) > 0) {
                 foreach ($step['args'] as $arg) {
@@ -481,12 +471,12 @@ class Error extends Message
 
         return $template->render('error/get_display', [
             'context' => $context,
-            'isUserError' => $this->isUserError(),
+            'is_user_error' => $this->isUserError(),
             'type' => $this->getType(),
             'file' => $this->getFile(),
             'line' => $this->getLine(),
             'message' => $this->getMessage(),
-            'backtraceDisplay' => $this->getBacktraceDisplay(),
+            'formatted_backtrace' => $this->getBacktraceDisplay(),
         ]);
     }
 
diff --git a/templates/error/get_display.twig b/templates/error/get_display.twig
index 43e22a08e7..81a555a0e9 100644
--- a/templates/error/get_display.twig
+++ b/templates/error/get_display.twig
@@ -1,17 +1,12 @@
 <div class="alert alert-{{ context }}" role="alert">
+  {%- if not is_user_error -%}
+    <p><strong>{{ type }}</strong> in {{ file }}#{{ line }}</p>
+  {%- endif -%}
 
-  {%- if isUserError == false %}
-    <strong>{{ type }}</strong> in {{ file ~'#'~ line }} <br>
-  {% endif -%}
+  {{ message|raw }}
 
-  {{ message }}
-
-  {% if isUserError == false %}
-    <br><br>
-    <strong>Backtrace</strong><br>
-    <br>
-    {{ backtraceDisplay }}
-  {% endif %}
-
-
-</div>
\ No newline at end of file
+  {%- if not is_user_error -%}
+    <p class="mt-3"><strong>Backtrace</strong></p>
+    {{- formatted_backtrace|raw -}}
+  {%- endif -%}
+</div>
diff --git a/test/classes/ErrorTest.php b/test/classes/ErrorTest.php
index 0f830ac5ee..2dab5dad49 100644
--- a/test/classes/ErrorTest.php
+++ b/test/classes/ErrorTest.php
@@ -120,7 +120,7 @@ class ErrorTest extends AbstractTestCase
     public function testGetBacktraceDisplay(): void
     {
         $this->assertStringContainsString(
-            'PHPUnit\Framework\TestResult->run(<Class:PhpMyAdmin\Tests\ErrorTest>)<br>',
+            'PHPUnit\Framework\TestResult->run(<Class:PhpMyAdmin\Tests\ErrorTest>)',
             $this->object->getBacktraceDisplay()
         );
     }
@@ -130,10 +130,18 @@ class ErrorTest extends AbstractTestCase
      */
     public function testGetDisplay(): void
     {
-        $this->assertStringContainsString(
-            '<div class="alert alert-danger" role="alert">    <strong>Warning</strong>',
-            $this->object->getDisplay()
+        $actual = $this->object->getDisplay();
+        $this->assertStringStartsWith(
+            '<div class="alert alert-danger" role="alert"><p><strong>Warning</strong> in error.txt#15</p>'
+            . '<img src="themes/dot.gif" title="" alt="" class="icon ic_s_error"> Compile Error'
+            . '<p class="mt-3"><strong>Backtrace</strong></p><ol class="list-group"><li class="list-group-item">',
+            $actual
         );
+        $this->assertStringContainsString(
+            'PHPUnit\Framework\TestResult->run(<Class:PhpMyAdmin\Tests\ErrorTest>)</li><li class="list-group-item">',
+            $actual
+        );
+        $this->assertStringEndsWith('</li></ol></div>' . "\n", $actual);
     }
 
     /**