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...

[Claude Brisson]

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<cla...@renegat.net>
wrote:

It looks like that with slf4j, we could keep loggers injection 
even in
session tools, so no need to go static:

"As of SLF4J version 1.5.3, logger instances survive serialization.
Thus,
serialization of the host class no longer requires any special
action, even
when loggers are declared as instance variables. In previous 
versions,
logger instances needed to be declared as transient in the host 
class."
(http://www.slf4j.org/faq.html#declared_static  )

So it looks like the perfect solution, and will simplify many things.

    Claude



On 20/08/2015 21:32, Sergiu Dumitriu wrote:

A recent discussion was proposing to switch to slf4j as the façade.
On Aug 20, 2015 3:12 PM, "Claude Brisson"<cla...@renegat.net>  
wrote:

Thanks for the review.
Yes, we should be using Velocity logging facade, but I just don't
see for
now how we should do this while preserving tools serialization.

For now I just mimicked a behavior present in some deprecated 
classes,
re-introducing the dependency of velocity-tools-view on
commons-logging.

I agree that we should try to do better, but if a logger cannot be
statically reached from a session tool, and if it cannot be 
passed at
initialization because of serialization, there's a problem...

Let say for instance that we use transient weak references on
non-serializable Velocity loggers, fed up at initialization 
time. Then
the
tool could detect it has lost its logger at deserialization, and 
ask a
new
one, but to whom? It's a bad idea either to keep a static 
reference on
the
servlet context.

I'm open to suggestions...


     Claude


On 20/08/2015 20:45, Sergiu Dumitriu wrote:

Isn't the logging framework supposed to be pluggable, meaning 
that all
calls go through org.apache.velocity.runtime.log.Log which is then
passed to the actual logging library in use, be it log4j, slf4j,
jcl or
commons-logging?

On 08/20/2015 01:16 PM,cbris...@apache.org  wrote:

Author: cbrisson
Date: Thu Aug 20 17:16:02 2015
New Revision: 1696822

URL:http://svn.apache.org/r1696822
Log:
J2EE containers session serialization implies static logging from
session scoped tools

Modified:
velocity/tools/trunk/src/site/xdoc/changes.xml


velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java 




velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java 




velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java 




velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java 




velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java 



Modified: velocity/tools/trunk/src/site/xdoc/changes.xml
URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/src/site/xdoc/changes.xml?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


--- velocity/tools/trunk/src/site/xdoc/changes.xml (original)
+++ velocity/tools/trunk/src/site/xdoc/changes.xml Thu Aug 20
17:16:02
2015
@@ -35,6 +35,7 @@
             <li>Reflect Velocity Engine dependency shading of
commons-lang
and commons-collections (cb)</li>
             <li>Removed deprecated class
org.apache.velocity.tools.generic.log.LogSystemCommonsLog 
(cb)</li>
             <li>MathTool: fixed result type calculations and 
added
bitwise
operations (cb)</li>
+        <li>have session scoped tools use a static logger to fix
J2EE
containers session serialization (cb)</li>
             </ul>
         </subsection>

Modified:

velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java 


URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


---

velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java 


(original)
+++

velocity/tools/trunk/velocity-tools-struts/src/main/java/org/apache/velocity/tools/struts/TilesTool.java 


Thu Aug 20 17:16:02 2015
@@ -101,7 +101,6 @@ public class TilesTool extends ImportSup
                 setRequest(ctx.getRequest());
                 setResponse(ctx.getResponse());
setServletContext(ctx.getServletContext());
- setLog(ctx.getVelocityEngine().getLog());
             }
         }

Modified:

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java 


URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


---

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java 


(original)
+++

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/AbstractSearchTool.java 


Thu Aug 20 17:16:02 2015
@@ -22,7 +22,8 @@ package org.apache.velocity.tools.view;
     import java.util.Collections;
     import java.util.List;
     import javax.servlet.http.HttpServletRequest;
-import org.apache.velocity.runtime.log.Log;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
     import org.apache.velocity.tools.Scope;
     import org.apache.velocity.tools.config.DefaultKey;
     import org.apache.velocity.tools.config.InvalidScope;
@@ -129,19 +130,11 @@ public abstract class AbstractSearchTool
         protected static final String STORED_RESULTS_KEY =
             StoredResults.class.getName();
     -    protected Log LOG;
+    protected static final Log LOG =
LogFactory.getLog(AbstractSearchTool.class);
+
         private String criteriaKey = DEFAULT_CRITERIA_KEY;
         private Object criteria;
     -    public void setLog(Log log)
