plone / plone.app.registry

provides Plone UI, persistence and GenericSetup integration for plone.registry
https://pypi.org/project/plone.app.registry
2 stars 6 forks source link

registry purge=False has no effect. #73

Open thet opened 1 year ago

thet commented 1 year ago

I'm importing the following registry entry in an upgrade step:

<?xml version="1.0"?>
<registry>

  <record name="ploneintranet.layout.hocuspocus_url">
    <field type="plone.registry.field.TextLine">
      <title>The hocuspocus URL</title>
      <description>
        This is the URL that will be used to communicate with the hocuspocus server.
        We will try to connect to it using a websocket.
      </description>
      <required>False</required>
    </field>
    <value purge="False"></value>
  </record>

</registry>

The previously set registry values are overwritten, which is unexpected. With purge="False they should be kept.

Applies to: plone.app.registry-1.7.9-py3.8.egg

mauritsvanrees commented 1 year ago

Related: https://github.com/plone/plone.app.registry/issues/42

But in your case I think it is not an error. This is a TextLine, so it will contain a one-line string, and purge=false has no meaning there. You get to this condition. Since the value is a string, all the conditions below this line will be false: it is no list, no tuple, etc. So in the end it sets the value.

thet commented 1 year ago

Thanks @mauritsvanrees

42 shows me I stumbled over the same problem twice within a month :)

However, with purge=False I would still expect the existing value to not being overwritten if it would evaluate to boot(existing_value) == True. It still fells like a bug.

gforcada commented 4 months ago

I just got bitten by this as well 😕

davisagli commented 4 months ago

purge=False has always been used for keeping the existing items in a list, not for preventing a single value from being overwritten. You're expecting it to do something different than what it does. But, I could imagine making it work like you expect as a new feature for values that are not lists.

gforcada commented 4 months ago

I would rather say that this is a bug.

My use case:

  <record name="shopping-list">
    <field type="plone.registry.field.List">
      <title>What you need to go buy?</title>
      <value_type type="plone.registry.field.TextLine" />
    </field>
  </record>

note that there is no reference to the <value>

In this case, I'm explicitly not giving any value to the record as I'm expecting to keep changing this value on the fly, actually editors of the website are meant to change them.

gforcada commented 3 months ago

🤦🏾 now I see that my example is actually a List that has TextLines as values, so a simple <value purge="False"> is enough to fix my problem 😓

gforcada commented 3 months ago

Seems to happen also with ASCIILine and URI fields as well.

Either a bug or a new functionality, should it be fixed though? I'm 👍🏾 on it, as I'm reviewing my registry.xml files and I see quite a few fields that will get overwritten next time that I'm re-importing that profile's registry (and actually in a few months I will have to do it 😅 )

gforcada commented 3 months ago

Interestingly enough, if an ASCIILine or TextLine is part of an interface, then values do not get overwritten.

gforcada commented 3 months ago

Related: #42

But in your case I think it is not an error. This is a TextLine, so it will contain a one-line string, and purge=false has no meaning there. You get to this condition. Since the value is a string, all the conditions below this line will be false: it is no list, no tuple, etc. So in the end it sets the value.

Actually you don't get to the list/tuple/dict if/elif cases because in the previous check:

if not value_purge and new_type == existing_type:

new_type is Nonetype and existing_type is str or the likes.

This check should be changed to also account for purge=False IMHO. I'm looking into it right now, let's see if I can make a PR that passes all the tests and the new ones I wrote to debug the issue 🤞🏾 🍀

gforcada commented 1 week ago

I'm avoiding this problem by changing our approach on upgrades: rather than importing the distribution profile's registry.xml, I'm creating (like plone.app.upgrade) small profiles for the updates.

This way, we avoid this problem.

Though, at least, we should add some sort of warning or make it clear on the documentation...