Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure default values get stored by add form #67

Merged
merged 2 commits into from
Feb 26, 2017

Conversation

davisagli
Copy link
Member

This fixes a bug where no value was stored if the value entered in the form is equal to the default value. That can then be quite annoying if the default is calculated in a defaultFactory that changes over time. For example if there is a defaultFactory which returns the current date, the value will always be read as today's date (because __getattr__ also reads defaults) until the item is edited with a value other than the current day's date.

The fix is made by using our own copy of applyChanges (originally from z3c.form) which now takes a force boolean. The add form passes force=True to skip the comparison to the existing value and always store the new value, which is appropriate when adding a new object.

Copy link
Member

@datakurre datakurre left a comment

Choose a reason for hiding this comment

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

LGTM.

Earlier @jensens was worried about possible increase in DB size when we start saving values that were not previously saved. If that's really an issue, maybe framework team should have an opinion.

My personal opinion is that this was a bug, and it's great to see it being fixed with a such small patch. (FWIW, I'm personally continuing forward with a DX version without implicit default value lookup.)

@davisagli
Copy link
Member Author

davisagli commented Feb 26, 2017

Yes, it will store more data for each object. It's my opinion that that is the correct (by which I mean less surprising) thing to do, so that we get the default value from when the object was created, not the current default.

I would also like to backport this for Plone 4.3. Any concerns about that? It is a change, but a bug fix in my opinion. It will only apply to newly created objects; existing items will continue to look up the current default via __getattr__ until a non-default value is saved.

@jensens
Copy link
Member

jensens commented Feb 26, 2017

DB size does not worry me, but we should keep this in mind.

I am +1 for this change. And yes I would consider this a bugfix.

Anyway, for Plone 4.3.x I would add a switch to not use the new way to store on first save. I am quite sure there are some sites out in the wild with customization relying on the old behavior.

@davisagli
Copy link
Member Author

Okay, for 4.3 I can add a way to opt in to the fix. I'll go ahead and merge this to master.

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

Successfully merging this pull request may close these issues.

3 participants