plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
245 stars 188 forks source link

Best practices to update registry records? #3827

Closed gforcada closed 8 months ago

gforcada commented 1 year ago

At work whenever we have change our profiles/default/*.xml we create a corresponding upgrade step to update those files on the Plone site.

Mostly it boils down to this function:

def update_gs_profile(profile_id=PROFILE_ID, tool=None):
    """Imports the given tool Generic Setup step from the given profile."""
    if tool is None:
        logger.critical('Needs a tool to run!')

    setup = api.portal.get_tool('portal_setup')
    setup.runImportStepFromProfile(profile_id, tool)

i.e. given a GS profile and a tool to update, it updates the configuration for that profile.

For registry.xml we have:

def update_gs_registry(profile_id=PROFILE_ID):
    """Imports registry.xml setup step."""
    update_gs_profile(profile_id=profile_id, tool='plone.app.registry')

What I found interesting is that given such a registry.xml file:

<?xml version="1.0" encoding="utf-8"?>
<registry>
  <records interface="freitag.one.interfaces.ISomeSettings" />
  <records interface="freitag.one.interfaces.ISomeMoreSettings" />
  <record name="freitag.one.a-url">
    <field type="plone.registry.field.URI">
      <title>A URL</title>
    </field>
    <value>https://example.org</value>
  </record>
</registry>

Running the update_gs_registry function mentioned above installs the new registry record freitag.one.a-url with the expected value.

I can now, happily go to the registry control panel and update the setting, all good and nice ✨

Now, comes the bug: 🐞

If I re-run the upgrade step (i.e. call again update_gs_registry) the value is reset to the one coming from registry.xml, my manual changes are lost!

Ok, looking at plone.app.registry documentation I can fix that by adding a purge="false" attribute on value, i.e. <value purge="false"></value>.

Still not, it gets overwritten again 🐞

So, what are the best practices when having this sort of registries? Never to call runImportStepFromProfile on it? πŸ€”

Interestingly enough, for the interfaces freitag.one.interfaces.ISomeSettings that define some records, they are not overwritten...

At least that feels inconsistent to me.

I was expecting that if a registry already exists is left alone, or that at least, you can hint GenericSetup to ignore the new value (by providing this purge as mentioned on the documentation)...

ale-rt commented 1 year ago

Maybe related https://github.com/plone/Products.CMFPlone/issues/1375

ale-rt commented 1 year ago

I have very little time, but AFAIK the solution in many case is to set the value through Python code

davisagli commented 1 year ago

I'm not sure what the best solution is but I'll give some thoughts...

I think this is designed to work either of 2 different ways:

  1. Specify all registry values in GenericSetup files, and never edit them through the web
  2. Use the GenericSetup values to add fields but not values, and always edit the values through the web.

With either of these approaches, it's safe to re-apply the registry step in an upgrade step, because it will not overwrite values. Where you run into trouble is sometimes setting values with xml and sometimes through the web, and not keeping them in sync.

purge="false" has a different purpose than what you are expecting. It is used with sequences of values (i.e. a list) to control whether the existing value should be purged (reset to empty) before appending the values in the xml file. Perhaps it could also be used for the purpose you have in mind, to control whether an existing value should be overwritten. But I think it would be less confusing if that was a new setting with a new name, maybe overwrite_existing="false".

Interestingly enough, for the interfaces freitag.one.interfaces.ISomeSettings that define some records, they are not overwritten... At least that feels inconsistent to me.

That doesn't seem inconsistent to me. The standalone record has a value, so importing it sets the value. The records based on an interface do not include any values, so importing them does not set a value.

Maybe what you want is a registry.xml in the default profile which does not set any values and can be safely used in an upgrade step, and then a separate registry.xml as part of an initial profile that sets values and is only used once.

mauritsvanrees commented 1 year ago

I agree with what David said.

Perhaps it could also be used for the purpose you have in mind, to control whether an existing value should be overwritten. But I think it would be less confusing if that was a new setting with a new name, maybe overwrite_existing="false".

Years ago, when the configuration registry was still being developed, I had hoped that something like this would be done, but it never happened, except that you can register an interface without overwriting anything.

BTW, I would write the upgrade in zcml like this:

  <genericsetup:upgradeDepends
    source="1000"
    destination="1001"
    profile="your.package:default"
    title="Update registry"
    import_steps="plone.app.registry"
    />
gforcada commented 8 months ago

Thanks! now I see that there was indeed some misunderstanding from my side 😊