Yeah, everyone's vote matters. Besides, i lean towards 1 as well, for
the "option soup" reason.
I don't know that 630 is any more likely than 285, but i think we
should default to being strict about pass by name, since that is the
set pattern.
By the way, the "suggestion box" for 2.0 is JIRA. No better place to
track such ideas and arguments. :)
On Wed, Oct 15, 2008 at 3:57 PM, Byron Foster <[EMAIL PROTECTED]> wrote:
>
> Hmm, well if it means anything I would vote for 1. I think adding another
> config option is contributing to option soup. And I think the majority of
> users, even experienced users aren't really going to want to consider the
> implications of strict pass by name. With 1) there is an option for those
> who are really effected by 285.
>
> Between 285 and 630, I think 630 is going to hit users more, even though I
> agree 285 is not desirable.
>
> If I could drop a note into the suggestion box for 2.0 I would say that pass
> by value should be the default, with pass by name being the exception. 285
> shows that people intuitively assume pass by value behavior. There is clear
> utility in being able to pass by name, but a developer should specify pass by
> name because they know why they want to use it, and have considered the
> implications.
>
> Additionally, there are performance gains to be realized with pass by value.
> Currently, All references in a macro are resolved by searching up the macro
> "stack", or context chain. In fact, even pass by name could be passed by
> value since ASTReference could simply look to see if a value is an
> ASTprocess, and if so execute it. A specialized context for storing ASTs
> would not be necessary.
>
> While I'm rambling... I would also make macros local scope, and create a
> global directive. Pass by name would work with local scope, which is
> currently not the case. I think all of this would lead to more intuitive
> behavior that could be clearly described in the Velocity docs, and that users
> would not find surprising.
>
> On Oct 15, 2008, at 11:01 , Nathan Bubna wrote:
>
>> ok, so here's my current thought. since we've already changed this in
>> the last production release, i'm hesitant to just "change our mind"
>> and leave the VELOCITY-285 advocates hanging. so, here's two options
>> i think i'd be comfortable with:
>>
>> 1:
>> Index: ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> ===================================================================
>> --- ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> (revision 701169)
>> +++ ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> (working copy)
>> @@ -247,6 +247,15 @@
>> }
>>
>> /**
>> + * Extension hook to allow VELOCITY-285 behavior by overriding
>> + * and using context.localPut(key, value). See VELOCITY-630.
>> + */
>> + protected void put(InternalContextAdapter context, String key,
>> Object value)
>> + {
>> + context.put(key, value);
>> + }
>> +
>> + /**
>> * renders the #foreach() block
>> * @param context
>> * @param writer
>> @@ -325,11 +334,11 @@
>>
>> while (!maxNbrLoopsExceeded && i.hasNext())
>> {
>> - // TODO: JDK 1.4+ -> Integer.valueOf()
>> - context.localPut(counterName , new Integer(counter));
>> - context.localPut(hasNextName, Boolean.valueOf(i.hasNext()));
>> + // TODO: JDK 1.5+ -> Integer.valueOf()
>> + put(context, counterName , new Integer(counter));
>> + put(context, hasNextName, Boolean.valueOf(i.hasNext()));
>> Object value = i.next();
>> - context.localPut(elementKey, value);
>> + put(context, elementKey, value);
>>
>> or 2:
>> Index: ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> ===================================================================
>> --- ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> (revision 701169)
>> +++ ../src/java/org/apache/velocity/runtime/directive/Foreach.java
>> (working copy)
>> @@ -177,6 +177,11 @@
>> private boolean skipInvalidIterator;
>>
>> /**
>> + * Whether or not [EMAIL PROTECTED] #put} uses context.put or
>> context.localPut
>> + */
>> + private boolean strictPassByName;
>> +
>> + /**
>> * The reference name used to access each
>> * of the elements in the list object. It
>> * is the $item in the following:
>> @@ -217,6 +222,7 @@
>> }
>> skipInvalidIterator =
>> rsvc.getBoolean(RuntimeConstants.SKIP_INVALID_ITERATOR, true);
>> + strictPassByName =
>> rsvc.getBoolean(RuntimeConstants.STRICT_FOREACH_PASS_BY_NAME, true);
>>
>> /*
>> * this is really the only thing we can do here as everything
>> @@ -247,6 +253,22 @@
>> }
>>
>> /**
>> + * Controls whether loop vars are set locally or generally. See
>> VELOCITY-630
>> + * and VELOCITY-285.
>> + */
>> + protected void put(InternalContextAdapter context, String key,
>> Object value)
>> + {
>> + if (strictPassByName)
>> + {
>> + context.put(key, value);
>> + }
>> + else
>> + {
>> + context.localPut(key, value);
>> + }
>> + }
>> +
>> + /**
>> * renders the #foreach() block
>> * @param context
>> * @param writer
>> @@ -325,11 +347,11 @@
>>
>> while (!maxNbrLoopsExceeded && i.hasNext())
>> {
>> - // TODO: JDK 1.4+ -> Integer.valueOf()
>> - context.localPut(counterName , new Integer(counter));
>> - context.localPut(hasNextName, Boolean.valueOf(i.hasNext()));
>> + // TODO: JDK 1.5+ -> Integer.valueOf()
>> + put(context, counterName , new Integer(counter));
>> + put(context, hasNextName, Boolean.valueOf(i.hasNext()));
>> Object value = i.next();
>> - context.localPut(elementKey, value);
>> + put(context, elementKey, value);
>>
>> On Tue, Oct 14, 2008 at 9:40 PM, Nathan Bubna <[EMAIL PROTECTED]> wrote:
>>>
>>> hmm. i suppose that makes sense... of course, i'm too tired to think
>>> straight right now. let me re-examine this tomorrow.
>>>
>>> On Tue, Oct 14, 2008 at 6:39 PM, Byron Foster <[EMAIL PROTECTED]> wrote:
>>>>
>>>> So I looked into this some more and here is the deal. This bug was
>>>> introduced with VELOCITY-285. The solution to 285 was to make the #foreach
>>>> index variable forced to local scope, which introduced bug VELOCITY-630.
>>>> However, I assert that 285 was never a bug!
>>>>
>>>> Given the following VTL:
>>>>
>>>> #macro(dig $x)
>>>> x1: $x
>>>> #foreach($i in $x)
>>>> x2 $x
>>>> #dig($i)
>>>> #end
>>>> #end
>>>> #dig([[1,2],[3,4]])
>>>>
>>>> The premiss of 285 is that reference $x at x1: and x2: should render with
>>>> the same value. However, in a pass by name world this is not true. On at
>>>> least the second recursion level to #dig the reference $x is essentially an
>>>> alias for $i. Since $i changes value between x1 and x2, rendering $x will
>>>> change.
>>>>
>>>> I agree with the sentiment that this is surprising, but it is working as it
>>>> should.
>>>>
>>>> On Oct 14, 2008, at 1:58 , Byron Foster wrote:
>>>>
>>>>> Actually I don't think it's all that odd, the error case is somewhat
>>>>> general (it came up in my own usage of Velocity to generate HTML lists).
>>>>> Any
>>>>> pass by name reference to the #foreach iteration variable will fail, it
>>>>> doesn't need to be in a string literal, for example:
>>>>>
>>>>> #macro(test $x)#foreach($i in [1, 2])$x#end#end
>>>>> #test($i)
>>>>>
>>>>> Maybe a different disscussion, but somewhat related to how #foreach is
>>>>> implemented with local scope. The macro localscope setting
>>>>> (velocimacro.context.localscope) has the same behavior as above for the
>>>>> #set
>>>>> directive. However, I'm not sure what Velocity's intended behavior is
>>>>> with
>>>>> scoping rules and pass by name.
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>>>> For additional commands, e-mail: [EMAIL PROTECTED]
>>>>
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>> For additional commands, e-mail: [EMAIL PROTECTED]
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