-    {
-        if (log == null)
-        {
-            throw new NullPointerException("log should not be
set to
null");
-        }
-        this.LOG = log;
-    }
-
         /**
          * Sets the criteria *if* it is set in the request
parameters.
          */

Modified:

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java 


URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


---

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java 


(original)
+++

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/BrowserTool.java 


Thu Aug 20 17:16:02 2015
@@ -24,7 +24,8 @@ import java.util.regex.Pattern;
     import java.util.regex.PatternSyntaxException;
     import java.util.*;
     import javax.servlet.http.HttpServletRequest;
-import org.apache.velocity.runtime.log.Log;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
     import org.apache.velocity.tools.Scope;
     import org.apache.velocity.tools.ConversionUtils;
     import org.apache.velocity.tools.generic.FormatConfig;
@@ -73,8 +74,8 @@ import org.apache.velocity.tools.config.
     public class BrowserTool extends FormatConfig implements
java.io.Serializable
     {
         private static final long serialVersionUID =
1734529350532353339L;
-
-    protected Log LOG;
+
+    protected static final Log LOG =
LogFactory.getLog(BrowserTool.class);
           /* User-Agent header variables */
         private String userAgent = null;
@@ -187,19 +188,6 @@ public class BrowserTool extends FormatC
         }
           /**
-     * Set log.
-     */
-    public void setLog(Log log)
-    {
-        if (log == null)
-        {
-            throw new NullPointerException("log should not be
set to
null");
-        }
-        this.LOG = log;
-    }
-
-
-    /**
          * Sets the User-Agent string to be parsed for info.  If
null,
the
string
          * will be empty and everything will return false or 
null.
Otherwise,
          * it will set the whole string to lower case before
storing to
simplify

Modified:

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java 


URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


---

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java 


(original)
+++

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/ImportSupport.java 


Thu Aug 20 17:16:02 2015
@@ -39,12 +39,17 @@ import javax.servlet.ServletOutputStream
     import javax.servlet.http.HttpServletRequest;
     import javax.servlet.http.HttpServletResponse;
     import javax.servlet.http.HttpServletResponseWrapper;
-import org.apache.velocity.runtime.log.Log;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
       /**
      * <p>Provides methods to import arbitrary local or remote
resources
as strings.</p>
      * <p>Based on ImportSupport from the JSTL taglib by Shawn
Bayern</p>
      *
+ * <p>If you wish to take profit of your J2EE container session
serialization,
+ *    inheriting tools should always be in the request 
scope.</p>
+ *
      * @author <a href="mailto:mari...@centrum.is";>Marino A.
Jonsson</a>
      * @since VelocityTools 2.0
      * @version $Revision$ $Date$
@@ -57,7 +62,8 @@ public abstract class ImportSupport
         /** Default character encoding for response. */
         protected static final String DEFAULT_ENCODING =
"ISO-8859-1";
     -    protected Log LOG;
+    protected static final Log LOG =
LogFactory.getLog(ImportSupport.class);
+
         protected ServletContext application;
         protected HttpServletRequest request;
         protected HttpServletResponse response;
@@ -65,15 +71,6 @@ public abstract class ImportSupport
           // --------------------------------------- Setup 
Methods
-------------
     -    public void setLog(Log log)
-    {
-        if (log == null)
-        {
-            throw new NullPointerException("log should not be
set to
null");
-        }
-        this.LOG = log;
-    }
-
         /**
          * Sets the current {@link HttpServletRequest}. This is
required
          * for this tool to operate and will throw a
NullPointerException

Modified:

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java 


URL:

http://svn.apache.org/viewvc/velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java?rev=1696822&r1=1696821&r2=1696822&view=diff 




============================================================================== 


---

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java 


(original)
+++

velocity/tools/trunk/velocity-tools-view/src/main/java/org/apache/velocity/tools/view/tools/ImportTool.java 


Thu Aug 20 17:16:02 2015
@@ -36,7 +36,6 @@ public class ImportTool extends org.apac
                 setRequest(ctx.getRequest());
                 setResponse(ctx.getResponse());
setServletContext(ctx.getServletContext());
- setLog(ctx.getVelocityEngine().getLog());
             }
         }
     }



--------------------------------------------------------------------- 


To unsubscribe, e-mail:dev-unsubscr...@velocity.apache.org
For additional commands, e-mail:dev-h...@velocity.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail:dev-unsubscr...@velocity.apache.org
For additional commands, e-mail:dev-h...@velocity.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org