[Zope3-checkins] SVN: Zope3/trunk/src/zope/app/form/browser/ Fixed a bug in SimpleInputWidget. _getFormValue had an evil side

[Jim Fulton]

Log message for revision 37365:
  Fixed a bug in SimpleInputWidget.  _getFormValue had an evil side
  effect of setting _error.  I assumed that this was unintentional, but
  there were tests supporting it, which is weird.  If client code had
  not called getInputValue directly, then there is no reason to set
  _error. _getFormValue is only called by widget rendering code, which
  should not produce this side effect.
  
  Consider the following use case:
  
  - We present a form
  
  - The form contains widgets that have sub-forms.  Here "sub-forms" are
    collections of input widgets without a form tag. Many logical
    subforms share a common form tag.  Processing is dispatched based on
    the submit button used in a request.
  
  - A user interacts with a widget by clicking on a sub widget, for
    example do do "search and select".  They did not click on a submit
    button for the overall form.
  
  - The application does not ytry to get input from any of the widgets
    nor does it try to validate user input.  User input, of any, is know
    to be incomplete, because the form-submit buttons haven't been used.
  
  - In response to the users action, the application generates an
    updated form by rendering the widgets.  Because the user hasn't
    tried to submit the form, they should not be presented with error
    messages, at least not at the form level.
  
  In summary, widgets should not generate error messages if they haven't
  been asked for valid values.
  

Changed:
  U   Zope3/trunk/src/zope/app/form/browser/tests/test_itemswidget.py
  U   Zope3/trunk/src/zope/app/form/browser/tests/test_textwidget.py
  U   Zope3/trunk/src/zope/app/form/browser/widget.py

-=-
Modified: Zope3/trunk/src/zope/app/form/browser/tests/test_itemswidget.py
===================================================================
--- Zope3/trunk/src/zope/app/form/browser/tests/test_itemswidget.py	2005-07-21 14:51:20 UTC (rev 37364)
+++ Zope3/trunk/src/zope/app/form/browser/tests/test_itemswidget.py	2005-07-21 14:54:59 UTC (rev 37365)
@@ -216,11 +216,13 @@
              u'<option value="token2">Two</option>',
              u'<option value="token3">Three</option>'])             
 
-    def test_error(self):
-        widget = self._makeWidget(form={'field.choice': 'ten'})
-        widget.setPrefix('field.')
-        widget._getFormValue()
-        self.assert_(isinstance(widget._error, ConversionError))
+# This test is disabled because it tests for the presense of a missfeature,
+# which has been removed.  Did someone actually *want* this?
+##     def test_error(self):
+##         widget = self._makeWidget(form={'field.choice': 'ten'})
+##         widget.setPrefix('field.')
+##         widget._getFormValue()
+##         self.assert_(isinstance(widget._error, ConversionError))
     
     def test_hidden(self):
         widget = self._makeWidget(form={'field.choice': 'token2'})
@@ -413,11 +415,13 @@
              '>One</option>\n', 'value="token2"', '>Two</option>\n',
              'value="token3"', '>Three</option>', '</select>'])
 
-    def test_error(self):
-        widget = self._makeWidget(form={'field.numbers': ['ten']})
-        widget.setPrefix('field.')
-        widget._getFormValue()
-        self.assert_(isinstance(widget._error, ConversionError))
+# This test is disabled because it tests for the presense of a missfeature,
+# which has been removed.  Did someone actually *want* this?
+##     def test_error(self):
+##         widget = self._makeWidget(form={'field.numbers': ['ten']})
+##         widget.setPrefix('field.')
+##         widget._getFormValue()
+##         self.assert_(isinstance(widget._error, ConversionError))
     
     def test_hidden(self):
         widget = self._makeWidget(

Modified: Zope3/trunk/src/zope/app/form/browser/tests/test_textwidget.py
===================================================================
--- Zope3/trunk/src/zope/app/form/browser/tests/test_textwidget.py	2005-07-21 14:51:20 UTC (rev 37364)
+++ Zope3/trunk/src/zope/app/form/browser/tests/test_textwidget.py	2005-07-21 14:54:59 UTC (rev 37365)
@@ -253,7 +253,24 @@
       value=""
       />
 
+    """
 
+def test_no_error_on_render_only():
+    """This is really a test of a bug fix to SimpleInputWidget.
+
+    _error shouldn't be set due to an *internal* call to getInputValue
+    when rendering.
+
+    >>> from zope.publisher.browser import TestRequest
+    >>> from zope.schema import TextLine
+    >>> field = TextLine(__name__='foo')
+    >>> request = TestRequest(form={'field.foo': ''})
+    >>> widget = TextWidget(field, request)
+    >>> ignored = widget()
+    >>> unicode(widget.error())
+    u''
+
+
     """
 
 def test_suite():

Modified: Zope3/trunk/src/zope/app/form/browser/widget.py
===================================================================
--- Zope3/trunk/src/zope/app/form/browser/widget.py	2005-07-21 14:51:20 UTC (rev 37364)
+++ Zope3/trunk/src/zope/app/form/browser/widget.py	2005-07-21 14:54:59 UTC (rev 37365)
@@ -356,9 +356,16 @@
         """Returns a value suitable for use in an HTML form."""
         if not self._renderedValueSet():
             if self.hasInput():
+
+                # It's insane to use getInputValue this way. It can
+                # cause _error to get set spuriously.  We'll work
+                # around this by saving and restoring _error if
+                # necessary.
+                error = self._error
                 try:
                     value = self.getInputValue()
                 except InputErrors:
+                    self._error = error
                     return self.request.form.get(self.name, self._missing)
             else:
                 value = self._getDefault()