Log4php
  1. Log4php
  2. LOG4PHP-161

All configurable components should trigger warnings when given invalid values

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.0
    • Fix Version/s: 2.2.0
    • Component/s: Code

      Description

      This includes: appenders, layouts, renderers, filters (and any future ones).

      This is usually implemented in the property setter (sometime in activateOptions()). The problem is that the code for checking if something is an integer/boolean/string/whatever, and reporting an error is relatively long (cca 5-10 lines). We do not want to recycle this code for each and every setter.

      The proposed solution is to make a base class from which all configurable classes will inherit. This class will provide generic setters which include validation and error reporting.

      This is a mockup:
      http://pastebin.com/0h4M35Br

      I believe a solution to this issue should be included in 2.2 to complement the new configurator.

        Activity

        Hide
        Ivan Habunek added a comment -

        Done. That was a big scary commit... Hopefully nothing is broken. All tests pass.

        Show
        Ivan Habunek added a comment - Done. That was a big scary commit... Hopefully nothing is broken. All tests pass.
        Hide
        Florian Semm added a comment -

        I like the last version very much. It's a very clean solution.

        Is it possible to rename this class to 'Configurable'. This classname says more about the work. A appender and a filter could inherit from this class.

        Show
        Florian Semm added a comment - I like the last version very much. It's a very clean solution. Is it possible to rename this class to 'Configurable'. This classname says more about the work. A appender and a filter could inherit from this class.
        Hide
        Ivan Habunek added a comment -

        Actually, after trying to convert a couple of appenders to use the base class, it seems much simpler to remove the generic set() method and just use the actual setter methods - setString(), setInteger() etc.

        This also removes a lot of the code from the base class, here's another mockup:
        http://pastebin.com/2yMsU9st

        It might be possible to go to a higher level of abstraction with the setter methods, since a lot of the code is repeated. But that might be going a bit too far.

        Show
        Ivan Habunek added a comment - Actually, after trying to convert a couple of appenders to use the base class, it seems much simpler to remove the generic set() method and just use the actual setter methods - setString(), setInteger() etc. This also removes a lot of the code from the base class, here's another mockup: http://pastebin.com/2yMsU9st It might be possible to go to a higher level of abstraction with the setter methods, since a lot of the code is repeated. But that might be going a bit too far.
        Hide
        Ivan Habunek added a comment -

        Here's a modified example where LoggerOptionConverter handles all the conversion, and the base class does the logging.
        http://pastebin.com/AyGWwqSN

        What do you think?

        Show
        Ivan Habunek added a comment - Here's a modified example where LoggerOptionConverter handles all the conversion, and the base class does the logging. http://pastebin.com/AyGWwqSN What do you think?
        Hide
        Florian Semm added a comment -

        1. it's right that a object knows its parameters and the type of this parameters, but the validation isn't a job for a object

        2. this point is absolutly correct. i haven't considered this case.

        the base class solution is the way to solve the problem, but the validation should do a helper class. the LoggerOptionConverter class is a good candidate for this.

        Show
        Florian Semm added a comment - 1. it's right that a object knows its parameters and the type of this parameters, but the validation isn't a job for a object 2. this point is absolutly correct. i haven't considered this case. the base class solution is the way to solve the problem, but the validation should do a helper class. the LoggerOptionConverter class is a good candidate for this.
        Hide
        Ivan Habunek added a comment -

        I can see two problems with the above ideas:

        1. In both cases, the configurator would have to know how to validate parameters. Currently, this is not the case, and I would like to keep it that way. The object which is being configured (appender/filter/whatever) is the only one which knows how to validate it's own parameters.

        2. If one was to programatically configure log4php (without using the configurator), the parameters should still be validated. For example, if I was to do this:

        $appender = new LoggerAppenderFile();
        $appender->setAppend("not a boolean");

        I would like the appender to throw a warning, since the "append" property is boolean.

        The only other solution I can see is to do this individually in each setter as it is done at the moment (in places where it is implemented at all). And this means a LOT of code recycling.

        I'm not a big fan of the base class solution, but it seems as the most economical one. Why do you think it's such a bad idea?

        Show
        Ivan Habunek added a comment - I can see two problems with the above ideas: 1. In both cases, the configurator would have to know how to validate parameters. Currently, this is not the case, and I would like to keep it that way. The object which is being configured (appender/filter/whatever) is the only one which knows how to validate it's own parameters. 2. If one was to programatically configure log4php (without using the configurator), the parameters should still be validated. For example, if I was to do this: $appender = new LoggerAppenderFile(); $appender->setAppend("not a boolean"); I would like the appender to throw a warning, since the "append" property is boolean. The only other solution I can see is to do this individually in each setter as it is done at the moment (in places where it is implemented at all). And this means a LOT of code recycling. I'm not a big fan of the base class solution, but it seems as the most economical one. Why do you think it's such a bad idea?
        Hide
        Florian Semm added a comment -

        It's a good idea to implement one "configurator", but I wouldn't do this with a god-logger class.

        I think there are two solutions:

        1. do it like the symfony2-framework

        Define the configuration in a class and validate the input with the configuration-class. See https://github.com/floriansemm/Log4PhpBundle/blob/master/DependencyInjection/Configuration/Configuration.php

        This happens before you setup the appenders/filters/what ever

        2. The LoggerAppender-class has a reference to a configuration class

        When you setup the appender the configuration class "validate" this config-property. The man problem is here: you have to declare the possible config-parameters for the appender

        Maybe it isn't the easiest way, but to create another "BaseClass" isn't a good idea. A mixed solution of your idea and my is also possible!

        Show
        Florian Semm added a comment - It's a good idea to implement one "configurator", but I wouldn't do this with a god-logger class. I think there are two solutions: 1. do it like the symfony2-framework Define the configuration in a class and validate the input with the configuration-class. See https://github.com/floriansemm/Log4PhpBundle/blob/master/DependencyInjection/Configuration/Configuration.php This happens before you setup the appenders/filters/what ever 2. The LoggerAppender-class has a reference to a configuration class When you setup the appender the configuration class "validate" this config-property. The man problem is here: you have to declare the possible config-parameters for the appender Maybe it isn't the easiest way, but to create another "BaseClass" isn't a good idea. A mixed solution of your idea and my is also possible!

          People

          • Assignee:
            Ivan Habunek
            Reporter:
            Ivan Habunek
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development