Fixed review issues. Grouped config params. Inject factories for loggers instead of actual loggers in LoggerProxy. Added config path to ConfigOptionsListConstants. Fixed unit tests as needed.

federivo · federivo · commit 3bb5d6205df6 · 2017-04-18T03:19:20.000-03:00
diff --git a/app/code/Magento/Developer/Console/Command/QueryLogDisableCommand.php b/app/code/Magento/Developer/Console/Command/QueryLogDisableCommand.php
@@ -61,7 +61,7 @@ protected function configure()
     protected function execute(InputInterface $input, OutputInterface $output)
     {
         $data = [LoggerProxy::PARAM_ALIAS => LoggerProxy::LOGGER_ALIAS_DISABLED];
-        $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]);
+        $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => [LoggerProxy::CONF_GROUP_NAME => $data]]);
 
         $output->writeln("<info>". self::SUCCESS_MESSAGE . "</info>");
     }
diff --git a/app/code/Magento/Developer/Console/Command/QueryLogEnableCommand.php b/app/code/Magento/Developer/Console/Command/QueryLogEnableCommand.php
@@ -107,7 +107,9 @@ protected function execute(InputInterface $input, OutputInterface $output)
         $data[LoggerProxy::PARAM_QUERY_TIME] = number_format($logQueryTime, 3);
         $data[LoggerProxy::PARAM_CALL_STACK] = (int)($logCallStack != 'false');
 
-        $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]);
+        $configGroup[LoggerProxy::CONF_GROUP_NAME] = $data;
+
+        $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $configGroup]);
 
         $output->writeln("<info>". self::SUCCESS_MESSAGE . "</info>");
     }
diff --git a/app/code/Magento/Developer/Test/Unit/Console/Command/QueryLogDisableCommandTest.php b/app/code/Magento/Developer/Test/Unit/Console/Command/QueryLogDisableCommandTest.php
@@ -51,7 +51,7 @@ public function testExecute()
         $this->configWriter
             ->expects($this->once())
             ->method('saveConfig')
-            ->with([ConfigFilePool::APP_ENV => $data]);
+            ->with([ConfigFilePool::APP_ENV => [LoggerProxy::CONF_GROUP_NAME => $data]]);
 
         $commandTester = new CommandTester($this->command);
         $commandTester->execute([]);
diff --git a/app/code/Magento/Developer/Test/Unit/Console/Command/QueryLogEnableCommandTest.php b/app/code/Magento/Developer/Test/Unit/Console/Command/QueryLogEnableCommandTest.php
@@ -57,7 +57,7 @@ public function testExecuteWithNoParams()
         $this->configWriter
             ->expects($this->any())
             ->method('saveConfig')
-            ->with([ConfigFilePool::APP_ENV => $data]);
+            ->with([ConfigFilePool::APP_ENV => [LoggerProxy::CONF_GROUP_NAME => $data]]);
 
         $commandTester = new CommandTester($this->command);
         $commandTester->execute([]);
@@ -83,7 +83,7 @@ public function testExecuteWithParams()
         $this->configWriter
             ->expects($this->any())
             ->method('saveConfig')
-            ->with([ConfigFilePool::APP_ENV => $data]);
+            ->with([ConfigFilePool::APP_ENV => [LoggerProxy::CONF_GROUP_NAME => $data]]);
 
         $commandTester = new CommandTester($this->command);
         $commandTester->execute(['false', '0.05', 'false']);
diff --git a/app/etc/di.xml b/app/etc/di.xml
@@ -1320,10 +1320,10 @@
     </type>
     <type name="Magento\Framework\DB\Logger\LoggerProxy">
         <arguments>
