Advice wanted: what is the best way to get a large change integrated

Advice wanted: what is the best way to get a large change integrated

by Tim Hunt -
Number of replies: 8
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

There is a feature I want to develop. For this question, it does not matter what the feature is (but if you are curious, it is MDL-74610).

This feature fits into some bits of Moodle that were written a long time ago: mod/quiz/edit.php, associated JS, which I think is still in YUI, Ajax stuff there hitting mod/quiz/edit_rest.php, rather than wervices, not using templates, classes defined in files like mod/quiz/attemptlib.php, not the classes folder ...

I think that my new feature presents a good opportunity to clean up some or all of this mess, and I would like to do so, but I am also a bit apprehensive about what it would take to get this through Moodle's integration process, so I wondered if anyone had any advice?

My thinking is: it needs to be done a a series of small steps. And, probably those steps are:

  1. Move all the classes to the classes folder (I made MDL-74923 for that).
  2. Replace all HTML genearation in mod/quiz/classes/output/edit_renderer.php with templates (without, at this stage, doing many changes to the HTML produced).
  3. Rewrite associated YUI modules -> AMD (without, at this stage, changing the ajax)
  4. All the library code that currently powers edit_rest.php (and other places that are not currently ajax-y like quiz_add_questoin), consolidate that back-end code into mod_quiz\structure class (and deprecate old stuff)
  5. Replace edit_rest.php with services, built using the structure class, and update the JS to use them.
  6. ... then, and only then, might I start on the new feature.

(Not sure that list is in quite the right order, or the right items, but it is the best I have come up with so far.)

But, I look at that list, and then think: even if that gets integrated at the rate of one issue per week, if I start now, I won't acutally get to working on the new feature until Feburary (what with Christmas). And, one-week turn-around on integration just seems impossibly ambitious based on recent history (although I know the I-team are trying to improve this.) So, do I even start?
Average of ratings: -
In reply to Tim Hunt

Re: Advice wanted: what is the best way to get a large change integrated

by Andrew Lyons -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Hi Tim,

I'm still digesting and thinking about this, but I would suggest an additional commit after point 1 (part of the same issue) to address all linting failures as a separate commit. It may also be worth running something like phpstan to identify any other potential issues expected in PHP 8.1, 8.2, or 8.3 as a part of this (probably a separate commit to the initial lint fix).

Also, I believe there are a couple of classes which pre-date our Level 2 namespace rules, so perhaps they can also be addressed at the same time.

I'm still thinking about the rest of this,

Andrew
Average of ratings: -
In reply to Tim Hunt

Re: Advice wanted: what is the best way to get a large change integrated

by Michael Hughes -
Picture of Core developers Picture of Plugin developers
As an aside, I think if even developers that are as experienced with Moodle as yourself are asking about guidance on shepherding a new substantial change through Moodle's process, then there is a concern about the process itself. This sort of apprehension I think is fairly common and discouraging for those who may want to consider "sweeping" enhancements and how to "land" them coherently.

Unfortunately I have no constructive advice, but would like an answer to this and I wish you the best of luck!

MH
Average of ratings: -
In reply to Michael Hughes

Re: Advice wanted: what is the best way to get a large change integrated

by Andrew Lyons -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Hi Michael,

I think that, whilst the process does have certain expectations, I don't necessarily think that this is a problem with the process (at least , not in this instance).

Tim is asking for advice on refactoring a large chunk of fairly important and integral code which has a large amount of legacy code and technical debt. The question isn't about how to do it per se, but how to do it in such a way that doesn't impact users (and developers). That also includes how to make the changes in such a way that they are easy to understand, review, test, and maintain. It's not just about making the changes now, but what happens if an existing bug is found or fixed after they've landed which must be backported - these kinds of changes can make such a backport that bit harder for example.

I think it's also worth considering in changes like these what the possible future changes you may want to make are, and how you can make things easier for the future. As I noted in my response, one example of this would be trying to identify changes in future PHP versions (dynamic property deprecations, implict array creation deprecation, undefined property and variable deprecations, for example).

