Plugin saving and hook workflow should be reviewed
There are some problems with the plugin saving workflow and hook system. Here is how it should work in my opinion: For an object creation: Run the precreate hooks of main tab-> if one fails, show the error and cancel the creation. (problem: in FD code, at this point it’s difficult to salvage the data, we would end up on the management class as we do for LDAP errors) Run the ldap save of main tab -> if it fails, cancel the saving (same as above) Run the postcreate hook of main tab -> failure has no effect other than showing an error For each tab:
- Run the precreate hooks of the tab -> if it fails, cancel the activation of this tab? (this will be complicated with tab dependencies and all this)
- Run the ldap save of the tab -> if it fails, cancel the ldap save of this tab (meaning do not run post saving stuff)
- Run the postcreate hooks of this tab -> failure has no effect other than showing an error (It’s almost the same for modification but with modify hooks)
What I mean by failure: If a script returns an other code than 0, it’s considered an error. It if outputs texts while returning an other code than 0 this text is considered the error text. If it outputs text but still returns 0 code it’s considered success but the text is shown in an info pop-up (All this is already in place as far as I know)
So the main changes from present situation would be:
- Failed precreate hook on main tab should cancel creation.
- Failed LDAP save should cancel postcreate/postmodify hooks from happening, and logging should not log the creation/modification, only the error.
- Failed LDAP save of main tab should cancel the save of other tabs (as they cannot succeed anyway).
I will add information in this ticket as the planning, thinking, coding or testing evolves.
(from redmine: issue id 4893, created on 2016-06-21, closed on 2017-02-23)
- Relations:
- relates #2126 (closed)
- relates #4891 (closed)
- relates #3789 (closed)
- relates #4917
- relates #5187
- relates #5411
- relates #5551
- relates #5565
- Changesets:
- Revision 30a5d2ac on 2016-07-11T09:55:48.000Z:
Fixes #4893 Removed unused cleanup attribute from simplePlugin::ldap_save
- Revision dacddb21 on 2016-07-12T05:25:08.000Z:
Fixes #4893 changed simplePlugin::save behavior
- Revision d1f598b2 on 2016-07-12T07:02:42.000Z:
Fixes #4893 Refactored tabs saving and checking
- Revision a4a0c445 on 2016-07-12T07:19:56.000Z:
Fixes #4893 ldap_save should always return an array even if empty
- Revision 0a678cfa on 2016-07-12T10:45:01.000Z:
Fixes #4893 Oops
- Revision 25278f63 on 2016-07-12T10:55:52.000Z:
Fixes #4893 Added missing global $config in prepare_save
- Revision 81826968 on 2016-07-12T11:18:06.000Z:
Fixes #4893 Fixed var name in simpleTabs
- Revision b571d4ed on 2016-07-13T05:05:43.000Z:
Fixes #4893 Returning errors from ldap_save
- Revision 7c7e8fb8 on 2016-07-13T05:06:55.000Z:
Fixes #4893 Fixes in userRoles about new errors handling
- Revision e8715659 on 2016-07-13T05:28:11.000Z:
Fixes #4893 first go at fixing copy/paste for new save workflow
- Revision e298e4d3 on 2016-07-13T05:41:48.000Z:
Fixes #4893 Fixed syntax errors
- Revision 4ba9d5e2 on 2016-07-13T05:58:05.000Z:
Fixes #4893 Removed last call to check() from copyPaste
- Revision a35cf153 on 2016-07-13T07:05:58.000Z:
Fixes #4893 fixed userRoles
- Revision 8fc857d2 on 2016-07-27T10:14:57.000Z:
Fixes #4893 Fixed remove workflow too
- Revision 28758603 on 2016-07-28T15:46:34.000Z:
Fixes #4893 Fixed configuration tabs save method
- Revision d21673db by Côme Chilliet on 2016-09-21T12:45:50.000Z:
Fixes #4893 Fixed a few cases of error handling
- Custom Fields:
- Bug in version: 1.0.13