Talk:Backup 2.0 - Converters review 2011-04
Please, for discussing any of the points in the page, do it in the corresponding MDL-xxx issue in the Tracker. TIA! --Eloy Lafuente (stronk7) 22:45, 26 April 2011 (WST)
Ongoing discussion Eloy & Mark (20110428)
Three (3) targets are planned, no action on next target will be performed before completing the previous one:
- The "basic" (1st): Be able to restore everything but user-related information.
- The "desirable" (2nd): Be able to restore activity user-related information.
- The "complete" (3rd): Rest of information.
From the review, points 01-19 must be analyzed, discussed and implemented, specially the ones detected as blockers for the consecution of the targets above. The ones not being considered blockers can be left for later, once the "basic" (1st) target has been fulfilled. The review page will show which ones are considered blockers and where (MDL-xxxx) their implementation is going to happen.
Immediate plan, something like:
- you take a look to those points and add any objection / comment / missing point in the last section of the page.
- I also take a look to the thing and start implementing some stuff, like the start/end dispatchers and some tests to have them under control.
- Next day, availability of parties determined, we share thoughts.
- Next-next day, we start "doing" things :-)
Feedback from Mark regarding review
Related to integration with restore_controller & general behavior
- R0401: I see what you are trying to get at here, but seems like over architecting IMO. For example, you have something that goes from FOO > BAR > MOODLE2 (CONVERTPATH1) and then you have something that goes to FOO > MOODLE2 (CONVERTPATH2) both installed, then your proposal would be necessary. Though, why would you have both installed? CONVERTPATH2 would replace CONVERTPATH1 making CONVERTPATH1 obsolete and never used anyways. Anyways, I'd say let's hold this off as a fun project after everything else is done.
- The problem with current implementation is that, right now, nothing guarantees multiple formats to be supported, so if we want to have (real example) IMSCC => BCK19 => BCK20 current stuff can / cannot work, it depends exclusively on the order returned by convert_factory::converters(). And that doesn't seem to be reliable at all. That's the reason for suggesting some simple "best path" implementation (Dijkstra or similar), just to keep out the "random factor" above. Also, note that, in the future, we can also create converters which origin is, exactly, moodle2, and the target can be IMSCC / BCK19 / WHATEVER (aka exporters), so potentially we could provide automatic conversion between any of them transparently. Say, convert one BCK19 package into one IMSCC or whatever. Just a matter of using the corresponding (best) path.
- R0402: I don't see much of an advantage here as by moving to static you then loose access to all of your instance variables and methods. Cost of creating a new instance shouldn't be that terrible. If this is required, fine, let's do it.
- Surely it's not important now, as far as current converter (moodle1) have nothing important on its constructor. And we have not hundreds of converters. But anything can grow and seems like a logic requirement to avoid instantiation of non-used stuff.
- As for checking the environment, I think that can be done in can_convert right? Part of figuring out if it can handle the conversion.
- But what if one converter needs to create one webservice request over, say, SOAP before performing the conversion, or requires SSL... and the server does not fulfill those requirements. Once again static separated methods sound like the best solution.
- R0403-5: These are fine. We should probably make our own converter_exceptions, what do you think?
- Yes. I think one abstract converter_exception (exactly like backup_exception) is ok, then each converter extends it. Also, the steps/tasks/plans need their exceptions, surely extending base_xxxx_exception
Related to the converters/moodle1 infrastructure
- R0408: There is execute_after_convert but really, that's called after the XML has been processed. Execution flow is kind of weird due to the processor. We could add another stage of processing, though I do think execution flow will greatly improve once start/end callbacks are being used.
- Agree, I think start/end dispatchers will alleviate a lot current after_execute() code. So perhaps we need to implement R0409 ASAP (priority1) and move stuff there to see how things improve.
- R0410: I'm not sure what this is.
- I mean, right now, the get_deprecated/renamed/new() methods belong to the moodle1_structure_step. And that is ok as far as there aren't collisions (repeated tag names in the tree being processed by the step). What if we have the "timemodified" tag, both in the forum and in the forum_post elements and we want them to be "renamed" diferently. That caused me to think that such transformations should be defined on each convert_path_element and not in the steps (to avoid those collisions). And then, the convert_data() method could be applied automatically, so the $data arriving to the convert_xxxx($data) methods could have all those transformations (and lowercase too) automatically performed. Just that.
- Rest are fine.
Related to coding in general
- Sounds fine.