With regards your concerns about this kind of change being discouraging for those wanting to make sweeping enhancements, unfortunately such changes are never easy, especially in an ecosystem as broad and varied as Moodle's, because there are many cases to consider. They are possible, but the best way to approach them is with open communication with the community and HQ (as Tim is doing here). Some examples of past broader changes in the past include the Activity chooser, Course JS rewrites, Asynchronous backup and restore, Safe Exam Browser integrations, Big Blue Button, Brickfields Accessibility toolkit, and others. If you have a more specific example you wish to discuss, I'm happy to do so.

Andrew
Average of ratings: -
In reply to Tim Hunt

Re: Advice wanted: what is the best way to get a large change integrated

by Davo Smith -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
Could any of this be done in parallel?

It strikes me that steps 1-3 could possibly be done in any order, as they're focused on different parts of the code, so they wouldn't be held up in integration, waiting for each other?

4+5 look dependent on 3, so probably can't be done until that is agreed (and 6 definitely needs to wait for 1-5 to be integrated).

My mind is drawn to the phrase "a journey of a thousand miles ...", and I'd personally encourage you to at least make a start on the list, as these steps would certainly make the code better / more maintainable, even if it takes a while before you can benefit from this and get your new feature?
Average of ratings: -
In reply to Davo Smith

Re: Advice wanted: what is the best way to get a large change integrated

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Wow! thanks everyone for speedy and useful feedback smile Please keep it coming.

Git so so powerful, that if you are prepared to spend arbitrary amounts of time fighting merge conflicts, the you can probably do anything in any order, and change your mind later. But, in this case, I think it is better to not do that, because all these touch the same area of code, so they will all conflict.

And, if you have a massive branch to rebase every time, de-motivates you from going back and improving something in the first commit. And, the benefit of all the Moodle review is the discsusion about what is the best way to do thing, and the resulting improvements in the code before it is integrated. So, I would very much like to limit the amount of different things that are work-in-progress at any one time, if that is possible.

But, I certainly agree with your last point: a journey of a thousand miles begins with a single step. (Also, some of the items above could be broken down further. E.g. 1 could be spilt into issues (or separate commits to address one issue) one per class moved. Similarly 5 could be split into one thing per service.
Average of ratings: -
In reply to Tim Hunt

Re: Advice wanted: what is the best way to get a large change integrated

by Andrew Lyons -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
I agree here with you Tim, though there is still likely some parallelisation possible. For example, it may make more sense to split the class migrations up into smaller chunks as you suggest and to migrate to templates _before_ doing the renderer migrations. Or it may be easier to do those renderer migrations first, and then template migrations at the same time as unrelated class changes.
Average of ratings: -
In reply to Tim Hunt

Re: Advice wanted: what is the best way to get a large change integrated

by Andrew Lyons -
Picture of Core developers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers Picture of Testers
Thinking about this some more,

I'd suggest breaking the classes down into smaller chunks of related areas and consider landing them in an order which assists subsequent steps. For example (as I noted elsewhere), maybe it's worth migrating the renderers and output changes first as they're a small and easy-to-process chunk. Once they're landed you can look at template migrations independently of the other class moves.

I'd also suggest adding in as a 1b step to break out the current 2000 lines of mod/quiz/classes/external.php into separate classes.

I'm happy to try and assist on the YUI -> ESM migration wherever possible.
Average of ratings: -
In reply to Andrew Lyons

Re: Advice wanted: what is the best way to get a large change integrated

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Well, what I have done is to make a commit to just move one class. That is now on MDL-74923, so I can see what CiBoT and github actions make of it. Can anyone take a look, and see if I seem to have covered everything?

I think that, as long as I keep things in separate commits, it is OK to multiple class renames in one tracker issue. And, I am going to try to be very disciplined, and just move the classes in these commits. Other cleanups can be in other issues (although, I already broke my own rule here, I fixed an unrelated PHPdoc comment, and a few other comment typos. ... OK, since the other changes are in comments, I won't revert it now.)

These cleanups are going to touch a lot of lines of code, and add use statements on consecutive lines, so the will largely conflict with each other. Therefore they have to be done in a strict order. Therefore I think it is easiest to do them in one, or a few, tracker issues.

I will see how much I get done in the remainer of this week, and see if I can get a batch submitted to integration for next week, if possible.
Average of ratings: -