added a comment - - edited
Just reviewing all the pending points and cases and putting all them here
(I've annotated them as critical, important, medium, low just in case you need to prioritize them):
- P6 - LOW - The output singleton: That singleton was added there as a way to separate any output from process completely. And also, allow easy extension along the time (providing other visualizations of the progress, right now it's a simple "lister"). Its only relation with loggers is that we can "flag" logger actions to be also sent to output if desired but it isn't in charge of outputting logs (each logger has its own output defined, independent of the singleton).
So, along the next days... I'll be adding log() - backup_helper::log() - calls here and there, and some of them will have the $display param enabled, hence will be sent to singleton, also, there will be some direct calls to output_controller::get_instance->output() sending other bits to the singleton.
So, from the backup ui POV, one "list" (html/text) will be set to output (execution stage). Just that. I don't think we need any integration (ui and output don't need to "meet" the other at all). But, perhaps, instead of the current auto-redirect at the end of the stage... we should show one "continue" button, to give the user the possibility of reading the output and so on. Just that.
- P13 - MEDIUM - Filling filename: both format and type are controller's format and type. Same for id. In the other side, users and anonymised are the values of those settings. I was thinking about to set the default for filename = ''. Then, when it's firstly "rendered" or harcoded in that stage page, fill it if empty with results from the function. Also note that that filed must be validated to be non empty, PARAM_FILENAME and end with .zip (we are missing those validations right now, my fault I didn't specified them anywhere).
- P24 - CRITICAL - Security checks, for users not having the 'moodle/backup:userinfo' (teachers by default), sets the 'users' setting to value = false and status = LOCKED_BY_PERMISSION. But... once the UI is shown... the setting isn't "locked" at all. Worst, teacher is able to set it back to yes (enabling a lot of dependent settings). Luckily, it breaks in schema page, but they shouldn't be able to change that setting at all. LOCKED_BY_PERMISSION should be impossible to unlock / change value. Curiously, if you go to the schema page without changing it to "yes", in that page it appears with the tiny lock icon. To reproduce, just try one backup as teacher.
- P25 - LOW - There is one new capability ( 'moodle/backup:configure' ) that specifies if the user can configure any of the backup settings or no. Form the UI POV it means that users lacking the capability should go straight to the "confirmation" stage without possibility to edit anything. Not critical, as far as, by default, all the teachers have it allowed, but once ppl start playing with the capability... it should be working as commented.
- Case 1 - IMPORTANT - dependencies not working "on cascade" in mform.
activity_included dependent of section_included
section_userinfo also dependent of section_included
activity_userinfo dependent of section_userinfo
Expected behavior: by unchecking section_included, all the rest (section_userinfo, activity_included and activity_userinfo) should become unchecked and disabled.
Current behavior: section_userinfo and activity_included are disable (not unchecked) and activity_userinfo continues enabled and checked.
Note: If I go to the confirmation stage, I see all them properly set to no and locked. So I guess it's some failure only at the mform level, not supporting "chained" changes to happen. Can this be fixed in any way? Or do I need to explicity create a new dependency to make activity_userinfo directly dependent of section_included? From a backup architecture pov sounds far from perfect, as far as "chained" dependencies work there.
Note2: Perhaps if the chained thing is a "hard" limit in the mforms stuff, when creating it, we should make to follow all the chain-tree (down-up) and accumulate disableif rules from all the possible "ancestors" ? Of course, if they can work in a chained way (it isn't a hard limit in mforms, better).
- Case 2 - IMPORTANT - some "direct" dependencies not disabling dependent settings.
activity_included dependent to root activities setting
activity_included is also dependent of section_included
activity_userinfo is dependent of action_included
Expected behavior: by selecting "no" in root activities setting, all the activity_included should become unchecked and disabled (directly) and all the activity_userinfo too (this indirectly).
Current behavior: activity_userinfo settings are shown properly unchecked and disabled (the indirect one!) but activity_included (direct relation!) shows them unchecked but enabled (not locked)
Note: This seems to be against the Case 1 above, so here we have indirect dependencies working (although between different stages). The strange point is why the activity_included settings aren't disabled.
Note 2: Perhaps it's because of the other dependency they have (with section_included?). If so, we are back somehow to P21, so some dependencies are affecting to others improperly, unlocking things that should remain locked by another dependency.
- PMEMORY - MEDIUM - TO RESEARCH - Finally one comment that, while not important for 1-backup can have high relevance when implementing multiple backups (in the same request) like scheduled backups / category backups...
I've noticed that, after putting all the ui machinery to work, in my test server, memory usage has grown from 10M to 15M for the same course. Worst, executing in the same script 5 backups, caused each one to raise memory 1M, and now it's near 2M (I guess this is because of the new circular references introduced that php isn't able to destroy properly and now we far more than before).
So this is one problem to take into account when implementing multiple-backups unless we find one way to properly destroy all those references, so php will be able to gc that memory. Nor critical for now... but could become...