[engine] Review VELOCITY-926 fix:

deprecate velocimacro.arguments.preserve_literals in favor of the new velocimacro.enable_bc_mode flag,
which mimics 1.7 velocimacro behavior:

- preserve arguments literals:

    #macro(m $arg) $arg #end
    m($null)

will displays $arg w/o bc mode and $null with bc mode

- use global defaults for missing arguments:

    #macro(m $foo) $foo #end
    #set($foo='foo')
    #m()

will display $foo w/o bc mode and 'foo' with bc mode

The following use cases have been left aside from backward compatibility:

- preserving local macro scope values

    #macro(test) #set($foo = 'foo') $some_tool.change_foo_value_in_global_context_to_bar() $foo #end
    #test()

will always display 'bar', while 1.7 displayed 'foo'

- setting a null argument to null, while argument name exists in context, 

    #macro(setnull $foo) #set($foo = $null) #end
    #set($foo='foo')
    #setnull($null)
    $foo

Will always display 'foo' (w or w/o bc mode), while 1.7 did display $foo




git-svn-id: https://svn.apache.org/repos/asf/velocity/engine/trunk@1873244 13f79535-47bb-0310-9956-ffa450edef68

4 files changed, 49 insertions, 20 deletions

diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
index 2d876919..3789a76e 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/DeprecatedRuntimeConstants.java
@@ -274,4 +274,12 @@ public interface DeprecatedRuntimeConstants
      */
     String OLD_SPACE_GOBBLING = "space.gobbling";
 
+    /**
+     * When displaying null or invalid non-quiet references, use the argument literal reference
+     * instead of the one in the macro block. Defaults to false.
+     * @since 2.1
+     * @Deprecated since 2.2, see {@link RuntimeConstants#VM_ENABLE_BC_MODE}
+     **/
+    String OLD_VM_ENABLE_BC_MODE = "velocimacro.arguments.preserve_literals";
+
 }
diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
index 9b640e8d..89d3c41f 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/RuntimeConstants.java
@@ -331,11 +331,21 @@ public interface RuntimeConstants extends DeprecatedRuntimeConstants
     String VM_ARGUMENTS_STRICT = "velocimacro.arguments.strict";
 
     /**
-     * When displaying null or invalid non-quiet references, use the argument literal reference
-     * instead of the one in the macro block. Defaults to false.
-     * @since 2.1
-     **/
-    String VM_PRESERVE_ARGUMENTS_LITERALS = "velocimacro.arguments.preserve_literals";
+     * This flag enable the 1.7 backward compatible mode for velocimacros (defaults to false):
+     * <ul>
+     *     <li>
+     *         preserve argument literals: when displaying null or invalid non-quiet references,
+     *         use the argument literal reference instead of the one in the macro block. Defaults to false.
+     *     </li>
+     *     <li>
+     *         use global values for missing arguments: when calling a macro with fewer arguments than declared,
+     *         if those arguments don't have an explicit default value in the macro definition, default values will
+     *         be looked for in the global context
+     *     </li>
+     * </ul>
+     * @since 2.2
+     */
+    String VM_ENABLE_BC_MODE = "velocimacro.enable_bc_mode";
 
     /**
      * Specify the maximum depth for macro calls
diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
index e8467fcf..ee4bddd3 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
@@ -56,7 +56,7 @@ public class VelocimacroProxy extends Directive
     private boolean strictArguments;
     private int maxCallDepth;
     private String bodyReference;
-    private boolean preserveArgumentsLiterals;
+    private boolean enableBCmode;
 
     private static final Object NULL_VALUE_MARKER = new Object();
 
@@ -99,7 +99,7 @@ public class VelocimacroProxy extends Directive
 
         // for performance reasons we precache these strings - they are needed in
         // "render literal if null" functionality
-        if (preserveArgumentsLiterals)
+        if (enableBCmode)
         {
             literalArgArray = new String[macroArgs.size()];
             for (int i = 0; i < macroArgs.size(); i++)
@@ -160,7 +160,7 @@ public class VelocimacroProxy extends Directive
         // get name of the reference that refers to AST block passed to block macro call
         bodyReference = rsvc.getString(RuntimeConstants.VM_BODY_REFERENCE, "bodyContent");
 
-        preserveArgumentsLiterals = rsvc.getBoolean(RuntimeConstants.VM_PRESERVE_ARGUMENTS_LITERALS, false);
+        enableBCmode = rsvc.getBoolean(RuntimeConstants.VM_ENABLE_BC_MODE, false);
     }
 
     /**
@@ -247,9 +247,10 @@ public class VelocimacroProxy extends Directive
             {
                 MacroArg macroArg = macroArgs.get(i);
                 current = context.get(macroArg.name);
-                if (current == values[(i-1) * 2 + 1])
+                Object given = values[(i-1) * 2 + 1];
+                Object old = values[(i-1) * 2];
+                if (current == given || current == null && given == NULL_VALUE_MARKER)
                 {
-                    Object old = values[(i-1) * 2];
                     if (old == null)
                     {
                         context.remove(macroArg.name);
@@ -264,11 +265,11 @@ public class VelocimacroProxy extends Directive
                     }
                 }
 
-                if (preserveArgumentsLiterals)
+                if (enableBCmode)
                 {
                     /* allow for nested calls */
                     Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
