Ticket #6480 (closed Bug: worksforme)

Opened 7 years ago

Last modified 4 years ago

Zope 3 vocabularies have a problem with multiple identical labels

Reported by: limi Owned by: jladage
Priority: major Milestone: 4.0
Component: General Version:
Keywords: Cc:

Description (last modified by limi) (diff)

This is likely to cause a lot of pain in Plone, so it's imperative that we get this fixed in Zope 3.

PDB output:

-> return SimpleVocabulary.fromItems(items)
(Pdb) items
[(u'Boolean Criterion', 'ATBooleanCriterion'), ('ChangeSet', 'ChangeSet'),
(u'Collection', 'Topic'), (u'Current Author Criterion',
'ATCurrentAuthorCriterion'), (u'Date Range Criterion',
'ATDateRangeCriterion'), ('Discussion Item', 'Discussion Item'), (u'Event',
'Event'), (u'Favorite', 'Favorite'), (u'File', 'File'), (u'Folder',
'Folder'), (u'Friendly Date Criteria', 'ATDateCriteria'), (u'Image',
'Image'), ('Issue', 'PoiIssue'), ('Issue Tracker', 'PoiPscTracker'), ('Issue
Tracker', 'PoiTracker'), (u'Large Folder', 'Large Plone Folder'), (u'Link',
'Link'), (u'List Criterion', 'ATListCriterion'), ('Location', 'GeoLocation'), 
(u'News Item', 'News Item'), (u'Page', 'Document'), (u'Path Criterion', 
'ATPathCriterion'), ('Plone Site', 'Plone Site'), (u'Portal Types Criterion', 
'ATPortalTypeCriterion'), (u'Reference Criterion', 'ATReferenceCriterion'), 
(u'Relative Path Criterion', 'ATRelativePathCriterion'), ('Response', 
'PoiResponse'), (u'Selection Criterion', 'ATSelectionCriterion'), (u'Simple 
Int Criterion', 'ATSimpleIntCriterion'), (u'Simple String Criterion', 
'ATSimpleStringCriterion'), (u'Sort Criterion', 'ATSortCriterion'), 
('TempFolder', 'TempFolder')]

The issue here is that we have two things called "Issue Tracker" — even if they have different metatypes, there is a collision on the Titles.

Traceback:

