Update logger to build logging function without Eval

[benjisg]

Updated to use immediate function with state to build logging function for each logging state

[baudehlo]

Merged as 679b4a9

[mscdex]

Why are there separate log functions for each level anyway? Removing that requirement would alleviate the scoping issue with the level variable and such.

[baudehlo]

Because I didn't like the design in Qpsmtpd of having ->log(LEVEL, msg) and just prefer separate methods. Just a design decision.

[baudehlo]

OK, so this patch still didn't work. I think it might actually be a bug in v8 though - there may be a premature optimisation of string.splice(0) just returning the same string object.

I wonder if doing it via new String() might work though...

[benjisg]

Actually this was from a bug I didn't catch in my patch. It was causing the loglevel to always be compared to a dynamic referenced version of key_copy in such a way that it always compared itself to 0 (and thus always logged everything). I also realized the key.slice(0) wasn't needed as it returns the exact same string, so I factored it out and both commits are now available on my fork.

[mscdex]

Only non-primitive types ("objects", arrays, functions) are handled by reference. string.slice(0) does return a new string, but comparing it with string using == or === will always yield true because you are comparing values, not references. Using new String(string.slice(0)) will create a new string object though, which will no longer === string.

However, it is better to just keep the one string around like this last commit does.

[benjisg]

Actually in this case key_copy was specifically being treated as a reference due to it's relation in the built functions. If we add in a log of the value of key_copy to the old code:

for (var key in logger) {
    if (key.match(/^LOG\w/)) {
        var level = key.slice(3);
        var key_copy = key.slice(0); // copy
        logger[key.toLowerCase()] = (function(level) {
            return function(data) {
                logger.log(key_copy);
                if (loglevel >= logger[key_copy]) {
                    logger.log("[" + level + "] " + data);
                }
            }
        })(level);
    }
}

and then call the individual log functions:

logger.logprotocol("protocol log");
logger.logdebug("debug log");
logger.loginfo("info log");
logger.lognotice("notice log");
logger.logwarn("warn log");
logger.logerror("error log");
logger.logcrit("crit log");
logger.logalert("alert log");
logger.logemerg("emerg log");

you'll end up with this being logged:

LOGEMERG
[PROTOCOL] protocol log
LOGEMERG
[DEBUG] debug log
LOGEMERG
[INFO] info log
LOGEMERG
[NOTICE] notice log
LOGEMERG
[WARN] warn log
LOGEMERG
[ERROR] error log
LOGEMERG
[CRIT] crit log
LOGEMERG
[ALERT] alert log
LOGEMERG
[EMERG] emerg log

This is due to each function storing a reference to key_copy rather than it's actual value at build time, such that every successive update to key_copy is propagating it's value to all of the functions created so far in the loop. So we end up with whatever value was passed to it last (LOGEMERG in this case). key.slice(0) likely does make a new string but the var key_copy declaration would be hoisted out of the for loop anyway.

As you said, it shouldn't matter now that it's been factored out. :)

[baudehlo]

OK, all merged now.