jugmac00
commented
3 years ago Hi @mauritsvanrees,
thanks for investigating and sorry that my change caused you so much work.
I have never used properties, but when I have a look at our documentation ( https://zope.readthedocs.io/en/latest/zdgbook/Products.html#ofs-propertymanager ) there is no mention of bytes at all.
I also lack imagination, why I ever want to store bytes on an object. Though I never used properties, I could imagine they are used for e.g. dates, delivery numbers or whatever...
Usually it is a good idea to convert from bytes to text at the outer most border of a system. But maybe I oversee some use-cases.
As of your suggested fixes - I have a hard time to imagine what people used a list of bytes for.
I do not like the idea to have a getter (getProperty) which does not only get but changes the underlying data - just when reading or even permanent, so an upgrade step which converts the data would be preferable.
You write
Other packages which do similar things, would need to update their code as well.
Are you aware which other packages use the converters?
I just grepped the zopefoundation repos, and it looks like there is only one more repository Products.CMFCore:
❯ all-repos-grep -C zope.json type_converters '*.py'
output_zope/zopefoundation/Products.CMFCore:src/Products/CMFCore/MemberDataTool.py:from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Products.CMFCore:src/Products/CMFCore/MemberDataTool.py: if proptype in type_converters:
output_zope/zopefoundation/Products.CMFCore:src/Products/CMFCore/MemberDataTool.py: value = type_converters[proptype](value)
output_zope/zopefoundation/Products.GenericSetup:src/Products/GenericSetup/utils.py:from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Products.GenericSetup:src/Products/GenericSetup/utils.py: if prop_type in type_converters:
output_zope/zopefoundation/Products.GenericSetup:src/Products/GenericSetup/utils.py: # The type_converters use the ZPublisher default_encoding
output_zope/zopefoundation/Products.GenericSetup:src/Products/GenericSetup/utils.py: prop_value = type_converters[prop_type](prop_value)
output_zope/zopefoundation/Products.GenericSetup:src/Products/GenericSetup/utils.py: prop_value = type_converters[prop_type](prop_value)
output_zope/zopefoundation/Zope:src/OFS/PropertyManager.py:from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Zope:src/OFS/PropertyManager.py: if proptype in type_converters:
output_zope/zopefoundation/Zope:src/OFS/PropertyManager.py: value = type_converters[proptype](value)
output_zope/zopefoundation/Zope:src/OFS/PropertyManager.py: if type in type_converters:
output_zope/zopefoundation/Zope:src/OFS/PropertyManager.py: value = type_converters[type](value)
output_zope/zopefoundation/Zope:src/OFS/PropertySheets.py:from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Zope:src/OFS/PropertySheets.py: if proptype in type_converters:
output_zope/zopefoundation/Zope:src/OFS/PropertySheets.py: value = type_converters[proptype](value)
output_zope/zopefoundation/Zope:src/OFS/PropertySheets.py: if type in type_converters:
output_zope/zopefoundation/Zope:src/OFS/PropertySheets.py: value = type_converters[type](value)
output_zope/zopefoundation/Zope:src/ZPublisher/BaseRequest.py:from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Zope:src/ZPublisher/BaseRequest.py: return list(type_converters.keys())
output_zope/zopefoundation/Zope:src/ZPublisher/Converters.py:type_converters = {
output_zope/zopefoundation/Zope:src/ZPublisher/Converters.py:get_converter = type_converters.get
output_zope/zopefoundation/Zope:src/ZPublisher/tests/testHTTPRequest.py: from ZPublisher.Converters import type_converters
output_zope/zopefoundation/Zope:src/ZPublisher/tests/testHTTPRequest.py: for type, convert in list(type_converters.items()):I am short on time today, but I will have a closer look on the weekend.
mauritsvanrees
leorochael
Zope 5.3 changed the lines property from storing bytes to storing text. See issue https://github.com/zopefoundation/Zope/issues/962 and PR https://github.com/zopefoundation/Zope/pull/965.
What now happens:
So depending on when you created or edited the lines property,
getPropertyin Zope 5.3 may return bytes or string. I don't really know if this is a problem or not.But there are already some test failures in GenericSetup, which is why I started looking. Apart from some expectations in tests which need to be updated, GenericSetup needs a few fixes in the part of its code where it is adding or updating properties, so the affected code is limited. See PR. The most difficult about that PR, is that there is code in GenericSetup which supports non-utf8 encodings in profile files, so the changes are more convoluted than they should have been. Other packages which do similar things, would need to update their code as well.
If we think this should be fixed in Zope, I can see two solutions:
getPropertycall the converter of the field and return the result. But this means extra code gets run whenever you callgetProperty. Not nice for performance.zopeFindAndApply. Plone could choose to do this in a GenericSetup upgrade step, although this may take long when your Data.fs is 100 GB.Additionally, a function could be useful to go over all deprecated properties, for example turning
ulinesintolines. The two functions could be combined.Does someone already have code like this? Even when Zope itself does not call the function anywhere, having it available in Zope would be useful. cc @jugmac00
O, let me share some output from debugging. I started with Zope 5.2.1 and several properties containing non-ascii:
äh. In the ZMI this looks fine in all fields, except for thelinesfield, where it looks wrong:äh. Withbin/instance debug:Save it a couple of times, and the ZMI show
äh:So a
linesfield with non-ascii in older Zopes was already wrong: you should have used aulinesfield. So probably the real problems are only there when you already had a problem in the earlier Zopes.Start in Zope 5.3 and it it still looks wrong, because the original data is bad. Saving it once (or multiple times), changes the bytes to string, but it is still looking wrong:
Manually insert the proper
ähin the ZMI and save, and then it is fine: