[Laszlo-dev] For Review: Change 20090423-maxcarlson-5 Summary: Add warnings for non-string LzDataElement attributes

[Max Carlson]

P T Withington wrote:
> Ok, I am officially confused. (The data stuff always does that to me!)
> 
> If you can't reliably make an xpath query against a non-string element 
> in a dataset, I have to ask, "how is webtop's usage working?"

They are making queries against attribute names, not values.  A specific 
case is a combo box of mail folders where the attribute name is the 
label, and the value is a reference to the actual mail folder.  There 
are many other examples that haven't been tracked down yet

> Can we get a specific example of what it is that webtop is trying to do 
> with these non-string datasets?  It really sounds to me like they were 
> (ab)using a loophole in the implementation to do something that was not 
> really intended and that the correct fix is on the usage end, not to 
> revert the LFC implementation.

Your assessment is correct and I agree - but since this loophole has 
been there for a long time, I feel we should allow for a release before 
making this change.  It could break a lot of apps out there...

> On 2009-04-26, at 06:47EDT, André Bargull wrote:
> 
>> I think the warnings produce more noise than actually necessary. And 
>> it's not really forbidden to use non-string values, the values will 
>> just be coerced to strings, so a warning is also a bit misleading 
>> because people may think they must not use non-string values (which 
>> isn't true as explained).

That's correct - you can put non-string values in, but you can't get 
them back out again.  Perhaps the warning message should be re-worded, 
but I think it will help people track down places where their 
applications are making this mistake.

>> If it's considered to risky to change (which means to fix) the 
>> behaviour in one release without further notification, I like to 
>> propose to back out the warnings and the coercion, and instead make a 
>> release note for the next release (4.4?) explaining the upcoming 
>> change for the release after that one (4.5?).
>> Thoughts?

I agree about the release note, but I think we should leave the warnings 
in...  Perhaps they should use Debug.info() instead of Debug.warn() - is 
there a setting in the debugger to restrict the level to cut down the noise?

>> On 4/25/2009 9:08 PM, Max Carlson wrote:
>>> I agree completely - I'm just worried about users who will get bit by 
>>> this, since we didn't enforce the restriction before.  One of our 
>>> biggest customers (webtop) is seeing a lot of warnings related to 
>>> this change...
>>> André Bargull wrote:
>>>> But string coercion is already enforced in 
>>>> "lz.DataElement#setAttr(..)", it's actually required by the DOM-spec 
>>>> [1] and without coercion you can't work reliably with xpaths.
>>>> For example this testcase only works correctly in trunk, if you try 
>>>> it in OL4.3, you'll only see the text-element displaying "first".
>>>>
>>>>> <canvas debug="true">   <dataset name="ds" />
>>>>>  <vbox>
>>>>>    <text datapath="ds:/item[@shown='true']/text()" />
>>>>>  </vbox>
>>>>>  <handler name="oninit">
>>>>>    canvas.ds.appendChild(new lz.DataElement('item', {shown: 
>>>>> 'true'}, [new lz.DataText("first")]));
>>>>>    canvas.ds.appendChild(new lz.DataElement('item', {shown: true}, 
>>>>> [new lz.DataText("second")]));
>>>>>  </handler>
>>>>> </canvas>
>>>>
>>>>
>>>> [1] 
>>>> http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-F68F082 
>>>>
>>>>
>>>>
>>>>
>>>> On 4/25/2009 7:17 PM, Max Carlson wrote:
>>>>> I think we should back out the string coercion for at least one 
>>>>> release, and leave in these warnings.  This is a pretty major 
>>>>> change for folks that use LzDataElements to store non-string 
>>>>> attribute values.  Thoughts?
>>>>>
>>>>> André Bargull wrote:
>>>>>> Two nits:
>>>>>> - please add `*`-type to the "val"-variable
>>>>>> - maybe it should say "lz.DataElement" instead of "LzDataElement"
>>>>>>
>>>>>> Otherwise approved.
>>>>>>
>>>>>>
>>>>>> On 4/24/2009 12:50 AM, Max Carlson wrote:
>>>>>>> Change 20090423-maxcarlson-5 by maxcarlson at Bank.local on 
>>>>>>> 2009-04-23 15:45:43 PDT
>>>>>>>    in /Users/maxcarlson/openlaszlo/trunk-clean
>>>>>>>    for http://svn.openlaszlo.org/openlaszlo/trunk
>>>>>>>
>>>>>>> Summary: Add warnings for non-string LzDataElement attributes
>>>>>>>
>>>>>>> Bugs Fixed: LPP-8083 - LzDataElement.setAttribute('attributes', 
>>>>>>> {...}) forces attribute values to strings
>>>>>>>
>>>>>>> Technical Reviewer: andre.bargull at udo.edu
>>>>>>> QA Reviewer: ptw at laszlosystems.com
>>>>>>>
>>>>>>> Details: Add warnings when an attribute is assigned a non-string 
>>>>>>> value.
>>>>>>>
>>>>>>> Tests: See LPP-8083
>>>>>>>
>>>>>>> Files:
>>>>>>> M      WEB-INF/lps/lfc/data/LzDataElement.lzs
>>>>>>>
>>>>>>> Changeset: 
>>>>>>> http://svn.openlaszlo.org/openlaszlo/patches/20090423-maxcarlson-5.tar 
>>>>>>>
>>>>>>>
>>>>>
> 

-- 
Regards,
Max Carlson
OpenLaszlo.org