-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
control panel throws exception if accessed before addon upgrade step that adds field (controlpanel serializer so it handles schemas with new fields by returning field default) #1736
base: main
Are you sure you want to change the base?
Conversation
…h new fields by returning field default
✅ Deploy Preview for plone-restapi canceled.
|
@djay thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment:
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be confusing when a default is calculated every time the field is read. For example if there's a field with a defaultFactory that returns the current userid, then it will look like a different value is stored depending on which user reads the value. For that reason, I would be inclined to require an explicit upgrade step if a new field is added that uses defaultFactory.
I would also suggest putting DefaultRecordsProxy in plone.registry so that it can be used by both plone.restapi and plone.api instead of duplicating it. That does make things a bit more complicated since plone.restapi needs to potentially handle old versions of Plone that don't have it.
Well it does only fix the case of accessing hte control panel so the user will see their name and if they hit save then it persist.
Thats the case now that this is trying to solve. Control panels break until you upgrade your plugin which is confusing for admins. There is no nice explanation why, just a stack trace. And in addition its just extra work for the developer to go put in explicit upgrade steps that rerun your profile or add that specific missing data in the registry when they can just specify a default in the schema instead (the more intuitive thing to do) A more complete solution is to have different versions of the schema interface linked to a particular installed version and use the old one until the upgrade step is run. Or maybe fields in a schema linked to a version? But all of this also adding more work for the developer. Or we solve this by showing a nicer error to users asking them to upgrade the plugin?
|
My concern is that they are seeing a value that is not actually persisted. If it's the value they want, they'll probably think they don't need to save. It's not really a problem for a static default value, but more of a potential problem if there's a defaultFactory that returns different defaults at different times. An unrelated question: It looks like the existing RecordsProxy already falls back to the field's |
The You are right. the semantics of using this make more sense that using the default. Not sure developers would intuitively think to use missing_value but either way documentation would be needed. |
@djay Hmm, no, it's not that simple. It'll use the The registry design stores a persistent field definition as part of a record; I expect it will be an uphill battle to get things to work correctly without an upgrade step to add records for the new fields. |
You are right again. my test doesn't properly test setting since I didn't actually remove the value from the registry
It does? I wasn't aware. if so why can't the recordsproxy use that to check against instead of the current Interface? Then it will be checking against the schema that was there at the time it was installed which would also solve the problem I think? EDIT: ok it wouldn't... because the control panel view would also have to display widgets etc based on the schema stored in the registry for this to work. |
…lone.restapi into fix_upgraded_controlpanel_schema
@davisagli my test did delete the record so does show that the value can be set again in that case. I've changed it to use the missing value and the test passes. |
To reproduce.
Ultimately the issue is that schemas aren't versioned and they are loaded via code not configuration so there is no way to properly manage schema migration in plone/zope. This applies to dexterity schemas too.
In the absence of that, not breaking and returning something sensible when data is missing (the default) seems like a good way forward.
What this change does is make the restapi return the default defined in the schema if it can't find the data in the registry.
This change also means that in many cases you don't need an upgrade step for your addons that add fields. But until the control panel is saved again the value in the registry will still be missing. Not sure if that creates other problems.
Note this is a problem for classic as well (as pointed out by @fredvd ) so a similar fix might be needed there too?