Skip to content

Respect purge=False on simple field types#88

Open
gforcada wants to merge 3 commits into
masterfrom
73-no-overwrite-textlines
Open

Respect purge=False on simple field types#88
gforcada wants to merge 3 commits into
masterfrom
73-no-overwrite-textlines

Conversation

@gforcada

@gforcada gforcada commented Aug 4, 2024

Copy link
Copy Markdown
Member

Closes #73

🎉 Turns out that the fix is rather small and it's great to work on a repository that has good enough test coverage 🤩

When `purge=False` is set.

Up until now, it was only the case for lists, tuples or dictionaries,
but not for text, ASCII, URL etc.
@mister-roboto

Copy link
Copy Markdown

@gforcada thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@gforcada

gforcada commented Aug 4, 2024

Copy link
Copy Markdown
Member Author

@jenkins-plone-org please run jobs

@davisagli davisagli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will definitely not approve this without documentation, because I am a little bit unclear on what you are trying to accomplish.

My initial assumption was that purge="False" on a simple value would mean "set this value, unless there is already a value for this field" (that would be a breaking change, btw, so I'm not sure we can do it).

But that's not what this implementation is doing. There's no change to what happens to an existing record if the new value is truthy. If it's falsy, it won't be set if the xml has purge="false". This doesn't make any sense to me.

@mauritsvanrees

Copy link
Copy Markdown
Member

I think the "purge" keyword only makes sense on fields that have lists of elements. For example the Tuple plone.types_not_searched or the List plone.alignment_styles.

There you have these meanings:

  • purge=False: keep the current elements and add new ones from the XML.
  • purge=True: remove the current elements and add all elements from the XML.

For simple fields like TextLine or URI it currently overwrites the value, no matter what you have in purge. Changing this would be a breaking change for people like me who rely on the previous behavior. If I create an upgrade step/profile and set a new value for a registry record, I expect that this value is set and overwrites a previous value.

I think what you want could only be done with a new keyword, something like overwrite or initial_only or only_set_when_empty or only_set_when_undefined or only_set_when_value_is_default. As you can already see from these suggested names, it could be tricky to know when we would be allowed to set a value:

  • If the record is not there yet: obviously create it and set the value.
  • If the record is there but the value is None: sure, set it. (I am not sure from the top of my head if 'None' can ever be set, maybe this is when you have no default.)
  • If the value is an empty string, empty list, empty whatever: probably set it.
  • But what if the field has a default value and this is also currently the value of the record? Should we change that as well? Or if a Site Admin has explicitly emptied the value, should we then change it? Can we somehow know this?

Maybe only_set_when_empty would fit most use cases. By default this would be False, so there is no difference to how it currently works. Something like this would certainly be useful.

Additionally only_set_when_default="true" would check if the current value is the same as the default for the field, and only set the new value when the current value differs. Checking the default could be trickier, especially if the default could be determined by a factory, or needs checking with a vocabulary.

With only_set_when_empty="true", we would not need to check the "purge" value for this record, as purge=true would be implied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

registry purge=False has no effect.

4 participants