You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@velocity.apache.org by Will Glass-Husain <wg...@forio.com> on 2004/12/17 19:40:12 UTC
null in foreach
Hi,
Does anyone have any strong opinions about null in foreach? (specifically, reasons why we should not do this patch).
http://issues.apache.org/bugzilla/show_bug.cgi?id=27741
Note - I see this as a separate issue from the #set and #if handling of nulls. The problem with #foreach is that when looping through a collection containing a null, if a null is hit the iterator carries the old value rather than a null. This just doesn't make sense to me.
The provided patch (by Shinobu - thanks!) fixes it, but uses a config property to use old vs new behavior. I'd like to drop the config property and just make #for work as expected, e.g. return the current value of the collection (null or not).
Are there any compatibility issues with this?
WILL
_______________________________________
Forio Business Simulations
Will Glass-Husain
phone (415) 440-7500 x89
mobile (415) 235-4293
wglass@forio.com
www.forio.com
Re: null in foreach
Posted by Christoph Reck <ap...@recks.org>.
Hi,
sample patch code below...
Will Glass-Husain wrote:
> Christoph,
>
> Thanks for the detailed explanation of the alternative approaches. To
> sum up then, $foreach puts the iterating value into the current
> context. Since we can't assume that the current context will allow a
> value of null, this approach is not safe.
>
> I like the idea of adding a chained context. It'd be nice if this could
> delegate to the current context for all non-null values. (Have to look
Yup, this is what I mean. Since the #foreach has control of its body
rendereing, it has the chance of inserting the chained context at this place.
The chained context delegetes everything to the parent context. "#set"ing
the loop variable to something non-null should set a flag within the
chained context to disable it, thus affecting only the parent - thus
preserving BC.
When the loop variable is non-null, #foreach could just use the original
context; avoiding the additioanl chained context layer.
The chained context key=null inner class can be lazily instantiated when
first needed (null encountered while iterating).
See sample patch code below!
> into this). If the user does a #set in the middle of the #foreach, it's
> important the semantics of the original context be preserved. (For
> example, the parent context might be case-insensitive)
>
> Can I post your explanation as a comment in bugzilla? (Or do you want to?)
It's in there now!
>
> WILL
>
>
> ----- Original Message ----- From: "Christoph Reck" <ap...@recks.org>
> To: "Velocity Developers List" <ve...@jakarta.apache.org>
> Sent: Friday, December 17, 2004 2:01 PM
> Subject: Re: null in foreach
>
>
>> Hi,
>>
>> +1 to make a #foreach act properly when it encounters a null.
>>
>> -1 (non binding) for the available patch. Reasoning is next:
>>
>> The historical source of why velocity does not handle nulls:
>> Originally vel started off with the posibility of using a simple
>> Hashtable
>> as the context - which does not allow nulls.
>>
>> a) If we where allowed to set null into the context - no problem.
>>
>> b) We could also wrap the user context into a chained one with only
>> the #foreach var - which does allow setting it to null.
>> -> This could raise some BC problem, since after the #foreach loop
>> the original context may be restored, but the application wants
>> to see the last value we had in the loop.
>>
>> c) When setting null into the map value is not allowed, removing the key
>> could yield the desired result.
>> -> but beware... if the context was a chained context, the parent
>> key could suddendly show - ugh.
>> -> also beware that the parent context could be read-only and thus
>> may not be altered...
>>
>> d) The best solution is to have a pre-made key=null context at hand in
>> the
>> foreach directive instance to overlay it as a chained context every
>> time a null is encountered.
>>
>> Hope this helps to understand the whole problem.
>>
>> I hope the coding of solution d) is easy as it could be made as an inner
>> class to the #foreach directive and could be lazily instantiated upon
>> the first use.
>>
>> The solution d) is not possible for a #set directive, since it does not
>> have a body and thus it is not possible to overlay a chained context.
>> So the pittfalls of c) come to show...
>>
>> Cheers,
>> Christoph
>>
>> Will Glass-Husain wrote:
>>
>>> Hi,
>>>
>>> Does anyone have any strong opinions about null in foreach?
>>> (specifically, reasons why we should not do this patch).
>>> http://issues.apache.org/bugzilla/show_bug.cgi?id=27741
>>>
>>> Note - I see this as a separate issue from the #set and #if handling
>>> of nulls. The problem with #foreach is that when looping through a
>>> collection containing a null, if a null is hit the iterator carries
>>> the old value rather than a null. This just doesn't make sense to me.
>>>
>>> The provided patch (by Shinobu - thanks!) fixes it, but uses a config
>>> property to use old vs new behavior. I'd like to drop the config
>>> property and just make #for work as expected, e.g. return the current
>>> value of the collection (null or not).
>>>
>>> Are there any compatibility issues with this?
>>>
>>> WILL
Sample code for a pach (untested, but good cahnces to work immediately!):
within the Foreach.render(), replaing the exiting while loop:
// no sync needed, since at most one instance per render() call!
NullHolderContext nullHolderContext = null;
while( i.hasNext() )
{
context.put( counterName, new Integer(counter) );
Object value = i.next();
context.put(elementKey, value);
if( value == null )
{
if( nullHolderContext == null )
{
// lazy instantiation
nullHolderContext = new NullHolderContext(elementKey, context);
}
node.jjtGetChild(3).render(nullHolderContext, writer);
}
else
{
node.jjtGetChild(3).render(context, writer);
}
counter++;
}
Inner class for the Foreach directive:
/**
* A chained context as placeholder for a null valued loop variable.
*/
protected class NullHolderContext implements Context
{
private Context innerContext;
private String loopVariableKey = "";
private boolean active = true;
private NullHolderContext( String key, Context context )
{
innerContext = context;
if( key != null )
loopVariableKey = key;
}
public Object get( String key )
{
return ( active && loopVariableKey.equals(key) )
? null
: innerContext.get(key)
}
public Object put( String key, Object value )
{
if( loopVariableKey.equals(key) )
{
if( value == null )
{
active = true;
}
return innerContext.put( key, value )
}
}
public boolean containsKey( Object key )
{
return loopVariableKey.equals(key) || innerContext.containsKey(key);
}
public Object[] getKeys()
{
return innerContext.getKeys();
}
public Object remove(Object key)
{
if( loopVariableKey.equals(key) )
{
active = false;
}
return innerContext.remove(key);
}
}
Cheers,
Christoph
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: null in foreach
Posted by Will Glass-Husain <wg...@forio.com>.
Christoph,
Thanks for the detailed explanation of the alternative approaches. To sum
up then, $foreach puts the iterating value into the current context. Since
we can't assume that the current context will allow a value of null, this
approach is not safe.
I like the idea of adding a chained context. It'd be nice if this could
delegate to the current context for all non-null values. (Have to look into
this). If the user does a #set in the middle of the #foreach, it's
important the semantics of the original context be preserved. (For example,
the parent context might be case-insensitive)
Can I post your explanation as a comment in bugzilla? (Or do you want to?)
WILL
----- Original Message -----
From: "Christoph Reck" <ap...@recks.org>
To: "Velocity Developers List" <ve...@jakarta.apache.org>
Sent: Friday, December 17, 2004 2:01 PM
Subject: Re: null in foreach
> Hi,
>
> +1 to make a #foreach act properly when it encounters a null.
>
> -1 (non binding) for the available patch. Reasoning is next:
>
> The historical source of why velocity does not handle nulls:
> Originally vel started off with the posibility of using a simple Hashtable
> as the context - which does not allow nulls.
>
> a) If we where allowed to set null into the context - no problem.
>
> b) We could also wrap the user context into a chained one with only
> the #foreach var - which does allow setting it to null.
> -> This could raise some BC problem, since after the #foreach loop
> the original context may be restored, but the application wants
> to see the last value we had in the loop.
>
> c) When setting null into the map value is not allowed, removing the key
> could yield the desired result.
> -> but beware... if the context was a chained context, the parent
> key could suddendly show - ugh.
> -> also beware that the parent context could be read-only and thus
> may not be altered...
>
> d) The best solution is to have a pre-made key=null context at hand in the
> foreach directive instance to overlay it as a chained context every
> time a null is encountered.
>
> Hope this helps to understand the whole problem.
>
> I hope the coding of solution d) is easy as it could be made as an inner
> class to the #foreach directive and could be lazily instantiated upon
> the first use.
>
> The solution d) is not possible for a #set directive, since it does not
> have a body and thus it is not possible to overlay a chained context.
> So the pittfalls of c) come to show...
>
> Cheers,
> Christoph
>
> Will Glass-Husain wrote:
>> Hi,
>>
>> Does anyone have any strong opinions about null in foreach?
>> (specifically, reasons why we should not do this patch).
>> http://issues.apache.org/bugzilla/show_bug.cgi?id=27741
>>
>> Note - I see this as a separate issue from the #set and #if handling of
>> nulls. The problem with #foreach is that when looping through a
>> collection containing a null, if a null is hit the iterator carries the
>> old value rather than a null. This just doesn't make sense to me.
>>
>> The provided patch (by Shinobu - thanks!) fixes it, but uses a config
>> property to use old vs new behavior. I'd like to drop the config property
>> and just make #for work as expected, e.g. return the current value of the
>> collection (null or not).
>>
>> Are there any compatibility issues with this?
>>
>> WILL
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
Re: null in foreach
Posted by Christoph Reck <ap...@recks.org>.
Hi,
+1 to make a #foreach act properly when it encounters a null.
-1 (non binding) for the available patch. Reasoning is next:
The historical source of why velocity does not handle nulls:
Originally vel started off with the posibility of using a simple Hashtable
as the context - which does not allow nulls.
a) If we where allowed to set null into the context - no problem.
b) We could also wrap the user context into a chained one with only
the #foreach var - which does allow setting it to null.
-> This could raise some BC problem, since after the #foreach loop
the original context may be restored, but the application wants
to see the last value we had in the loop.
c) When setting null into the map value is not allowed, removing the key
could yield the desired result.
-> but beware... if the context was a chained context, the parent
key could suddendly show - ugh.
-> also beware that the parent context could be read-only and thus
may not be altered...
d) The best solution is to have a pre-made key=null context at hand in the
foreach directive instance to overlay it as a chained context every
time a null is encountered.
Hope this helps to understand the whole problem.
I hope the coding of solution d) is easy as it could be made as an inner
class to the #foreach directive and could be lazily instantiated upon
the first use.
The solution d) is not possible for a #set directive, since it does not
have a body and thus it is not possible to overlay a chained context.
So the pittfalls of c) come to show...
Cheers,
Christoph
Will Glass-Husain wrote:
> Hi,
>
> Does anyone have any strong opinions about null in foreach? (specifically, reasons why we should not do this patch).
> http://issues.apache.org/bugzilla/show_bug.cgi?id=27741
>
> Note - I see this as a separate issue from the #set and #if handling of nulls. The problem with #foreach is that when looping through a collection containing a null, if a null is hit the iterator carries the old value rather than a null. This just doesn't make sense to me.
>
> The provided patch (by Shinobu - thanks!) fixes it, but uses a config property to use old vs new behavior. I'd like to drop the config property and just make #for work as expected, e.g. return the current value of the collection (null or not).
>
> Are there any compatibility issues with this?
>
> WILL
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
RE: null in foreach
Posted by Tim Colson <tc...@cisco.com>.
> I, for one, have always considered this "feature" to be quite
> silly ...
+1 on silly
Not meaning to offend, since in the past I have read threads where folks had
rational explanations on why this is The Way... but to me, it still has
always felt a bit silly.
<grin>
Regarding Backward compat... I'm sure there are a few folks who might have
built templates that take advantage of this "feature" knowingly or
unwittingly. But I'd be okay, and a prominent "Release notes" mention of the
change should probably help others.
Cheers,
Timo
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
RE: null in foreach
Posted by "Marinó A. Jónsson" <ma...@centrum.is>.
I, for one, have always considered this "feature" to be quite silly ... so
please go ahead and drop the config property :-) I can't imagine many
users relying on this.
cheers,
Marino
-----Original Message-----
From: Will Glass-Husain [mailto:wglass@forio.com]
Sent: 17. desember 2004 18:40
To: velocity-dev@jakarta.apache.org
Subject: null in foreach
Hi,
Does anyone have any strong opinions about null in foreach? (specifically,
reasons why we should not do this patch).
http://issues.apache.org/bugzilla/show_bug.cgi?id=27741
Note - I see this as a separate issue from the #set and #if handling of
nulls. The problem with #foreach is that when looping through a collection
containing a null, if a null is hit the iterator carries the old value
rather than a null. This just doesn't make sense to me.
The provided patch (by Shinobu - thanks!) fixes it, but uses a config
property to use old vs new behavior. I'd like to drop the config property
and just make #for work as expected, e.g. return the current value of the
collection (null or not).
Are there any compatibility issues with this?
WILL
_______________________________________
Forio Business Simulations
Will Glass-Husain
phone (415) 440-7500 x89
mobile (415) 235-4293
wglass@forio.com
www.forio.com
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org