2007-04-26 02:06:29 ERROR Zope.SiteErrorLog http://localhost:8080/3/@@types-controlpanel
Traceback (innermost last):
  Module ZPublisher.Publish, line 119, in publish
  Module ZPublisher.mapply, line 88, in mapply
  Module ZPublisher.Publish, line 42, in call_object
  Module plone.app.controlpanel.types, line 114, in __call__
  Module Shared.DC.Scripts.Bindings, line 313, in __call__
  Module Shared.DC.Scripts.Bindings, line 350, in _bindAndExec
  Module Products.PageTemplates.PageTemplateFile, line 129, in _exec
  Module Products.PTProfiler.ProfilerPatch, line 83, in __patched_render__
  Module Products.PageTemplates.PageTemplate, line 89, in pt_render
  Module zope.pagetemplate.pagetemplate, line 117, in pt_render
  Module zope.tal.talinterpreter, line 271, in __call__
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 891, in do_useMacro
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 891, in do_useMacro
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 949, in do_defineSlot
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 957, in do_defineSlot
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 891, in do_useMacro
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 957, in do_defineSlot
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 534, in do_optTag_tal
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 949, in do_defineSlot
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 536, in do_optTag_tal
  Module zope.tal.talinterpreter, line 521, in do_optTag
  Module zope.tal.talinterpreter, line 516, in no_tag
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 949, in do_defineSlot
  Module zope.tal.talinterpreter, line 346, in interpret
  Module zope.tal.talinterpreter, line 586, in do_setLocal_tal
  Module zope.tales.tales, line 696, in evaluate
   - URL: types
   - Line 30, Column 4
   - Expression: <PathExpr standard:'view/type_id'>
   - Names:
      {'container': <PloneSite at /3>,
       'context': <PloneSite at /3>,
       'default': <object object at 0x40528>,
       'here': <PloneSite at /3>,
       'loop': {},
       'nothing': None,
       'options': {'args': ()},
       'repeat': <Products.PageTemplates.Expressions.SafeMapping object at 0x642e350>,
       'request': <HTTPRequest, URL=http://localhost:8080/3/@@types-controlpanel>,
       'root': <Application at >,
       'template': <ImplicitAcquirerWrapper object at 0x6c41390>,
       'traverse_subpath': [],
       'user': <PropertiedUser 'limi'>,
       'view': <Products.Five.metaclass.TypesControlPanel object at 0x6c41e70>,
       'views': <zope.app.pagetemplate.viewpagetemplatefile.ViewMapper object at 0x72b29d0>}
  Module Products.PTProfiler.ProfilerPatch, line 32, in __patched_call__
  Module zope.tales.expressions, line 217, in __call__
  Module Products.PageTemplates.Expressions, line 131, in _eval
  Module zope.tales.expressions, line 124, in _eval
  Module Products.PageTemplates.Expressions, line 83, in boboAwareZopeTraverse
  Module zope.traversing.adapters, line 164, in traversePathElement
   - __traceback_info__: (<Products.Five.metaclass.TypesControlPanel object at 0x6c41e70>, 'type_id')
  Module zope.traversing.adapters, line 44, in traverse
   - __traceback_info__: (<Products.Five.metaclass.TypesControlPanel object at 0x6c41e70>, 'type_id', [])
  Module plone.memoize.instance, line 43, in memogetter
  Module plone.app.controlpanel.types, line 31, in type_id
  Module plone.memoize.instance, line 43, in memogetter
  Module plone.app.controlpanel.types, line 121, in selectable_types
  Module plone.app.vocabularies.types, line 51, in __call__
  Module zope.schema.vocabulary, line 83, in fromItems
  Module zope.schema.vocabulary, line 67, in __init__
ValueError: Supplied vocabulary values resulted in duplicate term tokens

Disclaimer: this is how it was explained to me, make sure it's actually a Zope 3 problem before reporting it there.

Change History

comment:1 Changed 7 years ago by limi

  • Description modified (diff)
  • Milestone changed from 3.0 to 3.0b2

comment:2 Changed 7 years ago by limi

  • Milestone changed from 3.0b2 to 3.0

comment:3 Changed 7 years ago by fschulze

This is not a problem in Zope 3, but incorrect use of vocabularies. The end user would be confused by this list anyway, because there are two items called "Issue Tracker" to select from. The title needs to be fixed and then fromItems would probably work as well again.

comment:4 Changed 7 years ago by limi

  • Owner changed from somebody to hannosch

comment:5 Changed 7 years ago by limi

  • Keywords upstream removed

comment:6 Changed 7 years ago by hannosch

  • Priority changed from critical to major
  • Milestone changed from 3.0 to 3.0.x

I'm not going to fix this before the final, as it introduces too much risk. For 3.0.1 we can change our vocabularies to have distinct id's, values and titles where those are available.

comment:7 Changed 7 years ago by limi

  • Priority changed from major to critical

It affects the workflow Titles too, and there have been multiple reports about people having trouble with this.

Changing our own vocabularies won't help unless we make it a bit smarter. Really, it needs to be able to handle duplicate values, it's very common when using <optgroup> tags, for instance.

Should definitely be among the top things to fix for 3.0.1.

comment:8 Changed 6 years ago by alecm

Don't vocabularies have Values, Terms, Tokens, and Titles (optionally). Couldn't this be handled by using the the title features (which IIRC makes vocabularies considerably more complex). Also, Tokens are supposed to be ASCII only (in this case they are, but it seems a bit wrong to use unicode strings or Messages for them). In any case, as Florian points out, how can two identical options make any sense to a user anyway?

comment:9 Changed 6 years ago by dayne

I have to agree with Limi that this is a critical issue. I wanted to create a customized workflow based on one of the provided ones. I copied/pasted the workflow and renamed the workflow. I noticed the same error as in this issue when I try to access the Types tool in the Plone Site Setup.

