lines property in Zope 5.3: new one has text, old one has bytes #987
Comments
|
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 ( You write
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 I am short on time today, but I will have a closer look on the weekend. |
The change itself seems good to me, don't worry. :-)
On Python 3: sure. And then you switch the project to Python 3, and Where it goes wrong in the The other way around could also happen: new code written for a Zope 5.3 project see a
It's not just code that directly uses the converters. It is also code that calls |
|
👀 |
|
I have a suggestion: Having And it's always best if a system can behave itself in the absence of migrations, even if migrations are going to be performed later. So I suggest having This could be implemented by switching the |
|
Additionally, we could mark this conversion functionality as deprecated and remove it at the next Zope release. |
|
@leorochael's suggestion looks like a good compromise. Any objections or other opinions on this? |
|
That could work. I have meanwhile merged my GenericSetup branch, so no changes are needed in Zope for that. But in other cases it can still be needed. Same for migration code. |
mauritsvanrees
added a commit
that referenced
this issue
Dec 2, 2021
You can call this to fix lines properties to only contain strings, not bytes. It also replaces the deprecated property types ulines, utext, utoken, and ustring with their non-unicode variants. See #987 for why this is needed. Note: the code is not called by Zope itself. The idea is that integrators who think they need this in their database, can call it. Sample use: app.ZopeFindAndApply(app, apply_func=fix_properties) I intend to use this (or a temporary copy) in the Plone 6 upgrade code. See plone/Products.CMFPlone#3305
mauritsvanrees
added a commit
that referenced
this issue
Dec 2, 2021
You can call this to fix lines properties to only contain strings, not bytes. It also replaces the deprecated property types ulines, utext, utoken, and ustring with their non-unicode variants. See #987 for why this is needed. Note: the code is not called by Zope itself. The idea is that integrators who think they need this in their database, can call it. Sample use: app.ZopeFindAndApply(app, apply_func=fix_properties) I intend to use this (or a temporary copy) in the Plone 6 upgrade code. See plone/Products.CMFPlone#3305
mauritsvanrees
added a commit
that referenced
this issue
Dec 2, 2021
You can call this to fix lines properties to only contain strings, not bytes. It also replaces the deprecated property types ulines, utext, utoken, and ustring with their non-unicode variants. See #987 for why this is needed. Note: the code is not called by Zope itself. The idea is that integrators who think they need this in their database, can call it. Sample use: app.ZopeFindAndApply(app, apply_func=fix_properties) I intend to use this (or a temporary copy) in the Plone 6 upgrade code. See plone/Products.CMFPlone#3305
mauritsvanrees
added a commit
that referenced
this issue
Dec 2, 2021
* Add function ``ZPublisher.utils.fix_properties``. You can call this to fix lines properties to only contain strings, not bytes. It also replaces the deprecated property types ulines, utext, utoken, and ustring with their non-unicode variants. See #987 for why this is needed. Note: the code is not called by Zope itself. The idea is that integrators who think they need this in their database, can call it. Sample use: app.ZopeFindAndApply(app, apply_func=fix_properties) I intend to use this (or a temporary copy) in the Plone 6 upgrade code. See plone/Products.CMFPlone#3305 * Update src/ZPublisher/utils.py Co-authored-by: Jens Vagelpohl <jens@plyp.com> * Update src/ZPublisher/utils.py Co-authored-by: Jens Vagelpohl <jens@plyp.com> * Fix unicode properties: mark object as changed when _properties is changed. Co-authored-by: Jens Vagelpohl <jens@plyp.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Zope 5.3 changed the lines property from storing bytes to storing text. See issue #962 and PR #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:The text was updated successfully, but these errors were encountered: