Ticket #9674 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

Enabling versioning for file type fails for pre-existing files.

Reported by: sandyreid Owned by: alecm
Priority: major Milestone: 3.3.5
Component: Versioning Version:
Keywords: Cc: tom_gross

Description

Plone Version Overview

  • Plone 3.3.1
  • CMF 2.1.2
  • Zope (Zope 2.10.9-final, python 2.4.3, linux2)
  • Python 2.4.3 (#1, Jul 27 2009, 17:57:39) [GCC 4.1.2 20080704 (Red Hat 4.1.2-44)]
  • PIL 1.1.6

I've just enabled versioning for files (in setup/types). I now have a history tab for pre-existing files, but when I try to access them I get an error message (traceback attached below). If I add a new file, it's history tab works as expected. The problem seems to be with files which were added to plone before enabling versioning.

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 Shared.DC.Scripts.Bindings, line 313, in call Module Shared.DC.Scripts.Bindings, line 350, in _bindAndExec Module Products.CMFCore.FSPageTemplate, line 216, in _exec Module Products.CacheSetup.patch_cmf, line 51, in FSPT_pt_render Module Products.CacheSetup.patch_cmf, line 126, in PT_pt_render

  • Warning: Macro expansion failed
  • Warning: exceptions.KeyError: 'view_macro'

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 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 861, in do_defineMacro 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 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: file:/home/xxx/webapps/zope/buildout/eggs/Products.CMFEditions-1.2.4-py2.4.egg/Products/CMFEditions/skins/CMFEditions/versions_history_form.pt
  • Line 7, Column 2
  • Expression: <PythonExpr history.getLength(countPurged=False)>
  • Names:

{'container': <PloneSite at /Artik>,

'context': <ATFile at /xxx/information/administrator-information/test-pages/test-docs/test-word-doc>, 'default': , 'here': <ATFile at /xxx/information/administrator-information/test-pages/test-docs/test-word-doc>, 'loop': {}, 'nothing': None, 'options': {'args': ()}, 'repeat': <Products.PageTemplates.Expressions.SafeMapping object at 0x1015f5ec>, 'request': <HTTPRequest, URL= http://xxx/information/administrator-information/test-pages/test-docs/test-word-doc/versions_history_form>, 'root': <Application at >, 'template': <FSPageTemplate at /xxx/versions_history_form used for /Artik/information/administrator-information/test-pages/test-docs/test-word-doc>, 'traverse_subpath': [], 'user': <PropertiedUser 'xxx'>}

Module Products.PageTemplates.ZRPythonExpr, line 49, in call

  • traceback_info: history.getLength(countPurged=False)

Module PythonExpr, line 1, in <expression>

AttributeError: 'list' object has no attribute 'getLength'

Change History

comment:1 Changed 4 years ago by esrever_otua

We've hit this too.

This is caused by the fact that in ZVCStorage.py getHistoryMetadata returns and empty list in the case of a non-existent history, but the template (versions_history.pt) assumes that (a) a history item will always be returned, and (b) that the history is a LazyHistory, which implements a 'getLength' function, instead of just using the builtin len. This could be solved by either:

a) testing for 'truthiness' of history before trying to use methods/attributes on it in versions_history.pt

b) returning an empty shadowhistory object. this looks tricky because of the assumptions baked into the code

c) changing policy of len implementation to set countPurged=False, and then just use len(history) in the template versions_history.pt

d) Pick your own solution here :)

comment:2 Changed 4 years ago by esrever_otua

  • Priority changed from minor to major

comment:3 Changed 4 years ago by esrever_otua

Actually, after a second look at the code, it seems to me that best/most elegant solution, is simply to do away with the various tests in ZVCStorage.ZVCStorageTool.getHistoryMetadata, and instead simply do:

return self._getShadowHistory(history_id, autoAdd=True)

Also, while I'm here, the code in CopyModifyMergeRepository.CopyModifyMergeRepositoryTool.getHistoryMetadata looks a little hokey - why the special test to decide whether to wrap hist in ImplicitAcquisitionWrapper or not? Probably that should be junked with a simple: return ImplicitAcquisitionWrapper(hist, self) ...regardless of what 'hist' is (None, an empty list, or an empty ShadowHistory...)

comment:4 Changed 4 years ago by esrever_otua

Woooweee, this just gets better and better; further to the suggestion above, plone.app.layout.viewlets.content, in getRevisions has a special shortcut where they comment that 'history might be an empty list' and then shortcut to simply return it. This breaks when history is now (correctly) and empty ShadowHistory object, so this special test and shortcut should be removed.

comment:5 Changed 4 years ago by esrever_otua

Sorry - not getRevisions, revisionHistory:

        # History may be an empty list
        if not history:
            return history

comment:6 Changed 4 years ago by tom_gross

  • Cc tom_gross added

comment:7 Changed 4 years ago by alecm

  • Status changed from new to closed
  • Resolution set to fixed

I think solution a) is by far the simplest and involves a single simple change to the version_history_form. The ShadowHistory is a persistent object and I'm not comfortable constructing and returning a dummy empty version of it when the return of _getShadowHistory is None. Just passing the None slightly simplifies the getHistoryMetadata method, but would require changes elsewhere.

This has been fixed in collective r111460 and merged into the 1.2 branch in r111461.

Note: See TracTickets for help on using tickets.