I would just modify the names/titles of all my states (and probably need to modify transitions too) but I am not allowed to (I get the error "The object <em>external</em> does not support this operation."). I noticed this is happening in OFS, and suspect the rename failure may be a side effect of this issue as well, but I cannot tell for sure. I thought this worked in Plone 2.5. This all means I have to create my workflows from scratch with unique names/titles.

comment:10 Changed 6 years ago by limi

  • Keywords upstream added

It should at least be able to handle identical values and add an identifier to keep them apart. This is going to happen all the time, whether you think it's the right thing or not. :)

In the initial example, something like:

[ ] Page
[ ] Issue Tracker (Poi)
[ ] Issue Tracker (PloneSoftwareCenter)
[ ] News Item

…would work. And optgroups can also end up being identical and still make sense, as mentioned earlier.

comment:11 Changed 6 years ago by limi

The things in the parentheses would be the ID of whatever item is rendered, if (and only if) there is a title collision.

comment:12 Changed 6 years ago by hannosch

  • Priority changed from critical to major

In the wild I haven't seen too many problems with this and quite a lot of the vocabularies have been changed already, so this is not critical anymore.

comment:13 Changed 5 years ago by hannosch

  • Owner hannosch deleted

comment:14 Changed 5 years ago by anthonygerrard

I don't think this is an issue at all and so should be closed. You're supposed to use the SimpleTerm.title for the label I think, in which case you need to avoid SimpleVocabulary.fromItems.

>>> from zope.schema.vocabulary import SimpleVocabulary
>>> from zope.schema.vocabulary import SimpleTerm
>>> 
>>> terms = []
>>> terms.append(SimpleTerm(value = 'PoiPscTracker', token = 'PoiPscTracker', title = 'Issue Tracker'))
>>> terms.append(SimpleTerm(value = 'PoiTracker', token = 'PoiTracker', title = 'Issue Tracker'))
>>> vocabulary = SimpleVocabulary(terms)
>>> 

As mentioned above if you use a unicode string with non ascii characters for the token you run into problems anyway. I've successfully used SimpleVocabulary using the idiom above to create instances with fields in content types and portlets in Plone 3.3.

comment:15 Changed 4 years ago by jluvsu2

  • Keywords TuneUp23 added; upstream removed

comment:16 Changed 4 years ago by jladage

  • Status changed from new to assigned
  • Owner set to jladage

comment:17 Changed 4 years ago by jladage

  • Milestone changed from 3.x to 4.x

The only place the fromItems method is actively used in plone 3.3 was plone.app.vocubularies. I rewrote that to use SimpleTerm instances instead. Similar errors are occuring in 4.0 thought, when clicking the Memberdata tab in the 'Users and group administration' configlet.

Let's reassign this issue to Plone 4

Please never use the fromItems() method of SimpleVocabulary, it uses the Title as the token, which is expected to be unique and don't support unicode.

comment:18 Changed 4 years ago by jluvsu2

  • Keywords TuneUp24 added; TuneUp23 removed

comment:19 Changed 4 years ago by hannosch

(In [32314]) Avoid the last use of SimpleVocabulary.fromItems in p.a.vocabularies. This refs #6480.

comment:20 Changed 4 years ago by hannosch

(In [32316]) Use the proper SimpleVocabulary/SimpleTerm API instead of encouraging bad practice. This refs #6480.

comment:21 Changed 4 years ago by hannosch

  • Keywords TuneUp24 removed
  • Status changed from assigned to closed
  • Resolution set to worksforme
  • Milestone changed from 4.x to 4.0

I checked the code again and remove all fromItems calls I could find. If these creep up again, we'll need to fix them :)

comment:22 Changed 4 years ago by maurits

(In [38870]) Fixed failing test for by_token in the workflow transition vocabulary, introduced in r31411. Refs #6480

comment:23 Changed 22 months ago by davisagli

  • Component changed from Infrastructure to General
Note: See TracTickets for help on using tickets.