Fixed #16.

[iham]

hope this is the "best" solution by now.

[jensens]

You can always start a Jenkins test-job on your own to test that your pull request (PR) does not break anything.

Below click on the link to Jenkins

log in with your Github user - if not already done before.

Paste the full pull request URL into the text field (if multiple pull requests need to be combined in one run, then one PR URL per line)

Click on Build

Jenkins reports back to the pull request(s) on Github in the status box at the bottom.

In Plone we value a lot our testing suite, so we usually never merge pull requests if they have not been tested by Jenkins first.

[davisagli]

I don't like this fix. If someone is using a vocabulary that intentionally uses string values that have only digits, this will break them.

It's impossible to know what is the right way to deserialize the value for all choice fields, because vocabulary values can be anything (even non-primitive objects). That's why this already uses the IFromUnicode adapter. You can override the IFromUnicode adapter of IChoice fields within your project if you need something more complicated than deserializing as a string.

[iham]

erm ... this is not about my project

this is a basic error we all need a solution for because it happens in GS, which is basic plone stuff and it happens with the vobaluray for the firstday of week, which is also basic plone stuff. maybe mine is not ideal, but how to fix it then?

i appreciate any solution. having to override the IFromUnicode adapter is none, as there is an error on how any (cant tell which) of the above mentioned is wrong implemented.

[davisagli]

I understand that this is a limitation which is not specific just to your project. I'm open to considering a proposal for how to do this as long as the XML model is explicit about the type that should be used to deserialize the vocabulary values, rather than requiring us to guess (which will be wrong sometimes, and backwards incompatible if we guess in the way implemented here).

Thanks for your work on this and I don't mean to be discouraging. But I'm concerned about breaking existing vocabularies.

[davisagli]

Oh, I wasn't paying close enough attention and missed that this is a problem affecting a built-in Plone vocabulary. We definitely need a fix for that regardless of how we solve the problem in general, then. Thanks for pointing it out.

[jensens]

David, I think you closed this one without understanding the problem space completely. This is indeed a core problem if one can not set the value type of a choice explicitly. Maybe this solution is not what we want, just closing and ignoring is not a solution. Fun fact: with Archetypes we had the same in past.

[thet]

zope.schema._field.Choice derives from zope.schema._bootstrapfield.Field, which defines a _type attribute. This is actually meant to define the value type for constraint checks and could be used to convert values into the field's own type. But the preceding underscore makes it an informal private attribute which shouldn't be used from outside - too bad! Defining the value type on the field itself would be the best place.

An alternative would be to define the type as an attribute of the field value node element in the XML file. But that feels a bit like being the wrong place...

Are schema tagged values something to consider?

[iham]

in the meantime i am pretty sure that extending the choice field by a value_type is wrong. (this would lead to a double definition: once as the value_type and secondly inside the vocabulary by creating a simpleterm) but setting self._type on the given vocabulary sounds promising to me.

reconsider: zope.schema._field.Choice._validate() does this

if value not in vocabulary:

"in vocabulary" triggers the contains in zope.schema.vocabulary.SimpleVocabulary and

return value in self.by_value

value can be given from the registry.xml -> plone.supermodel.utils.elementToValue() by_value is a dict of keys (the type can be anything from the init of a simpleterm) and a value (the simpleterm itself)

the value "only" (haha) needs to be casted by the type of those keys. and we still can use "_type" in Choice: on init of a choice field (zope.schema._fields) it checks for a vocabulary. if there is one, i can get the type of the - lets say first - key of this vocabulary and store it to _type.

self._type = type(self.vocabulary.by_value.keys[0])

the _type will then be used in plone.supermodel.utils.fieldTypecast() and should cast proberly.

does that sound like a solution?

phew, very confusing ;)

[iham]

i think i have a solution

in utils.py after IDict and ICollection is checked:

    elif IChoice.providedBy(field):
        vocabulary = None
        try:
            vcf = getUtility(IVocabularyFactory, field.vocabularyName)
            vocabulary = vcf(None)
        except:
            pass

        if vocabulary and hasattr(vocabulary, 'by_value'):
            try:
                field._type = type(vocabulary.by_value.keys()[0])
            except:
                pass

        value = fieldTypecast(field, element.text)

sadly i couldn't find a place to set _type from inside the choice field itself.

tell me, what you think

[iham]

i implemented the new fix. please can anybody review that? @jensens @thet @davisagli

i would love to set the _type on the choice instead of fiddling around with it here, but i didn't find any way to work on the choice itself (on init(), on whatever)

looking forward for your help, so we could finalise this.

[iham]

any news on that?

[jensens]

It think your solution fits perfect in the current set of solutions we have for this problems-space. Anyway I don't like this current way to solve the problem, but that is not the questions here.

May you run the tests? If green I am going to merge it

[iham]

@jensens all lights are green! please merge :)