Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broken configuration file error display is broken #17911

Closed
williamdes opened this issue Nov 20, 2022 · 1 comment
Closed

Broken configuration file error display is broken #17911

williamdes opened this issue Nov 20, 2022 · 1 comment
Assignees
Labels
affects/5.2 This issue or pull-request affects 5.2.x releases (and maybe further versions) affects/6.0 This issue or pull-request affects 6.0.x releases (and maybe further versions) Bug A problem or regression with an existing feature confirmed/5.2 This issue is confirmed to be reproduced on 5.2 at the time this label was set confirmed/6.0 This issue is confirmed to be reproduced on 6.0 at the time this label was set has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Milestone

Comments

@williamdes
Copy link
Member

Describe the bug

Due to c04f85f

To Reproduce

Make a config file with a php syntax mistake in it

Expected behavior

image

Screenshots

Do not try to fix errors one by one from the screenshot below, it is pointless as you will end up rendering ErrorHandler::handleException and a white non user friendly page

image

Server configuration

  • phpMyAdmin version: 5.2

Additional context

Preferred patch

diff --git a/libraries/classes/Common.php b/libraries/classes/Common.php
index 635dc69c33..3a57201dd5 100644
--- a/libraries/classes/Common.php
+++ b/libraries/classes/Common.php
@@ -92,8 +92,6 @@ public static function run(): void
 
         $containerBuilder = Core::getContainerBuilder();
 
-        /** @var ErrorHandler $errorHandler */
-        $errorHandler = $containerBuilder->get('error_handler');
 
         self::checkRequiredPhpExtensions();
         self::configurePhpSettings();
@@ -114,6 +112,9 @@ public static function run(): void
          */
         $config = $containerBuilder->get('config');
 
+        /** @var ErrorHandler $errorHandler */
+        $errorHandler = $containerBuilder->get('error_handler');
+
         /**
          * include session handling after the globals, to prevent overwriting
          */

Alternative patch

diff --git a/libraries/classes/ErrorHandler.php b/libraries/classes/ErrorHandler.php
index 1e114ccc2f..f89cc3f03b 100644
--- a/libraries/classes/ErrorHandler.php
+++ b/libraries/classes/ErrorHandler.php
@@ -64,6 +64,8 @@ class ErrorHandler
 
     public function __construct()
     {
+        global $isConfigLoading;
+
         /**
          * Do not set ourselves as error handler in case of testsuite.
          *
@@ -71,7 +73,9 @@ public function __construct()
          * rely on PHPUnit doing it's own error handling which we break here.
          */
         if (! defined('TESTSUITE')) {
-            set_exception_handler([$this, 'handleException']);
+            if ($isConfigLoading) {
+                set_exception_handler([$this, 'handleException']);
+            }
             set_error_handler([$this, 'handleError']);
         }
 
@williamdes
Copy link
Member Author

@MauricioFauth you did the fix I mentioned, what is the best patch ?

@williamdes williamdes added Bug A problem or regression with an existing feature affects/5.2 This issue or pull-request affects 5.2.x releases (and maybe further versions) affects/6.0 This issue or pull-request affects 6.0.x releases (and maybe further versions) confirmed/5.2 This issue is confirmed to be reproduced on 5.2 at the time this label was set confirmed/6.0 This issue is confirmed to be reproduced on 6.0 at the time this label was set labels Nov 20, 2022
@MauricioFauth MauricioFauth self-assigned this Nov 21, 2022
@MauricioFauth MauricioFauth added the has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete label Nov 21, 2022
@williamdes williamdes added this to the 5.2.1 milestone Nov 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects/5.2 This issue or pull-request affects 5.2.x releases (and maybe further versions) affects/6.0 This issue or pull-request affects 6.0.x releases (and maybe further versions) Bug A problem or regression with an existing feature confirmed/5.2 This issue is confirmed to be reproduced on 5.2 at the time this label was set confirmed/6.0 This issue is confirmed to be reproduced on 6.0 at the time this label was set has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Projects
None yet
Development

No branches or pull requests

2 participants