You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Sergiu Dumitriu <se...@gmail.com> on 2015/08/20 20:45:51 UTC
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
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, cbrisson@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:marinoj@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());
> }
> }
> }
>
>
--
Sergiu Dumitriu
http://purl.org/net/sergiu/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Nathan Bubna <nb...@gmail.com>.
Sounds reasonable to me. Thanks for tackling this, Claude!
On Sun, Aug 30, 2015 at 9:07 AM, Claude Brisson <cl...@renegat.net> wrote:
> Some remarks after a little investigation.
>
> 1) Using the ServletContext logger doesn't seem to be a widespread
> practice among slf4j folks. There is a slf4j-servletcontext implementation
> on github, but it is not referenced on the official slf4j site. Moreover, I
> tested it, and it does not support deserialization (I had some doubts about
> it, because deserialization may happen before ServletContext
> initialization), so it cannot be used as is. Plus, it needs to be declared
> as context's listener in the deployment file.
>
> It's possible to solve those two problems (alas not from our own codebase):
> - The logger object can be a proxy to a transcient implementation that
> waits for the servlet context to be initialized.
> - Since J2EE API v3.0, there is no need to declare the listener in the
> deployment descriptor, as it can be done using Java annotations, which
> would fit slf4j 'jar'-only binding strategy far more than the current state
> of the project.
>
> But not using the ServletContext logger is not necessarily a bad idea. Of
> course, "it's here", so why not use it, but:
> - In practice, container logger and webapp logger serve rather different
> purposes, as container logs won't be active but at initialization or
> destruction.
> - They're not formatted the same way.
> - It's not so cumbersome to watch several log files at once (I mean, on a
> real O.S. with a proper command line, of course) when you need to see both
> at the same time.
> - It's much easier to configure a per-webapp logging (and consequently to
> ease webapp deployment).
> - Gathering logs is always doable at the container level, which can also
> use slf4j (or one of its many bridges), bypassing ServletContext (which is
> just a proxy).
>
> Anyhow, even it's a little drawback for some people, it can not weight as
> much in the balance as being freed of all logging
>
> 2) We can -and should- keep our logger injection mechanism, since it not
> only helps separating different instances logs, but also permits unifying
> log line prefixes. No more need to define the logger in
> velocity.properties, by default it will be a static logger named
> "Velocity". The only constraint when trying to separate instances logs is
> to explicitly give different names to each logger (since the underlying
> LoggerFactory mechanism remains static). Not a big deal.
>
> Without any other objection, I'll start this drastic simplification very
> soon. We don't even need to keep the o.a.v.runtime.log package. And four
> sub-modules can disappear...
>
> Claude
>
>
> On 21/08/2015 17:25, Nathan Bubna wrote:
>
>> I like simple. :)
>>
>> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cl...@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"<cl...@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,cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:dev-unsubscribe@velocity.apache.org
>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
I should add that it implies shipping a new major version for
velocity-tools. I'm not against it, as we can take the opportunity to
drop the deprecated package o.a.v.tools.view.tools.
Also, do we maintain the maven-velocity-tools-plugin subproject? Right
now, it fails with:
[ERROR] Failed to parse plugin descriptor for
org.apache.velocity:maven-velocity-tools-plugin:2.1.0-SNAPSHOT
(/home/claude/projects/velocity/tools/trunk/maven-velocity-tools-plugin/target/classes):
No plugin descriptor found at META-INF/maven/plugin.xml -> [Help 1]
org.apache.maven.plugin.PluginDescriptorParsingException: Failed to
parse plugin descriptor for
org.apache.velocity:maven-velocity-tools-plugin:2.1.0-SNAPSHOT
(/home/claude/projects/velocity/tools/trunk/maven-velocity-tools-plugin/target/classes):
No plugin descriptor found at META-INF/maven/plugin.xml
at
org.apache.maven.plugin.internal.DefaultMavenPluginManager.extractPluginDescriptor(DefaultMavenPluginManager.java:249)
at
org.apache.maven.plugin.internal.DefaultMavenPluginManager.getPluginDescriptor(DefaultMavenPluginManager.java:184)
at
org.apache.maven.plugin.internal.DefaultMavenPluginManager.getMojoDescriptor(DefaultMavenPluginManager.java:298)
at
org.apache.maven.plugin.DefaultBuildPluginManager.getMojoDescriptor(DefaultBuildPluginManager.java:241)
[...]
I personally don't know at all how to fix that, and don't want to learn
how...
My proposal is to drop it, and if someone is interested in maintaining
it, then he can fetch it back from svn.
Claude
On 31/08/2015 07:16, Claude Brisson wrote:
> Of course: switching the log system to slf4j, which means:
>
> - getting rid of the org.apache.velocity.runtime.log package
>
> - kepping loggers injection as it is, but switch to org.slf4j.Logger
>
> - getting rid of the four log-realted sub-modules, that is
> velocity-engine-commons-logging, velocity-engine-log4j,
> velocity-engine-servlet, velocity-engine-slf4j
>
> - also getting rid of velocity-engine-uberjar, which isn't of any
> more use
>
> - replacing the two configuration keys runtime.log.logsystem and
> runtime.log.logsystem.class with:
> x runtime.log.instance (when the user wants to programmatically
> give to Velocity an existing instance of an org.slf4j.Logger)
> x runtime.log.name (when the user wants to give to Velocity with a
> named org.slf4j.Logger, like from inside velocity.properties)
>
> - letting the default logger be obtained via
> LoggerFactory.getLogger(Velocity.class) (which will prefix logs with
> the "Velocity" string)
>
> And of course, thereafter, reflecting this change on velocity-tools
> (not sure I'll handle other subprojects).
>
>
> Claude
>
> On 31/08/2015 03:44, Sergiu Dumitriu wrote:
>> Sorry, but I'm not sure I understand what you're proposing. Can you make
>> a short summary?
>>
>> On 08/30/2015 12:07 PM, Claude Brisson wrote:
>>> Some remarks after a little investigation.
>>>
>>> 1) Using the ServletContext logger doesn't seem to be a widespread
>>> practice among slf4j folks. There is a slf4j-servletcontext
>>> implementation on github, but it is not referenced on the official
>>> slf4j
>>> site. Moreover, I tested it, and it does not support deserialization (I
>>> had some doubts about it, because deserialization may happen before
>>> ServletContext initialization), so it cannot be used as is. Plus, it
>>> needs to be declared as context's listener in the deployment file.
>>>
>>> It's possible to solve those two problems (alas not from our own
>>> codebase):
>>> - The logger object can be a proxy to a transcient implementation
>>> that
>>> waits for the servlet context to be initialized.
>>> - Since J2EE API v3.0, there is no need to declare the listener in
>>> the
>>> deployment descriptor, as it can be done using Java annotations, which
>>> would fit slf4j 'jar'-only binding strategy far more than the current
>>> state of the project.
>>>
>>> But not using the ServletContext logger is not necessarily a bad idea.
>>> Of course, "it's here", so why not use it, but:
>>> - In practice, container logger and webapp logger serve rather
>>> different purposes, as container logs won't be active but at
>>> initialization or destruction.
>>> - They're not formatted the same way.
>>> - It's not so cumbersome to watch several log files at once (I
>>> mean, on
>>> a real O.S. with a proper command line, of course) when you need to see
>>> both at the same time.
>>> - It's much easier to configure a per-webapp logging (and
>>> consequently
>>> to ease webapp deployment).
>>> - Gathering logs is always doable at the container level, which can
>>> also use slf4j (or one of its many bridges), bypassing ServletContext
>>> (which is just a proxy).
>>>
>>> Anyhow, even it's a little drawback for some people, it can not weight
>>> as much in the balance as being freed of all logging
>>>
>>> 2) We can -and should- keep our logger injection mechanism, since it
>>> not
>>> only helps separating different instances logs, but also permits
>>> unifying log line prefixes. No more need to define the logger in
>>> velocity.properties, by default it will be a static logger named
>>> "Velocity". The only constraint when trying to separate instances logs
>>> is to explicitly give different names to each logger (since the
>>> underlying LoggerFactory mechanism remains static). Not a big deal.
>>>
>>> Without any other objection, I'll start this drastic simplification
>>> very
>>> soon. We don't even need to keep the o.a.v.runtime.log package. And
>>> four
>>> sub-modules can disappear...
>>>
>>> Claude
>>>
>>>
>>> On 21/08/2015 17:25, Nathan Bubna wrote:
>>>> I like simple. :)
>>>>
>>>> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cl...@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"<cl...@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,cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>>>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>>>>
>>>>>>>
>>>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail:dev-unsubscribe@velocity.apache.org
>>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>>
>>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
>>> For additional commands, e-mail: dev-help@velocity.apache.org
>>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
Of course: switching the log system to slf4j, which means:
- getting rid of the org.apache.velocity.runtime.log package
- kepping loggers injection as it is, but switch to org.slf4j.Logger
- getting rid of the four log-realted sub-modules, that is
velocity-engine-commons-logging, velocity-engine-log4j,
velocity-engine-servlet, velocity-engine-slf4j
- also getting rid of velocity-engine-uberjar, which isn't of any more use
- replacing the two configuration keys runtime.log.logsystem and
runtime.log.logsystem.class with:
x runtime.log.instance (when the user wants to programmatically give
to Velocity an existing instance of an org.slf4j.Logger)
x runtime.log.name (when the user wants to give to Velocity with a
named org.slf4j.Logger, like from inside velocity.properties)
- letting the default logger be obtained via
LoggerFactory.getLogger(Velocity.class) (which will prefix logs with the
"Velocity" string)
And of course, thereafter, reflecting this change on velocity-tools (not
sure I'll handle other subprojects).
Claude
On 31/08/2015 03:44, Sergiu Dumitriu wrote:
> Sorry, but I'm not sure I understand what you're proposing. Can you make
> a short summary?
>
> On 08/30/2015 12:07 PM, Claude Brisson wrote:
>> Some remarks after a little investigation.
>>
>> 1) Using the ServletContext logger doesn't seem to be a widespread
>> practice among slf4j folks. There is a slf4j-servletcontext
>> implementation on github, but it is not referenced on the official slf4j
>> site. Moreover, I tested it, and it does not support deserialization (I
>> had some doubts about it, because deserialization may happen before
>> ServletContext initialization), so it cannot be used as is. Plus, it
>> needs to be declared as context's listener in the deployment file.
>>
>> It's possible to solve those two problems (alas not from our own codebase):
>> - The logger object can be a proxy to a transcient implementation that
>> waits for the servlet context to be initialized.
>> - Since J2EE API v3.0, there is no need to declare the listener in the
>> deployment descriptor, as it can be done using Java annotations, which
>> would fit slf4j 'jar'-only binding strategy far more than the current
>> state of the project.
>>
>> But not using the ServletContext logger is not necessarily a bad idea.
>> Of course, "it's here", so why not use it, but:
>> - In practice, container logger and webapp logger serve rather
>> different purposes, as container logs won't be active but at
>> initialization or destruction.
>> - They're not formatted the same way.
>> - It's not so cumbersome to watch several log files at once (I mean, on
>> a real O.S. with a proper command line, of course) when you need to see
>> both at the same time.
>> - It's much easier to configure a per-webapp logging (and consequently
>> to ease webapp deployment).
>> - Gathering logs is always doable at the container level, which can
>> also use slf4j (or one of its many bridges), bypassing ServletContext
>> (which is just a proxy).
>>
>> Anyhow, even it's a little drawback for some people, it can not weight
>> as much in the balance as being freed of all logging
>>
>> 2) We can -and should- keep our logger injection mechanism, since it not
>> only helps separating different instances logs, but also permits
>> unifying log line prefixes. No more need to define the logger in
>> velocity.properties, by default it will be a static logger named
>> "Velocity". The only constraint when trying to separate instances logs
>> is to explicitly give different names to each logger (since the
>> underlying LoggerFactory mechanism remains static). Not a big deal.
>>
>> Without any other objection, I'll start this drastic simplification very
>> soon. We don't even need to keep the o.a.v.runtime.log package. And four
>> sub-modules can disappear...
>>
>> Claude
>>
>>
>> On 21/08/2015 17:25, Nathan Bubna wrote:
>>> I like simple. :)
>>>
>>> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cl...@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"<cl...@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,cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>>>
>>>>>>
>>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail:dev-unsubscribe@velocity.apache.org
>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>
>>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
>> For additional commands, e-mail: dev-help@velocity.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Sergiu Dumitriu <se...@gmail.com>.
Sorry, but I'm not sure I understand what you're proposing. Can you make
a short summary?
On 08/30/2015 12:07 PM, Claude Brisson wrote:
> Some remarks after a little investigation.
>
> 1) Using the ServletContext logger doesn't seem to be a widespread
> practice among slf4j folks. There is a slf4j-servletcontext
> implementation on github, but it is not referenced on the official slf4j
> site. Moreover, I tested it, and it does not support deserialization (I
> had some doubts about it, because deserialization may happen before
> ServletContext initialization), so it cannot be used as is. Plus, it
> needs to be declared as context's listener in the deployment file.
>
> It's possible to solve those two problems (alas not from our own codebase):
> - The logger object can be a proxy to a transcient implementation that
> waits for the servlet context to be initialized.
> - Since J2EE API v3.0, there is no need to declare the listener in the
> deployment descriptor, as it can be done using Java annotations, which
> would fit slf4j 'jar'-only binding strategy far more than the current
> state of the project.
>
> But not using the ServletContext logger is not necessarily a bad idea.
> Of course, "it's here", so why not use it, but:
> - In practice, container logger and webapp logger serve rather
> different purposes, as container logs won't be active but at
> initialization or destruction.
> - They're not formatted the same way.
> - It's not so cumbersome to watch several log files at once (I mean, on
> a real O.S. with a proper command line, of course) when you need to see
> both at the same time.
> - It's much easier to configure a per-webapp logging (and consequently
> to ease webapp deployment).
> - Gathering logs is always doable at the container level, which can
> also use slf4j (or one of its many bridges), bypassing ServletContext
> (which is just a proxy).
>
> Anyhow, even it's a little drawback for some people, it can not weight
> as much in the balance as being freed of all logging
>
> 2) We can -and should- keep our logger injection mechanism, since it not
> only helps separating different instances logs, but also permits
> unifying log line prefixes. No more need to define the logger in
> velocity.properties, by default it will be a static logger named
> "Velocity". The only constraint when trying to separate instances logs
> is to explicitly give different names to each logger (since the
> underlying LoggerFactory mechanism remains static). Not a big deal.
>
> Without any other objection, I'll start this drastic simplification very
> soon. We don't even need to keep the o.a.v.runtime.log package. And four
> sub-modules can disappear...
>
> Claude
>
>
> On 21/08/2015 17:25, Nathan Bubna wrote:
>> I like simple. :)
>>
>> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cl...@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"<cl...@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,cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>>
>>>>>
>>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:dev-unsubscribe@velocity.apache.org
>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>
>>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
--
Sergiu Dumitriu
http://purl.org/net/sergiu/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
Some remarks after a little investigation.
1) Using the ServletContext logger doesn't seem to be a widespread
practice among slf4j folks. There is a slf4j-servletcontext
implementation on github, but it is not referenced on the official slf4j
site. Moreover, I tested it, and it does not support deserialization (I
had some doubts about it, because deserialization may happen before
ServletContext initialization), so it cannot be used as is. Plus, it
needs to be declared as context's listener in the deployment file.
It's possible to solve those two problems (alas not from our own codebase):
- The logger object can be a proxy to a transcient implementation that
waits for the servlet context to be initialized.
- Since J2EE API v3.0, there is no need to declare the listener in the
deployment descriptor, as it can be done using Java annotations, which
would fit slf4j 'jar'-only binding strategy far more than the current
state of the project.
But not using the ServletContext logger is not necessarily a bad idea.
Of course, "it's here", so why not use it, but:
- In practice, container logger and webapp logger serve rather
different purposes, as container logs won't be active but at
initialization or destruction.
- They're not formatted the same way.
- It's not so cumbersome to watch several log files at once (I mean,
on a real O.S. with a proper command line, of course) when you need to
see both at the same time.
- It's much easier to configure a per-webapp logging (and consequently
to ease webapp deployment).
- Gathering logs is always doable at the container level, which can
also use slf4j (or one of its many bridges), bypassing ServletContext
(which is just a proxy).
Anyhow, even it's a little drawback for some people, it can not weight
as much in the balance as being freed of all logging
2) We can -and should- keep our logger injection mechanism, since it not
only helps separating different instances logs, but also permits
unifying log line prefixes. No more need to define the logger in
velocity.properties, by default it will be a static logger named
"Velocity". The only constraint when trying to separate instances logs
is to explicitly give different names to each logger (since the
underlying LoggerFactory mechanism remains static). Not a big deal.
Without any other objection, I'll start this drastic simplification very
soon. We don't even need to keep the o.a.v.runtime.log package. And four
sub-modules can disappear...
Claude
On 21/08/2015 17:25, Nathan Bubna wrote:
> I like simple. :)
>
> On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson<cl...@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"<cl...@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,cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>>> For additional commands, e-mail:dev-help@velocity.apache.org
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:dev-unsubscribe@velocity.apache.org
>> For additional commands, e-mail:dev-help@velocity.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Nathan Bubna <nb...@gmail.com>.
I like simple. :)
On Fri, Aug 21, 2015 at 2:18 AM, Claude Brisson <cl...@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" <cl...@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, cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>>> For additional commands, e-mail: dev-help@velocity.apache.org
>>>
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
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" <cl...@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, cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
>> For additional commands, e-mail: dev-help@velocity.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Sergiu Dumitriu <se...@gmail.com>.
A recent discussion was proposing to switch to slf4j as the façade.
On Aug 20, 2015 3:12 PM, "Claude Brisson" <cl...@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, cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
> For additional commands, e-mail: dev-help@velocity.apache.org
>
>
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
On 31/08/2015 19:52, Christopher Schultz wrote:
> Mike,
>
> On 8/31/15 1:48 PM, Mike Kienenberger wrote:
>> Read the whole thread.
> Yeah, I'm catching-up now.
>
>> My understanding is that serialization of static commons-logging was
>> the original problem needing fixed. slf4j doesn't have that issue.
> I didn't think that static members were serialized. I suppose it depends
> upon how the logging framework is being used. I rarely if ever use
> non-static logging references, but of course I can't control everyone
> else's coding styles :)
Yes, the problem is the serialization of non-static members.
Christopher, if you use Velocity, you do use non-static logging
references, at least indirectly, since Velocity does use them :-)
Static logging has the merit of simplicity, I often use it, but it poses
several limits. Using loggers injection has many advantages, like
allowing separate Velocity instances to use distinct loggers (note that
there are dirty classloader tricks to try to do that with static
references, which we don't want), allowing loggers mocking and capturing
features for testcases, better control over initialization order, and
certainly other advantages.
Claude
> -chris
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mike,
On 8/31/15 1:48 PM, Mike Kienenberger wrote:
> Read the whole thread.
Yeah, I'm catching-up now.
> My understanding is that serialization of static commons-logging was
> the original problem needing fixed. slf4j doesn't have that issue.
I didn't think that static members were serialized. I suppose it depends
upon how the logging framework is being used. I rarely if ever use
non-static logging references, but of course I can't control everyone
else's coding styles :)
-chris
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Mike Kienenberger <mk...@gmail.com>.
Chris,
Read the whole thread. My understanding is that serialization of
static commons-logging was the original problem needing fixed. slf4j
doesn't have that issue.
On Mon, Aug 31, 2015 at 1:41 PM, Christopher Schultz
<ch...@christopherschultz.net> wrote:
> Claude,
>
> On 8/20/15 3:12 PM, Claude Brisson wrote:
>> 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.
>
> A little late to the (logging) party, and this may be a bit off-topic,
> but why bother using a Velocity-specific Logging facade when
> commons-logging was built for that purpose and it is already pluggable?
>
> Frederic Brier posted earlier in the summer about /removing/
> commons-logging, so there's obviously some contention on how this all
> ought to be done.
>
> -chris
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/
velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Claude,
On 8/20/15 3:12 PM, Claude Brisson wrote:
> 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.
A little late to the (logging) party, and this may be a bit off-topic,
but why bother using a Velocity-specific Logging facade when
commons-logging was built for that purpose and it is already pluggable?
Frederic Brier posted earlier in the summer about /removing/
commons-logging, so there's obviously some contention on how this all
ought to be done.
-chris
Re: svn commit: r1696822 - in /velocity/tools/trunk: src/site/xdoc/
velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/ velocity-tools-view/src/main/java/org/apache/velocity/tools/view/
velocity-tools-view/src/main/java/org/apache/velocit...
Posted by Claude Brisson <cl...@renegat.net>.
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, cbrisson@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:marinoj@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-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org