-            <argument name="loggerAlias" xsi:type="init_parameter">\Magento\Framework\DB\Logger\LoggerProxy::PARAM_ALIAS</argument>
-            <argument name="logAllQueries" xsi:type="init_parameter">\Magento\Framework\DB\Logger\LoggerProxy::PARAM_LOG_ALL</argument>
-            <argument name="logQueryTime" xsi:type="init_parameter">\Magento\Framework\DB\Logger\LoggerProxy::PARAM_QUERY_TIME</argument>
-            <argument name="logCallStack" xsi:type="init_parameter">\Magento\Framework\DB\Logger\LoggerProxy::PARAM_CALL_STACK</argument>
+            <argument name="loggerAlias" xsi:type="init_parameter">Magento\Framework\Config\ConfigOptionsListConstants::CONFIG_PATH_DB_LOGGER_OUTPUT</argument>
+            <argument name="logAllQueries" xsi:type="init_parameter">Magento\Framework\Config\ConfigOptionsListConstants::CONFIG_PATH_DB_LOGGER_LOG_EVERYTHING</argument>
+            <argument name="logQueryTime" xsi:type="init_parameter">Magento\Framework\Config\ConfigOptionsListConstants::CONFIG_PATH_DB_LOGGER_QUERY_TIME_THRESHOLD</argument>
+            <argument name="logCallStack" xsi:type="init_parameter">Magento\Framework\Config\ConfigOptionsListConstants::CONFIG_PATH_DB_LOGGER_INCLUDE_STACKTRACE</argument>
         </arguments>
     </type>
 
diff --git a/lib/internal/Magento/Framework/Config/ConfigOptionsListConstants.php b/lib/internal/Magento/Framework/Config/ConfigOptionsListConstants.php
@@ -30,6 +30,10 @@ class ConfigOptionsListConstants
     const CONFIG_PATH_DB = 'db';
     const CONFIG_PATH_RESOURCE = 'resource';
     const CONFIG_PATH_CACHE_TYPES = 'cache_types';
+    const CONFIG_PATH_DB_LOGGER_OUTPUT = 'db_logger/output';
+    const CONFIG_PATH_DB_LOGGER_LOG_EVERYTHING = 'db_logger/log_everything';
+    const CONFIG_PATH_DB_LOGGER_QUERY_TIME_THRESHOLD = 'db_logger/query_time_threshold';
+    const CONFIG_PATH_DB_LOGGER_INCLUDE_STACKTRACE = 'db_logger/include_stacktrace';
     /**#@-*/
 
     /**#@+
diff --git a/lib/internal/Magento/Framework/DB/Logger/LoggerProxy.php b/lib/internal/Magento/Framework/DB/Logger/LoggerProxy.php
@@ -5,27 +5,34 @@
  */
 namespace Magento\Framework\DB\Logger;
 