-                    if (literalsStack != null) /* may be null if argument was missing in macro call */
+                    if (literalsStack != null) /* shouldn't be null */
                     {
                         literalsStack.removeFirst();
                         if (literalsStack.size() == 0)
@@ -354,6 +355,8 @@ public class VelocimacroProxy extends Directive
     	// Changed two dimensional array to single dimensional to optimize memory lookups
         Object[] values = new Object[macroArgs.size() * 2];
 
+        boolean warnedMissingArguments = false;
+
         // Move arguments into the macro's context. Start at one to skip macro name
         for (int i = 1; i < macroArgs.size(); i++)
         {
@@ -394,18 +397,26 @@ public class VelocimacroProxy extends Directive
             }
             else
             {
-                // Backward compatibility logging, Mainly for MacroForwardDefinedTestCase
-                log.debug("VM #{}: too few arguments to macro. Wanted {} got {}",
-                          macroArgs.get(0).name, macroArgs.size() - 1, callArgNum);
-                break;
+                if (!warnedMissingArguments)
+                {
+                    // Backward compatibility logging, Mainly for MacroForwardDefinedTestCase
+                    log.debug("VM #{}: too few arguments to macro. Wanted {} got {}",
+                        macroArgs.get(0).name, macroArgs.size() - 1, callArgNum);
+                    warnedMissingArguments = true;
+                }
+                if (enableBCmode)
+                {
+                    // use the global context value as default
+                    newVal = oldVal;
+                }
             }
 
             values[(i-1) * 2 + 1] = newVal;
 
-            /* when preserveArgumentsLiterals is true, we still store the actual reference passed to the macro
+            /* when enableBCmode is true, we still store the actual reference passed to the macro
                even if the value is not null, because *if* the argument is set to null *during* the macro rendering
                we still expect the passed argument literal to be displayed to be fully backward compatible. */
-            if (preserveArgumentsLiterals && /* newVal == null && */ argNode != null)
+            if (enableBCmode && /* newVal == null && */ argNode != null)
             {
                 /* allow nested macro calls for B.C. */
                 Deque<String> literalsStack = (Deque<String>)context.get(literalArgArray[i]);
@@ -415,7 +426,7 @@ public class VelocimacroProxy extends Directive
                     context.put(literalArgArray[i], literalsStack);
                 }
                 /* Reflects the strange 1.7 behavor... */
-                if (argNode instanceof ASTReference || argNode instanceof ASTStringLiteral)
+                if (argNode != null && (argNode instanceof ASTReference || argNode instanceof ASTStringLiteral))
                 {
                     literalsStack.addFirst(argNode.literal());
                 }
diff --git a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
index 527de725..747780dd 100644
--- a/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
+++ b/velocity-engine-core/src/main/java/org/apache/velocity/runtime/parser/node/ASTReference.java
@@ -157,7 +157,7 @@ public class ASTReference extends SimpleNode
 
         strictEscape = rsvc.getBoolean(RuntimeConstants.RUNTIME_REFERENCES_STRICT_ESCAPE, false);
         strictRef = rsvc.getBoolean(RuntimeConstants.RUNTIME_REFERENCES_STRICT, false);
-        lookupAlternateLiteral = rsvc.getBoolean(RuntimeConstants.VM_PRESERVE_ARGUMENTS_LITERALS, false);
+        lookupAlternateLiteral = rsvc.getBoolean(RuntimeConstants.VM_ENABLE_BC_MODE, false);
 
         /*
          *  the only thing we can do in init() is getRoot()