Log4php
  1. Log4php
  2. LOG4PHP-62

Does not print warning if ini file is corrupt

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: 2.0.0
    • Component/s: Code
    • Labels:
      None

      Description

      Grmpf! There was a simple typo in the ini file and log4php did ... nothing! Not even tell me that there was an error or even where exactly! It took me quite some time to debug that. How I hate such software...

      I understand that a logging framework tries to be as non-disruptive as possible and we can argue about whether it should throw an exception just because the database server, it tries to log, to is suddenly (temporarily) unavailable but as parsing the ini file happens at the very first of most applications I'm totally in favour of throwing an exception here!

      Please find a patch attached. It will cause the output to look like:

      $ php testPDO.php

      Exception: LoggerConfiguratorIni: Error parsing testPDO.properties on line 7
      in /srv/home/james/workspace/log4php/src/main/php/configurators/LoggerConfiguratorIni.php on line 307

      bye,

      christian

      1. WarnIfIniHasErrors-Test.diff
        0.7 kB
        Christian Hammers
      2. WarnIfIniHasErrors.diff
        0.7 kB
        Christian Hammers

        Activity

        Christian Hammers created issue -
        Hide
        Christian Hammers added a comment -

        Throw exceptions when ini file is corrupt.

        Show
        Christian Hammers added a comment - Throw exceptions when ini file is corrupt.
        Christian Hammers made changes -
        Field Original Value New Value
        Attachment WarnIfIniHasErrors.diff [ 12414561 ]
        Christian Grobmeier made changes -
        Fix Version/s 2.0 [ 12313916 ]
        Affects Version/s 2.0 [ 12313916 ]
        Affects Version/s 2.2 [ 12313918 ]
        Hide
        Christian Hammers added a comment -

        And now that the unit tests work, here is the necessary patch for the newly introduced exception:

        Index: src/test/php/configurators/LoggerConfiguratorIniTest.php
        ===================================================================
        — src/test/php/configurators/LoggerConfiguratorIniTest.php (Revision 798271)
        +++ src/test/php/configurators/LoggerConfiguratorIniTest.php (Arbeitskopie)
        @@ -65,7 +65,13 @@
        }

        public function testConfigureWithoutIniFile() {

        • self::assertFalse(LoggerConfiguratorIni::configure());
          + $catchedException = null;
          + try { + LoggerConfiguratorIni::configure(); + }

          catch (Exception $e)

          { + $catchedException = $e; + }

          + self::assertNotNull($catchedException);
          }

        public function testConfigureWithEmptyIniFile() {

        Show
        Christian Hammers added a comment - And now that the unit tests work, here is the necessary patch for the newly introduced exception: Index: src/test/php/configurators/LoggerConfiguratorIniTest.php =================================================================== — src/test/php/configurators/LoggerConfiguratorIniTest.php (Revision 798271) +++ src/test/php/configurators/LoggerConfiguratorIniTest.php (Arbeitskopie) @@ -65,7 +65,13 @@ } public function testConfigureWithoutIniFile() { self::assertFalse(LoggerConfiguratorIni::configure()); + $catchedException = null; + try { + LoggerConfiguratorIni::configure(); + } catch (Exception $e) { + $catchedException = $e; + } + self::assertNotNull($catchedException); } public function testConfigureWithEmptyIniFile() {
        Hide
        Christian Hammers added a comment -

        cut & paste from the comment does not work well so I attach the patch

        Show
        Christian Hammers added a comment - cut & paste from the comment does not work well so I attach the patch
        Christian Hammers made changes -
        Attachment WarnIfIniHasErrors-Test.diff [ 12414656 ]
        Hide
        Christian Grobmeier added a comment -

        Applied with rv798406 Thanks very much!

        Just had to count array vars cause I would like to support as many PHP 5.2.x as possible so I didn't count on return of false instead of empty array.

        I also had to make testConfigureWithEmptyIniFile fit like the other testmethod you send by patch.

        However, thanks very much!

        Show
        Christian Grobmeier added a comment - Applied with rv798406 Thanks very much! Just had to count array vars cause I would like to support as many PHP 5.2.x as possible so I didn't count on return of false instead of empty array. I also had to make testConfigureWithEmptyIniFile fit like the other testmethod you send by patch. However, thanks very much!
        Christian Grobmeier made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Ivan Habunek made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Christian Hammers
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development