FieldStorageUploadConverter doesn't support "not_empty" option to make it optional

I haven't really looked into this, but maybe the real problem is that what is called to_python here should be really _to_python?

Not without the changes I proposed, at least. If I revert the above patch on my local copy and just rename the method, then it doesn't work as expected.

Implementing just the init method, as proposed above, with a rename of the method from to_python to _to_python also doesn't work.

Keeping the patch and renaming the method also works, but the above patch is needed.

With regards to the method name, no change is really needed, though it should be done to conform to the documentation on inherited and overriden methods. From formencode.validators.FancyValidator?:

 |  Validators have two important external methods:
 |
 |  * .to_python(value, state):
 |    Attempts to convert the value.  If there is a problem, or the
 |    value is not valid, an Invalid exception is raised.  The
 |    argument for this exception is the (potentially HTML-formatted)
 |    error message to give the user.
(...)

and

 |  There are five important methods for subclasses to override,
 |  however none of these *have* to be overridden, only the ones that
 |  are appropriate for the validator:
 |
(...)
 |  * ._to_python(value, state):
 |    This returns the converted value, or raises an Invalid
 |    exception if there is an error.  The argument to this exception
 |    should be the error message.
(...)

A new patch, then, would be:

Index: validators.py
===================================================================
--- validators.py       (revisão 4084)
+++ validators.py       (cópia de trabalho)
@@ -177,13 +177,17 @@
 # see #1464357 on FE bugtracker (http://tinyurl.com/lm9ae).
 # Custom version of FieldStorage validator that does not break FE schema validator.
 class FieldStorageUploadConverter(TgFancyValidator):
-    def to_python(self, value, state=None):
+    def __init__(self, *args, **kwargs):
+        super(TgFancyValidator, self).__init__(*args, **kwargs)
+        self.not_empty = kwargs.get('not_empty')
+
+    def _to_python(self, value, state=None):
         if isinstance(value, cgi.FieldStorage):
             if value.filename:
                 return value
-            raise Invalid('invalid', value, state)
-        else:
-            return value
+            if self.not_empty:
+                raise Invalid('invalid', value, state)
+        return value
 
 # For translated messages that are not wrapped in a Validator.messages
 # dictionary, we need to reinstate the Turbogears gettext function under

Since TgFancyValidator inherits from Declarative, it sets instance variables automatically in the constructor. So setting self.not_empty explicitly in __init__ shouldn't be needed.

I still think the only problem is that TG overrides to_python instead of _to_python, because the not-empty-case is handled in to_python, and if you override it, it doesn't work any more.

I tried the following:

>>> import formencode
>>> formencode.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
formencode.api.Invalid: Please enter a value

So, in formencode it works. Now with turbogears:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
''

So, turbogears ignores the not_empty flag (this is your bug). Now I fix turbogears.validators by renaming to_python to _to_python in the FieldStorageUploadConverter class, and try again:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
...    not_empty=True).to_python('')
formencode.api.Invalid: Please enter a value

So after the rename, turbogears behaves as expected.

Actually, the case is making it optional, not mandatory. But it works with formencode, yes.

>>> import formencode
>>> formencode.validators.FieldStorageUploadConverter(
... not_empty=False).to_python('')
>>>

With the original code in TG:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=True).to_python('')
''
>>> 

This is a first failure. It should be invalid.

Renaming the method, as you suggested:

>>> import turbogears
>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=True).to_python('')
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/usr/local/lib/python2.5/site-packages/FormEncode-0.7.1-py2.5.egg/formencode/api.py", line 357, in to_python
    raise Invalid(self.message('empty', state), value, state)
formencode.api.Invalid: Please enter a value
>>> 

And now making the field optional:

>>> turbogears.validators.FieldStorageUploadConverter(
... not_empty=False).to_python('')
>>> 

I have to check if there is some other layer that is failing for a real form or not. I only tested this on the full application and it had still fails, even though it shouldn't according to what I have coded above.

I'll check if there's no other point in the TG Widget system that might be causing this failure.

So, for the validation part, just fixing the name should do it, indeed.

Any objections to commiting that fix?

I think instead of renaming the method we can remove the FieldStorageUploadConverter? class completely from TurboGears, since after the rename it would be the same as in FormEncode anyway. It seems this class was only included because of a bug in a very old FormEncode version.

Fixed now in r4106. Please let me know if is is still not working as expected.

In TG 1.0.x revision 4154 it does not work. It is not yet possible to make an fieldstorage optionnal.

applied in r4155 (1.0) and r4156 (1.1). Thx Jorge!

Ok, I see the problem now. FormEncode does not have the "if self.not_empty:" line.

I have two issues with the last checkin, though:

• The formencode bug #1464357 (http://tinyurl.com/lm9ae) mentioned there in the comment seems to be a different problem that has been already closed. So we should open a new bug on the formencode bugtracker and point to that instead. It is confusing to point to the wrong place and it makes no sense to solve this on the q.t. in TG without giving FE the chance to know about this bug and fix it.
• I'm pretty sure that the __init__ method is not needed, since as I said above, instance variables are set automatically for all FancyValidators. Can you confirm that it can be removed? This would avoid some confusion about what we are patching here against the original FE code.

Chris, I will look into this and come back to this ticket to notify...

I have already cared for this in r4183. There is now a unit test for this ticket, and I have submitted this as FormEncode bug 1905250 with our solution as suggested patch.

Please reopen again if there are still any issues with this.

Applied another fix in r4192 to make this work because the validator was returning a filename (string) instead of the cgi.FieldStorage? instance we need to grab the filedata from.

This now works as excpected but the FE bug report and patch should surely be improved to reflect this.

You're right - sorry for botching this up. The name of the FieldStorageUploadConverter? class and its docstring made me believe it actually converted the cgi.FieldStorage? instance to something (the filename) while in fact it should return the instance as it is, and only check if the filename is there. I have now adapted the unittests accordingly and merged this to 1.1 as well. Will also fix the FE bug report.