-class LoggerProxy extends LoggerAbstract
+use Magento\Framework\DB\LoggerInterface;
+
+class LoggerProxy implements LoggerInterface
 {
+    /**
+     * Configuration group name
+     */
+    const CONF_GROUP_NAME = 'db_logger';
+
     /**
      * Logger alias param name
      */
-    const PARAM_ALIAS = 'db_logger_mode';
+    const PARAM_ALIAS = 'output';
 
     /**
      * Logger log all param name
      */
-    const PARAM_LOG_ALL = 'db_logger_all';
+    const PARAM_LOG_ALL = 'log_everything';
 
     /**
      * Logger query time param name
      */
-    const PARAM_QUERY_TIME = 'db_logger_query_time';
+    const PARAM_QUERY_TIME = 'query_time_threshold';
 
     /**
      * Logger call stack param name
      */
-    const PARAM_CALL_STACK = 'db_logger_stack';
+    const PARAM_CALL_STACK = 'include_stacktrace';
 
     /**
      * File logger alias
@@ -38,41 +45,45 @@ class LoggerProxy extends LoggerAbstract
     const LOGGER_ALIAS_DISABLED = 'disabled';
 
     /**
-     * @var LoggerAbstract
+     * @var LoggerInterface
      */
     private $logger;
 
     /**
      * LoggerProxy constructor.
-     * @param File $file
-     * @param Quiet $quiet
+     * @param FileFactory $fileFactory
+     * @param QuietFactory $quietFactory
      * @param bool $loggerAlias
      * @param bool $logAllQueries
      * @param float $logQueryTime
      * @param bool $logCallStack
      */
     public function __construct(
-        File $file,
-        Quiet $quiet,
+        FileFactory $fileFactory,
+        QuietFactory $quietFactory,
         $loggerAlias,
         $logAllQueries = true,
         $logQueryTime = 0.001,
         $logCallStack = true
     ) {
         switch ($loggerAlias) {
             case self::LOGGER_ALIAS_FILE:
-                $this->logger = $file;
+                $this->logger = $fileFactory->create(
+                    [
+                        'logAllQueries' => $logAllQueries,
+                        'logQueryTime' => $logQueryTime,
+                        'logCallStack' => $logCallStack,
+                    ]
+                );
                 break;
             default:
-                $this->logger = $quiet;
+                $this->logger = $quietFactory->create();
                 break;
         }
-
-        parent::__construct($logAllQueries, $logQueryTime, $logCallStack);
     }
 
     /**
-     * @return LoggerAbstract|Quiet
+     * @return LoggerInterface
      */
     public function getLogger()
     {
@@ -110,4 +121,12 @@ public function critical(\Exception $e)
     {
         $this->logger->critical($e);
     }
+
+    /**
+     * @return void
+     */
+    public function startTimer()
+    {
+        $this->logger->startTimer();
+    }
 }
diff --git a/lib/internal/Magento/Framework/DB/Test/Unit/DB/Logger/LoggerProxyTest.php b/lib/internal/Magento/Framework/DB/Test/Unit/DB/Logger/LoggerProxyTest.php
@@ -6,6 +6,8 @@
 
 namespace Magento\Framework\Test\Unit\DB\Logger;
 
+use Magento\Framework\DB\Logger\FileFactory;
+use Magento\Framework\DB\Logger\QuietFactory;
 use Magento\Framework\DB\Logger\LoggerProxy;
 use Magento\Framework\DB\Logger\File;
 use Magento\Framework\DB\Logger\Quiet;
@@ -36,9 +38,23 @@ public function setUp()
      */
     public function testNewWithAliasFile()
     {
+        $fileLoggerMock = $this->getMockBuilder(File::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $fileLoggerFactoryMock = $this->getMockBuilder(FileFactory::class)
+            ->disableOriginalConstructor()
+            ->setMethods(['create'])
+            ->getMock();
+
+        $fileLoggerFactoryMock->expects($this->once())
+            ->method('create')
+            ->willReturn($fileLoggerMock);
+
         $this->loggerProxy = $this->objectManager->getObject(
             LoggerProxy::class,
             [
+                'fileFactory' => $fileLoggerFactoryMock,
                 'loggerAlias' => LoggerProxy::LOGGER_ALIAS_FILE,
             ]
         );
@@ -51,9 +67,23 @@ public function testNewWithAliasFile()
      */
     public function testNewWithAliasDisabled()
     {
+        $quietLoggerMock = $this->getMockBuilder(Quiet::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $quietLoggerFactoryMock = $this->getMockBuilder(QuietFactory::class)
+            ->disableOriginalConstructor()
+            ->setMethods(['create'])
+            ->getMock();
+
+        $quietLoggerFactoryMock->expects($this->once())
+            ->method('create')
+            ->willReturn($quietLoggerMock);
+
         $this->loggerProxy = $this->objectManager->getObject(
             LoggerProxy::class,
             [
+                'quietFactory' => $quietLoggerFactoryMock,
                 'loggerAlias' => LoggerProxy::LOGGER_ALIAS_DISABLED,
             ]
         );

Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ protected function configure()`
`61`	`61`	`protected function execute(InputInterface $input, OutputInterface $output)`
`62`	`62`	`{`
`63`	`63`	`$data = [LoggerProxy::PARAM_ALIAS => LoggerProxy::LOGGER_ALIAS_DISABLED];`
`64`		`- $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]);`
	`64`	`+ $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => [LoggerProxy::CONF_GROUP_NAME => $data]]);`
`65`	`65`
`66`	`66`	`$output->writeln("<info>". self::SUCCESS_MESSAGE . "</info>");`
`67`	`67`	`}`
Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,9 @@ protected function execute(InputInterface $input, OutputInterface $output)`
`107`	`107`	`$data[LoggerProxy::PARAM_QUERY_TIME] = number_format($logQueryTime, 3);`
`108`	`108`	`$data[LoggerProxy::PARAM_CALL_STACK] = (int)($logCallStack != 'false');`
`109`	`109`
`110`		`- $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]);`
	`110`	`+ $configGroup[LoggerProxy::CONF_GROUP_NAME] = $data;`
	`111`	`+`
	`112`	`+ $this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $configGroup]);`
`111`	`113`
`112`	`114`	`$output->writeln("<info>". self::SUCCESS_MESSAGE . "</info>");`
`113`	`115`	`}`