You are viewing a plain text version of this content. The canonical link for it is here.
Posted to rivet-dev@tcl.apache.org by Harald Oehlmann <ha...@elmicron.de> on 2012/05/29 17:08:47 UTC
form package bugfixes
Dear Rivet guys,
I agreed with Massimo that we try to fix the issues in the form package.
I will make a proposition and Massimo (or someone else) is doing the
fine tuning.
Attached is my proposition for review.
Comments:
load_response:
- insured that output array exists
- speed optimisation: reordered ifs to make less comparisons
form.tcl:
- return -> return ""
May have side effects otherwise, as last result is returned
- eval cmd $name $list -> ... cmd $name {*}$list
-> otherwise issues may raise if name has space.
- DefaultValues and list processing
There were many issues when default values were treated as lists.
Now, they are treated as lists if "__$name" exists (as outputted by
load_response).
The following widgets require lists:
radiobuttons checkboxes select -multiple
and all others require values.
The first element is used if a value is required and a list is given.
- Capsulated Default value processing internally be the protected
methods default_*
- enhanced public method default_value by option "-list" to get/set a
list of default values.
This is specially important to set the current value of the list
widgets (because there is no such option prevued):
my_form default_value -list -- "option check default" {"o 2" "o 3"}
my_form checkboxes "option check default"\
-values {"o 1" "o 2" "o 3" "o 4"}\
-labels {"l 1" "l 2" "l 3" "l 4"}
- Use uuid package only if it is loaded.
I personally didn't like that. In addition, there was no
"package require uuid".
IMHO the code should export the uuid values, if they are generated
on the fly. It is "half baken".
- Added widget type "checkboxes".
The manual describes "-values" and "-labels" for "checkbox", which
do not exist. So, I created method "checkboxes" with those values.
testform.tcl
My form test file.
Footnote:
for me, form "my form" raises an Itcl error.
I have forwarded this to Arnulf Wiedemann.
Enjoy,
Harald
Earthquake
Posted by Harald Oehlmann <ha...@elmicron.de>.
Dear Massimo,
I want to express our compassion to your region which experience a hard
time and people loss due to the continuing earthquakes.
I read in the local newspaper today and saw pictures and it looked
really frightend.
Good to have you here !
For the Rivet group,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Dear Damon,
thank you for the reply! It is a pleasure that someone looked it up.
Am 29.05.2012 17:26, schrieb Damon Courtney:
>> Comments:
>> load_response:
>> - insured that output array exists
>
> Just to note, you are breaking compatibility here. Previously one could rely on ![info exists response] to test whether any response had, in fact, been loaded. I'm not a backward compatibility nut, but change for the sake of change is bad. Is there a compelling reason for this?
No. I just had the sequence:
load_response
parray response
which failed if there was no response.
So I added that without thinking to far.
There is no big reason for it.
IMHO there is a lack of documentation here.
>> form.tcl:
>> - return -> return ""
>> May have side effects otherwise, as last result is returned
>
> There is no difference between [return] and [return ""]. A return with no arguments returns an empty string.
Correct, sorry.
> Don't use the uuid package. There's no need for it.
> Just use a simple, unique number within the request.
> There's no need for the overhead of generating a UUID (small though it
may be),
> and there's definitely no need to add a requirement for tcllib to Rivet.
> If you want to ship Rivet with tcllib included, I could be persuaded
for that,
> but don't make me get tcllib just to get a unique name for an HTML
form element.
I agree.
> ## If you don't mind creating a variable in the request namespace
> autogen_[incr ::request::__formElemCount]
>
> OR
>
> autogen_[info cmdcount]
>
> OR
>
> autogen_[clock microseconds]
>
> would all work just fine without the requirement of another package.
The uuid dependency was not introduced by myself.
I made this change to make the form package running for me, as I have
currently no uuid package.
I would prefer something more opaque to the user.
Thus modify method radiobuttons/checkboxes to use the name and an
increasing number as id and document that.
I would also prefer to have only an id generated if there is a special
parameter. Example:
my_form checkbox check -id #auto
which would generate the id check_[info cmdcount]
But that is another discussion. Not my building site.
I would encourrage Massimo to use your solution.
> Being that there are many bug fixes but also MANY incompatibilities in this version of
> the form package, we might consider moving these changes to a
form2.tcl file and
> giving it a [package provide form 2.0] label.
> This would leave the old one in place, and anyone running into
compatibility issues
> could simply change [package require form] to [package require form 1.0] to get the
> old one and keep running without all the pain in the ass.
>
> I'm not disagreeing with the changes here.
> They look good, and they do fix bugs.
> But give people a temporary migration path.
The only purpose is fixing bugs (52650, 52909) and I am not in favour
calling it forms2.
For me, the uuid-issue is heavy, but that was not me.
As the current form package has security issues due to the list handling
(you may easily crash it by special user input:
load_response
form my_form
form start
my_form radiobuttons radio -values {1 2 3}
form end
Called with
http://test.rvt?radio=%7B
will cause a runtime error, as the data is no valid list and it is
accessed via "lsearch" which causes a crash. )
there is action needed (See my monologues on bugs 52650, 52653, 52909).
So I agreed with Massimo to use the magic __$name indexes to indicate
lists. I was not for this method but it is the most compatible way.
So this is my implementation proposal.
I agreed with Massimo that he will refine it as we did with the
rivet::entity package.
Damon, if you have any proposals what to change, please post them and
otherwise check the package if it works for you.
IMHO there are only incompatibilities introduced which are necessary.
When I started with the form package, the default mechanism did not work
at all due to a variable name clash...
Thank you all,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Am 30.05.2012 17:51, schrieb Damon Courtney:
> And that's my point. If it matters, YOU should pass -id X instead of letting us generate one. In other words:
>
> set f [form #auto] ;
>
> $f checkbox foo -label Foo ; ## Note, I don't care about an ID here.
> $f checkbox bar -label Bar ; ## Don't care about this one either.
> $f checkbox moo -label Moo -id bar-checkbox ; ## I'm going to style this or use Javascript, so I need its ID.
>
> If you are letting US auto generate IDs and then using those in your CSS and JS, you're a bad, bad developer. One, tiny change to your HTML, and your shit is busted. If you rejiggered your form to move Bar above Foo, you just screwed up their ID you were relying on.
>
> The documentation should say that if you don't give us a -id, we'll auto-generate one that is unique. That's it. We can give people rope to hang themselves, but we don't have to put it around their neck and give them a push.
I agree.
I would document the way autogenerated IDs are build, e.g. say, that
they always start with "autogen".
Thank you,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Damon Courtney <da...@tclhome.com>.
And that's my point. If it matters, YOU should pass -id X instead of letting us generate one. In other words:
set f [form #auto] ;
$f checkbox foo -label Foo ; ## Note, I don't care about an ID here.
$f checkbox bar -label Bar ; ## Don't care about this one either.
$f checkbox moo -label Moo -id bar-checkbox ; ## I'm going to style this or use Javascript, so I need its ID.
If you are letting US auto generate IDs and then using those in your CSS and JS, you're a bad, bad developer. One, tiny change to your HTML, and your shit is busted. If you rejiggered your form to move Bar above Foo, you just screwed up their ID you were relying on.
The documentation should say that if you don't give us a -id, we'll auto-generate one that is unique. That's it. We can give people rope to hang themselves, but we don't have to put it around their neck and give them a push.
D
On May 30, 2012, at 10:42 AM, Harald Oehlmann wrote:
>> On May 30, 2012, at 10:20 AM, Harald Oehlmann wrote:
>>> Refering to the issue to generate the value "idcur" of:
>>> <label ref="idcur">labeltext</label><option id="idcur" name="namecur"
>>> value="valuecur"/>
>
>> Am 30.05.2012 17:34, schrieb Damon Courtney:
>> The ID need only be unique to the request and it does not need to be
>> repeatable for each request of the same page. It is simply a way for
>> the browser to link the label to the check / radio button such that
>> clicking the label will then trigger the button. They need to be unique
>> because all IDs in an HTML page need to be unique. I believe the UUID
>> package was used originally to ensure uniqueness no matter what the user
>> might choose as their IDs.
>>
>
> As far as I know, the ID may be used at two places:
>
> - JavaScript
> - Cascading Style Sheets (newer version)
>
> Thus it may matter to a developper and it would be helpful, if it is
> predictable and documented.
>
> Regards,
> Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
> On May 30, 2012, at 10:20 AM, Harald Oehlmann wrote:
>> Refering to the issue to generate the value "idcur" of:
>> <label ref="idcur">labeltext</label><option id="idcur" name="namecur"
>> value="valuecur"/>
Am 30.05.2012 17:34, schrieb Damon Courtney:
> The ID need only be unique to the request and it does not need to be
repeatable for each request of the same page. It is simply a way for
the browser to link the label to the check / radio button such that
clicking the label will then trigger the button. They need to be unique
because all IDs in an HTML page need to be unique. I believe the UUID
package was used originally to ensure uniqueness no matter what the user
might choose as their IDs.
As far as I know, the ID may be used at two places:
- JavaScript
- Cascading Style Sheets (newer version)
Thus it may matter to a developper and it would be helpful, if it is
predictable and documented.
Regards,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Damon Courtney <da...@tclhome.com>.
The ID need only be unique to the request and it does not need to be repeatable for each request of the same page. It is simply a way for the browser to link the label to the check / radio button such that clicking the label will then trigger the button. They need to be unique because all IDs in an HTML page need to be unique. I believe the UUID package was used originally to ensure uniqueness no matter what the user might choose as their IDs.
I think this is of little concern though. At least in the current patch, the IDs are explicitly called autogen_X, which is very unlikely to clash with a naming scheme from a developer. I could be wrong, but them's the breaks. The reason they don't need to be the same with every request is because the developer has basically TOLD us they don't care about the ID.
In other words, if the developer cared what the ID of the checkbox was (for CSS or Javascript), they would have passed us -id when creating the element, and we would use it. The only place this case comes up is when they are creating a check or radio button and they've basically said they don't care about the ID.
But we do. The browser can't link the label to the button without the button having id="X" and the label having for="X". That's all this does. Pick up slack where the developer hasn't cared enough to uniquely name the element. If you care, name your element, but if you don't, we still need to do the right thing for usability and accessibility.
I would probably just go with a class variable counter for the form package and increment it each time. Easy solution, the counter variable is obvious to anyone reading the form package, and the number will always be unique to the request even if they create multiple forms on a single page. Don't overcomplicate it.
Them's my two cents.
D
On May 30, 2012, at 10:20 AM, Harald Oehlmann wrote:
> Am 29.05.2012 17:26, schrieb Damon Courtney:
>> ## If you don't mind creating a variable in the request namespace
>> autogen_[incr ::request::__formElemCount]
>>
>> OR
>>
>> autogen_[info cmdcount]
>>
>> OR
>>
>> autogen_[clock microseconds]
>>
>> would all work just fine without the requirement of another package.
>
> Hi Damon,
>
> Refering to the issue to generate the value "idcur" of:
> <label ref="idcur">labeltext</label><option id="idcur" name="namecur"
> value="valuecur"/>
>
> I have another thought on your propositions how to replace the uuid package.
> There are some solutions to generate the value "idcur":
> S1) uuid package
> S2) counter which starts with 1 at each http request
> S3) [info cmdcount]
> S4) [clock microseconds]
> S5) interpreter wide counter
> S6) form name + form internal counter
>
> Here are some thoughts:
> T1) The id must be unique per http request result
> T2) when requesting the same url, there should be the same answer.
> T3) Two http requests should not be in any information relation.
> T4) Don't expose any internals
>
> I am not shure about T3, but this feels as a potential security hole.
>
> Here is the judgement of the solutions and the thoughts:
>
> | T1 | T2 | T3 | T3 |
> ---+----------+----------+----------+----------+
> S1 | yes | no | yes | yes |
> S2 | yes | yes | yes | yes |
> S3 | yes | no | no | no |
> S4 | no | no | no | no |
> S5 | yes | no | no | no |
> S6 | yes | yes | yes | no |
>
> So, S2 fullfills all thoughts.
>
> S3 to S5 would give internal information of the interpreter to the
> outside and put multiple requests in a relation (which request follows
> which). As stated before, I am not shure if this is an issue but it
> feels like a security hole.
>
> I prefer S6, as:
> + it is easiest to implement (object variable)
> + it is most logic to the outside
> - it exposes the internal form name (so T3 fails).
> Someone might have a better idea here.
> The form name may not be in the character set of an id.
>
> Enjoy,
> Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Am 01.06.2012 11:55, schrieb Ronnie Brunner:
>> - have the "_" in the prefix:
>> ${autoid_prefix}[incr auto_cnt]
>> default for variable 'autoid_prefix' is 'autogen_'
>
> Why not 'autoid_'?. Seems more consistent... Or was it released before
> (i.e. backward compatibilty)? (if yes: sorry for the noise)
It is still "autoid_" but it may be changed to a value without the "_".
It is a detail, not important.
-Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Am 01.06.2012 12:41, schrieb Massimo Manghi:
>> I am still against calling it form2.tcl.
>
> Mhh, sorry, I don't understand. The tclIndex.tcl file will be build in
> such a way your version will prevail, having been versioned with a
> higher number and thus implicitly providing a way to select which form
> package one has to require in a application.
Sorry, I thought, the whole package will be renamed to form2.
That is absolutely ok like that, no objections against it.
Thank you,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Hi Massimo,
The form.tcl with the new autogeneration works well for me.
I would:
- Name the whole feature "autoid"
- Name the option "-autoidprefix" (prefix is to global IMHO)
- have the "_" in the prefix:
${autoid_prefix}[incr auto_cnt]
default for variable 'autoid_prefix' is 'autogen_'
instead
> ${prefix}_[incr auto_cnt]
> default for variable 'prefix' is 'autogen'
to give custom settings more flexibility.
---
I am still against calling it form2.tcl.
Change summary to the user:
1) Bugfix of:treat default sometimes as value, sometimes as list
In consequence, package may crash by special user input
Authors of changes: Massimo, Harald
- use __$name indexes to indicate lists in default values
- extend the (so far undocumented but exported) method default_value
by a -list option to get and set parameters in list style
- extend "select" method to detect -multi 1 option and to use list style
default.
Possible incompatiblilities:
this code does not work any more:
set response(radio1) [var list radio1]
set response(checkbox1) [var list checkbox1]
form my_form -default response
my_form start
my_form radio radio1 -value "v 1" -label "l 1"
my_form checkbox checkbox1 -value "v 1" -label "l 1"
my_form end
as the "__name" indexes are missing.
This works now and is not vulnerable for attacks like "t.rvt?radio1=%7B"
(crash) or "t.rvt?radio1=v%201" (no detection) any more:
load_response
form my_form -default response
my_form start
my_form radio radio1 -value "v 1" -label "l 1"
my_form checkbox checkbox1 -value "v 1" -label "l 1"
my_form end
this didn't work before and works now:
load_response
form my_form -default response
my_form start
my_form select select1 -multiple 1 -values {"v 1" "v 2"}
my_form end
The first example must now be written as:
form my_form -default response
my_form default_value radio1 -list [var list radio1]
my_form default_value checkbox1 -list [var list checkbox1]
my_form start
my_form radio radio1 -value "v 1" -label "l 1"
my_form checkbox checkbox1 -value "v 1" -label "l 1"
my_form end
or as:
set response(radio1) [var list radio1]
set response(__radio1) ""
...
2) Bugfix/new method: checkboxes
Author of changes: Harald
The method "checkbox" docs stated, that it support the parameters
"-values" and "-labels". This was wrong.
In consequence, I added the method "checkboxes, which have those
parameters and which is similar to the methods radiobuttons/radio
It might be discussed if this is a bug-fix or a new feature.
Anyway, it is useful and does not harm anybody.
3) Bugfix/new function: Reference the option id in the label and
autogenerate an id if there is no id given (method checkbox/radiobutton)
Authors: Jeff, Damon, Massimo
There is a new option "-autoidprefix".
IMHO this is a new good harmless feature.
No incompatibilities introduced.
---
Looking to the changed issues, there is IMHO nothing justifying a new
form2.tcl package.
Most parts are bugfixes which must anyway be included in form.tcl. Or do
we just catch the "lsearch" and live with the status quo ?
Also, two packages must be:
- documented
- maintained !!!!!!
in future releases separately (IMHO the strongest argument, we have
other things to do)
I would prefer that others would test the new package and tell, if they
could live with that.
My much to long two pennies....
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Harald Oehlmann <ha...@elmicron.de>.
Am 29.05.2012 17:26, schrieb Damon Courtney:
> ## If you don't mind creating a variable in the request namespace
> autogen_[incr ::request::__formElemCount]
>
> OR
>
> autogen_[info cmdcount]
>
> OR
>
> autogen_[clock microseconds]
>
> would all work just fine without the requirement of another package.
Hi Damon,
Refering to the issue to generate the value "idcur" of:
<label ref="idcur">labeltext</label><option id="idcur" name="namecur"
value="valuecur"/>
I have another thought on your propositions how to replace the uuid package.
There are some solutions to generate the value "idcur":
S1) uuid package
S2) counter which starts with 1 at each http request
S3) [info cmdcount]
S4) [clock microseconds]
S5) interpreter wide counter
S6) form name + form internal counter
Here are some thoughts:
T1) The id must be unique per http request result
T2) when requesting the same url, there should be the same answer.
T3) Two http requests should not be in any information relation.
T4) Don't expose any internals
I am not shure about T3, but this feels as a potential security hole.
Here is the judgement of the solutions and the thoughts:
| T1 | T2 | T3 | T3 |
---+----------+----------+----------+----------+
S1 | yes | no | yes | yes |
S2 | yes | yes | yes | yes |
S3 | yes | no | no | no |
S4 | no | no | no | no |
S5 | yes | no | no | no |
S6 | yes | yes | yes | no |
So, S2 fullfills all thoughts.
S3 to S5 would give internal information of the interpreter to the
outside and put multiple requests in a relation (which request follows
which). As stated before, I am not shure if this is an issue but it
feels like a security hole.
I prefer S6, as:
+ it is easiest to implement (object variable)
+ it is most logic to the outside
- it exposes the internal form name (so T3 fails).
Someone might have a better idea here.
The form name may not be in the character set of an id.
Enjoy,
Harald
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org
Re: form package bugfixes
Posted by Damon Courtney <da...@tclhome.com>.
> Comments:
> load_response:
> - insured that output array exists
Just to note, you are breaking compatibility here. Previously one could rely on ![info exists response] to test whether any response had, in fact, been loaded. I'm not a backward compatibility nut, but change for the sake of change is bad. Is there a compelling reason for this?
> form.tcl:
> - return -> return ""
> May have side effects otherwise, as last result is returned
There is no difference between [return] and [return ""]. A return with no arguments returns an empty string.
> There were many issues when default values were treated as lists.
> Now, they are treated as lists if "__$name" exists (as outputted by
> load_response).
> The following widgets require lists:
> radiobuttons checkboxes select -multiple
> and all others require values.
> The first element is used if a value is required and a list is given.
> - Capsulated Default value processing internally be the protected
> methods default_*
> - enhanced public method default_value by option "-list" to get/set a
> list of default values.
> This is specially important to set the current value of the list
> widgets (because there is no such option prevued):
> my_form default_value -list -- "option check default" {"o 2" "o 3"}
> my_form checkboxes "option check default"\
> -values {"o 1" "o 2" "o 3" "o 4"}\
> -labels {"l 1" "l 2" "l 3" "l 4"}
> - Use uuid package only if it is loaded.
> I personally didn't like that. In addition, there was no
> "package require uuid".
> IMHO the code should export the uuid values, if they are generated
> on the fly. It is "half taken".
Don't use the uuid package. There's no need for it. Just use a simple, unique number within the request. There's no need for the overhead of generating a UUID (small though it may be), and there's definitely no need to add a requirement for tcllib to Rivet. If you want to ship Rivet with tcllib included, I could be persuaded for that, but don't make me get tcllib just to get a unique name for an HTML form element.
## If you don't mind creating a variable in the request namespace
autogen_[incr ::request::__formElemCount]
OR
autogen_[info cmdcount]
OR
autogen_[clock microseconds]
would all work just fine without the requirement of another package.
> Footnote:
> for me, form "my form" raises an Itcl error.
> I have forwarded this to Arnulf Wiedemann.
Being that there are many bug fixes but also MANY incompatibilities in this version of the form package, we might consider moving these changes to a form2.tcl file and giving it a [package provide form 2.0] label. This would leave the old one in place, and anyone running into compatibility issues could simply change [package require form] to [package require form 1.0] to get the old one and keep running without all the pain in the ass.
I'm not disagreeing with the changes here. They look good, and they do fix bugs. But give people a temporary migration path.
D
---------------------------------------------------------------------
To unsubscribe, e-mail: rivet-dev-unsubscribe@tcl.apache.org
For additional commands, e-mail: rivet-dev-help@tcl.apache.org