I like simple. :) On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson <cla...@renegat.net> wrote:
> It looks like that with slf4j, we could keep loggers injection even in > session tools, so no need to go static: > > "As of SLF4J version 1.5.3, logger instances survive serialization. Thus, > serialization of the host class no longer requires any special action, even > when loggers are declared as instance variables. In previous versions, > logger instances needed to be declared as transient in the host class." > ( http://www.slf4j.org/faq.html#declared_static ) > > So it looks like the perfect solution, and will simplify many things. > > Claude > > > > On 20/08/2015 21:32, Sergiu Dumitriu wrote: > >> A recent discussion was proposing to switch to slf4j as the façade. >> On Aug 20, 2015 3:12 PM, "Claude Brisson" <cla...@renegat.net> wrote: >> >> Thanks for the review. >>> >>> Yes, we should be using Velocity logging facade, but I just don't see for >>> now how we should do this while preserving tools serialization. >>> >>> For now I just mimicked a behavior present in some deprecated classes, >>> re-introducing the dependency of velocity-tools-view on commons-logging. >>> >>> I agree that we should try to do better, but if a logger cannot be >>> statically reached from a session tool, and if it cannot be passed at >>> initialization because of serialization, there's a problem... >>> >>> Let say for instance that we use transient weak references on >>> non-serializable Velocity loggers, fed up at initialization time. Then >>> the >>> tool could detect it has lost its logger at deserialization, and ask a >>> new >>> one, but to whom? It's a bad idea either to keep a static reference on >>> the >>> servlet context. >>> >>> I'm open to suggestions... >>> >>> >>> Claude >>> >>> >>> On 20/08/2015 20:45, Sergiu Dumitriu wrote: >>> >>> Isn't the logging framework supposed to be pluggable, meaning that all >>>> calls go through org.apache.velocity.runtime.log.Log which is then >>>> passed to the actual logging library in use, be it log4j, slf4j, jcl or >>>> commons-logging? >>>> >>>> On 08/20/2015 01:16 PM, cbris...@apache.org wrote: >>>> >>>> Author: cbrisson >>>>> Date: Thu Aug 20 17:16:02 2015 >>>>> New Revision: 1696822 >>>>> >>>>> URL: http://svn.apache.org/r1696822 >>>>> Log: >>>>> J2EE containers session serialization implies static logging from >>>>> session scoped tools >>>>> >>>>> Modified: >>>>> velocity/tools/trunk/src/site/xdoc/changes.xml >>>>> >>>>> >>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>> >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>> >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>> >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>> >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>> >>>>> Modified: velocity/tools/trunk/src/site/xdoc/changes.xml >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/src/site/xdoc/changes.xml?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- velocity/tools/trunk/src/site/xdoc/changes.xml (original) >>>>> +++ velocity/tools/trunk/src/site/xdoc/changes.xml Thu Aug 20 17:16:02 >>>>> 2015 >>>>> @@ -35,6 +35,7 @@ >>>>> <li>Reflect Velocity Engine dependency shading of >>>>> commons-lang >>>>> and commons-collections (cb)</li> >>>>> <li>Removed deprecated class >>>>> org.apache.velocity.tools.generic.log.LogSystemCommonsLog (cb)</li> >>>>> <li>MathTool: fixed result type calculations and added >>>>> bitwise >>>>> operations (cb)</li> >>>>> + <li>have session scoped tools use a static logger to fix J2EE >>>>> containers session serialization (cb)</li> >>>>> </ul> >>>>> </subsection> >>>>> >>>>> Modified: >>>>> >>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>> (original) >>>>> +++ >>>>> >>>>> velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java >>>>> Thu Aug 20 17:16:02 2015 >>>>> @@ -101,7 +101,6 @@ public class TilesTool extends ImportSup >>>>> setRequest(ctx.getRequest()); >>>>> setResponse(ctx.getResponse()); >>>>> setServletContext(ctx.getServletContext()); >>>>> - setLog(ctx.getVelocityEngine().getLog()); >>>>> } >>>>> } >>>>> >>>>> Modified: >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>> (original) >>>>> +++ >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java >>>>> Thu Aug 20 17:16:02 2015 >>>>> @@ -22,7 +22,8 @@ package org.apache.velocity.tools.view; >>>>> import java.util.Collections; >>>>> import java.util.List; >>>>> import javax.servlet.http.HttpServletRequest; >>>>> -import org.apache.velocity.runtime.log.Log; >>>>> +import org.apache.commons.logging.Log; >>>>> +import org.apache.commons.logging.LogFactory; >>>>> import org.apache.velocity.tools.Scope; >>>>> import org.apache.velocity.tools.config.DefaultKey; >>>>> import org.apache.velocity.tools.config.InvalidScope; >>>>> @@ -129,19 +130,11 @@ public abstract class AbstractSearchTool >>>>> protected static final String STORED_RESULTS_KEY = >>>>> StoredResults.class.getName(); >>>>> - protected Log LOG; >>>>> + protected static final Log LOG = >>>>> LogFactory.getLog(AbstractSearchTool.class); >>>>> + >>>>> private String criteriaKey = DEFAULT_CRITERIA_KEY; >>>>> private Object criteria; >>>>> - public void setLog(Log log) >>>>> - { >>>>> - if (log == null) >>>>> - { >>>>> - throw new NullPointerException("log should not be set to >>>>> null"); >>>>> - } >>>>> - this.LOG = log; >>>>> - } >>>>> - >>>>> /** >>>>> * Sets the criteria *if* it is set in the request parameters. >>>>> */ >>>>> >>>>> Modified: >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>> (original) >>>>> +++ >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java >>>>> Thu Aug 20 17:16:02 2015 >>>>> @@ -24,7 +24,8 @@ import java.util.regex.Pattern; >>>>> import java.util.regex.PatternSyntaxException; >>>>> import java.util.*; >>>>> import javax.servlet.http.HttpServletRequest; >>>>> -import org.apache.velocity.runtime.log.Log; >>>>> +import org.apache.commons.logging.Log; >>>>> +import org.apache.commons.logging.LogFactory; >>>>> import org.apache.velocity.tools.Scope; >>>>> import org.apache.velocity.tools.ConversionUtils; >>>>> import org.apache.velocity.tools.generic.FormatConfig; >>>>> @@ -73,8 +74,8 @@ import org.apache.velocity.tools.config. >>>>> public class BrowserTool extends FormatConfig implements >>>>> java.io.Serializable >>>>> { >>>>> private static final long serialVersionUID = >>>>> 1734529350532353339L; >>>>> - >>>>> - protected Log LOG; >>>>> + >>>>> + protected static final Log LOG = >>>>> LogFactory.getLog(BrowserTool.class); >>>>> /* User-Agent header variables */ >>>>> private String userAgent = null; >>>>> @@ -187,19 +188,6 @@ public class BrowserTool extends FormatC >>>>> } >>>>> /** >>>>> - * Set log. >>>>> - */ >>>>> - public void setLog(Log log) >>>>> - { >>>>> - if (log == null) >>>>> - { >>>>> - throw new NullPointerException("log should not be set to >>>>> null"); >>>>> - } >>>>> - this.LOG = log; >>>>> - } >>>>> - >>>>> - >>>>> - /** >>>>> * Sets the User-Agent string to be parsed for info. If null, >>>>> the >>>>> string >>>>> * will be empty and everything will return false or null. >>>>> Otherwise, >>>>> * it will set the whole string to lower case before storing to >>>>> simplify >>>>> >>>>> Modified: >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>> (original) >>>>> +++ >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java >>>>> Thu Aug 20 17:16:02 2015 >>>>> @@ -39,12 +39,17 @@ import javax.servlet.ServletOutputStream >>>>> import javax.servlet.http.HttpServletRequest; >>>>> import javax.servlet.http.HttpServletResponse; >>>>> import javax.servlet.http.HttpServletResponseWrapper; >>>>> -import org.apache.velocity.runtime.log.Log; >>>>> +import org.apache.commons.logging.Log; >>>>> +import org.apache.commons.logging.LogFactory; >>>>> + >>>>> /** >>>>> * <p>Provides methods to import arbitrary local or remote resources >>>>> as strings.</p> >>>>> * <p>Based on ImportSupport from the JSTL taglib by Shawn >>>>> Bayern</p> >>>>> * >>>>> + * <p>If you wish to take profit of your J2EE container session >>>>> serialization, >>>>> + * inheriting tools should always be in the request scope.</p> >>>>> + * >>>>> * @author <a href="mailto:mari...@centrum.is";>Marino A. >>>>> Jonsson</a> >>>>> * @since VelocityTools 2.0 >>>>> * @version $Revision$ $Date$ >>>>> @@ -57,7 +62,8 @@ public abstract class ImportSupport >>>>> /** Default character encoding for response. */ >>>>> protected static final String DEFAULT_ENCODING = "ISO-8859-1"; >>>>> - protected Log LOG; >>>>> + protected static final Log LOG = >>>>> LogFactory.getLog(ImportSupport.class); >>>>> + >>>>> protected ServletContext application; >>>>> protected HttpServletRequest request; >>>>> protected HttpServletResponse response; >>>>> @@ -65,15 +71,6 @@ public abstract class ImportSupport >>>>> // --------------------------------------- Setup Methods >>>>> ------------- >>>>> - public void setLog(Log log) >>>>> - { >>>>> - if (log == null) >>>>> - { >>>>> - throw new NullPointerException("log should not be set to >>>>> null"); >>>>> - } >>>>> - this.LOG = log; >>>>> - } >>>>> - >>>>> /** >>>>> * Sets the current {@link HttpServletRequest}. This is required >>>>> * for this tool to operate and will throw a >>>>> NullPointerException >>>>> >>>>> Modified: >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>> URL: >>>>> >>>>> http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff >>>>> >>>>> >>>>> ============================================================================== >>>>> --- >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>> (original) >>>>> +++ >>>>> >>>>> velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java >>>>> Thu Aug 20 17:16:02 2015 >>>>> @@ -36,7 +36,6 @@ public class ImportTool extends org.apac >>>>> setRequest(ctx.getRequest()); >>>>> setResponse(ctx.getResponse()); >>>>> setServletContext(ctx.getServletContext()); >>>>> - setLog(ctx.getVelocityEngine().getLog()); >>>>> } >>>>> } >>>>> } >>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org >>> For additional commands, e-mail: dev-h...@velocity.apache.org >>> >>> >>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org > For additional commands, e-mail: dev-h...@velocity.apache.org > >
