Moving the OU Moodle code to Moodle 2.6.1

I spent today upgrading our Moodle codebase from Moodle 2.5.4 to Moodle 2.6.1. This is the start of work towards our June release of the VLE. We have a March release based on Moodle 2.5.4 to get on the live servers first, and testing that will overlap with the development of the 2.6.1-based version.

Doing the merge

The first stage of the process is to merge in the new code. This is non-trivial because even if you just do

git checkout -b temp v2.5.4
git merge v2.6.1

Then you will get a lot of merge conflicts. That is a product of how the Moodle project manages its stable branches. If your own code changes also lead to other merge conflicts, then sorting out the two is a real mess.

Fortunately, there is a better way, because we know how we want to resolve any conflicts between 2.5.4 and 2.6.1. We want to end up with 2.6.1. Using git merge strategies, you can do that:

git checkout -b merge_helper_branch v2.6.1
git merge --strategy=ours v2.5.4

That gives you a commit that is upstream of both v2.5.4 and v2.6.1, and which contains code that is identical to v2.6.1. You can verify that using git diff v2.6.1 merge_helper_branch. That should produce no output.

Having built that helper branch, you can then proceed to upgrade your version of the code. Our version of Moodle lives on a branch called ouvle which we originally branched off Moodle 2.1.2 in October 2011. Since then, we have made lots of changes, including adding many custom plugins, and merging in many Moodle releases. Continuting from the above we do

git checkout ouvle
git merge --strategy-option=patience merge_helper_branch

That gave a lot of merge conflicts, but they were all to do with our changes. Most of them were due to MDL-38189, which sam marshall developed for Moodle 2.6, and which we had back-ported into our 2.5 code. That back-port made a big mess, but fortunately most of the files affected did not have any other ou-specific changes, so I could just overwrite them with the latest versions from v2.6.1.

git checkout --theirs lang/en backup lib/filestorage admin/settings/development.php lib/form/form.js
git add lang/en backup lib/filestorage admin/settings/development.php lib/form/form.js

Simiarly, we had backported MDL-35053 which lead to more conflicts that were easy to resolve. Another case was the Single activity course format which we had used as an add-on to Moodle 2.5. That is now part of the standard Moodle release. The change caused merge conflits, but again there was a simple solution: take the latest from 2.6.1.

After all that, there were only about 5 files that needed more detailed attention. They were mostly where a change had been made to standard Moodle code right next to a place where we had made a change. (Silly rules about full stops at the ends of comments!) They were easily to fix manually. The one tricky file was in lib/moodlelib.php where about 400 lines of code had been moved lib/classes/useragent.php. There were two ou-specific changes in the middle of that, which I had to re-do in the new version of that code.

Verifying the merge

Having resolved all the conflicts, it was then time to try to convince myself that I had not screwed anything up. The main check was to comparing our ouvle code with the standard 2.6.1 code. Just doing git diff v2.6.1 ouvle does not work well because it shows all contents of all the new files we have added. You need to read the git documentation and work out the incantation

git diff --patience --diff-filter=CDMRTUXB v2.6.1 ouvle

That tells git to just show changes to existing files - the ones that are part of standard Moodle 2.6.1. That is a manageable amount of output to review. We have a strict policy that any change to core Moodle code is marked up like this:

// ou-specific begins #2381 MDL-28567
/*
        $select = new single_select(new moodle_url(CALENDAR_URL.'set.php',
                array('return' => base64_encode($returnurl->out(false)),
                        'var' => 'setcourse', 'sesskey'=>sesskey())),
                'id', $courseoptions, $selected, null);
*/
        $select = new single_select(new moodle_url(CALENDAR_URL.'view.php',
                array('return' => $returnurl, 'view' => 'month')),
                'course', $courseoptions, $selected, null);
// ou-specific ends #2381 MDL-28567

That is, the original Moodle code is still there, but commented out, alongside our modified version, and the whole thing is wrapped in paired begin and end markers that refer to a ticket id in our issues database and if applicable a Moodle tracker issue. In this case I can check that MDL-28567 has still not been resolved, so we still need this ou-specific change. What I am doing looking at the diff output is verifying that every change is marked up like that, and that any issues mentioned are things that are still relevant.

The other check is to search the whole codebase for ou-specific and again review all the issue numbers mentioned. These combined checks find a few ou-specific changes that are no longer needed, which is a good thing.

What happens next

Now that I think the code seems right, it is time to test it, so I upgrade my development install. It mostly works, except that our custom memcache session handler no longer works (the session code seems to have changed a lot, including an official memcached session hander in core). For now I just switch back to default Moodle sessions, and make a note to investigate this later.

Apart from that, the upgrade goes smootly, and, apart from thousands of debugging warnings about use of deprecated code, I have a working Moodle site, so I push the code to our git server, and warn the rest of the team that they can upgrade if they feel brave.

The next thing, which will take place over the next few weeks is to check every single one of our custom plugins to verify that it still works properly in Moodle 2.6. To manage that we use a Google Docs spreadsheet that we can all edit that lists all the add-ons, with who is going to be responsible for checking it, and whether they have done so yet. Here is a small section.

The state of OU Moodle customisation

Our regular code-merges are a good moment to take stock of the extend to which we have customised Moodle. Here are some headline numbers:

• 212 custom plug-ins: Of those 10 are ones we have taken from the community, including Questionnaire, Certificate, Code-checker and STACK (we helped create those last two). Of our own plugins, 58 (over a quarter) are shared with the community, though the counting is odd because ForumNG contains 20 sub-plugins.
• 17 ou-specific issues: That is, reasons we made a change to core code that could not be an add-on.
• Due to those 17 reasons, there are 42 pairs of // ou-specific begins/ends comments in the code.

So, we continue to be disciplined about not changing core code unless we really have to, but the number of plugins is getting a bit crazy. A lot of the plugins, are, however, very small. They just do one thing. Also, we run a range of very different sites, including OpenLearn, OpenLearn works, The Open Science Lab and our exams server. A significant number of our plugisn were just designed to be used on one of those sites.

Here are the numbers of custom plugins broken down by type (and ignoring sub-plugins of our custom plugins).

Plugin type	Number
Activity module	25
Admin tools	8
Authentication methods	2
Blocks	30
Course formats	3
Editors	1
Enrolment methods	1
Filters	6
Gradebook reports	1
Local plugins	44
Message outputs	2
Portfolio outputs	1
Question behaviours	4
Question types	14
Quiz reports	6
Quiz access rules	2
Reports	19
Repositories	3
Themes	9
TinyMCE plugins	1

Automating git

This is a long-overdue follow-up to my previous post about using git to fix Moodle bugs. Thanks to Andrew Nichols of LUNS for nudging me in to writing this.

Git has an efficient command-line interface, but even so, there are some sequences of commands that you find yourself typing repeatedly. Git provides a mechanism called aliases which can be used to reduce this repetitive typing. This post explains how I use it in my Moodle development.

Basic usage

Let us start with the simplest possible example. I get bored typing git cherry-pick in full all the time. The solution is to edit the file .gitconfig in my home directory, and add

[alias]
        cp     = cherry-pick

Then git cp … is equivalent to git cherry-pick …. That saves 9 characters every time I have to copy a bug fix to a stable branch.

Simple aliases like this can also be used to to supply options. Another one I have set up is

        ff     = merge --ff-only

I use that when I need to update one of my local branches to match a remote branch. Suppose I think I are on the master branch, and I want to update that to the latest moodle/master. Normally one would just type git merge moodle/master and it would look like this:

timslaptop:moodle_head tim$ git merge moodle/master
Updating ddd84e9..b658200
Fast-forward

Suppose, however, that I had made a mistake, and I was actually on some other branch. Then git would try to do a merge between master and that branch, which is not what I want. The --ff-only option tells git not to do that. Instead it will stop with an error if it can't do a fast forward. So, to prevent mistakes, I normally use that option, and I do it frequently enough I found it worthwhile to create the alias.

Getting more ambitious

Sometimes the repeated operation you want to automate is a sequence of git commands. For example, when a new weekly build of Moodle comes out, I need to type a sequence of commands like this:

git checkout master
git fetch moodle
git merge --ff-only moodle/master
git push origin master

That updates my local copy of the master branch with the latest from moodle.org and then copies that to my area on github. To automate this sort of thing, you have to start using the power of Unix shell scripting. (If you are on Windows, don't worry, because you typically get the bash shell when you install git.)

Fortunately, you don't need to know much scripting, and you can probably just copy these examples blindly. The first thing to know is that you can put two commands on one line if you separate them using a semi-colon (just like in PHP). The previous sequence of commands could be typed on one line as

git checkout master; git fetch moodle; git merge --ff-only moodle/master; git push origin master

(Note that these lines of code are getting quite long, and will probably line-wrap. It should, however, be a single line of code.)

Doing it this way turns out to be a bad idea. What happens if one of the commands gives an error? Well, the system will just move on to the next command, even though the error from the previous command probably left things in an unknown state. Dangerous! Fortunately there is a better way. If you use && instead of ; then any error will cause everything to stop immediately. If you are familiar with PHP, then just image that every command is a function that returns true or false to say whether it succeeded or not. That is not so far from the truth. So, the right way to join the commands together looks like this:

git checkout master && git fetch moodle && git merge --ff-only moodle/master && git push origin master

Now we know what we want to automate, we need to teach this to git. It is a bit tricky because we don't just want to convert one single git command into another single git command. Instead we want to convert one git command into a sequence of shell commands. Fortunately this is supported, you just need to know the right syntax:

        updatemaster = !sh -c 'git checkout master && git fetch moodle && git merge --ff-only moodle/master && git push origin master' -

Now I just have to type git updatemaster to run that sequence of commands.

Parameterising your aliases

That is all very well for master, but what about all the stable branches? Do I have to create lots of separate aliases like update23, update22, update21, …? Of course not. Git was created by and for computer programmers. Shell scripts can take parameters, and the solution is an alias that looks like

        update = !sh -c 'git checkout MOODLE_$1_STABLE && git fetch moodle && git merge --ff-only moodle/MOODLE_$1_STABLE && git push origin MOODLE_$1_STABLE' -

With that alias, git update 23 will update my MOODLE_23_STABLE branch, git update 22 will update my MOODLE_22_STABLE, and so on.

You can use any number of parameters. If you remember my previous blog post, typically I will create the bug fix on a branch with a name like MDL-12345 that starts from master, and then I will want to copy that to a branch called MDL-12345_23 branching off MOODLE_23_STABLE. With the following alias, I just have to type git cpfix MDL-12345 23 in my Moodle 2.3 stable check-out:

        cpfix = !sh -c 'git fetch -p origin && git checkout -b $1_$2 MOODLE_$2_STABLE && git cherry-pick origin/master..origin/$1 && git push origin $1_$2' -

One final example that belongs in this section:

        killbranch = !sh -c 'git branch -d $1 && git push origin :$1' -

That deletes a branch both in the local repository and also from my area on github. That is useful once one of my bug fixes has been integrated. I then no longer need the MDL-12345 branch and can eliminate it with git killbranch MDL-12345.

To boldly go …

Of course, all this automation comes with some risk. If you are going to screw up, automation lets you screw up more things quicker. I feel obliged to emphasis that at this point. If you are going to shoot yourself in the foot, a machine gun gives the most spectacular results, and we are about to build one, at least metaphorically.

We just saw the killbranch command that can be used to clean up branches that have been integrated. What happens if I submitted lots of branches for integration last week. I have to delete lots of branches. Can that be automated? Using git I can at least get a list of those branches:

timslaptop:moodle_head tim$ git checkout master
Already on 'master'
timslaptop:moodle_head tim$ git branch --merged
  MDL-12345
  MDL-23456
* master

Those are the branches that are included in master, and so are presumably ones that have already been integrated. It is a bit irritating that the master branch itself is included in the list, but I can get rid of it using the standard command grep:

timslaptop:moodle_head tim$ git branch --merged | grep -v master
  MDL-12345
  MDL-23456

I have a list of branches to delete, but how can I actually delete them? I need to execute a command for each of those branch names. Once again, we find that shell scripting was developed by hacker, for hackers. The command xargs does exactly that. xargs executes a given command once for each line of input it receives. Feeding in the list of branches, and getting it to execute the killbranch command looks like this:

git branch --merged | grep -v $1 | xargs -I "{}" git killbranch "{}"

Now to make that into an alias

        killmerged = !sh -c 'git checkout $1 && git branch --merged | grep -v $1 | xargs -I "{}" git killbranch "{}"' -

With that in place, git killmerged master will delete all my branches that have been integrated into master. Note that you can use one alias (killbranch) inside another (killmerged). That makes it easier to build more complex aliases.

Once I have deleted all the things that were integrated, I am left with the branches I have in progress that have not been integrated yet. Those all need to be rebased, and that can be automated too:

        updatefix = !sh -c 'git checkout $1 && git rebase $2 && git checkout $2 && git push origin -f $1' -
        updatefixes = !sh -c 'git checkout $1 && git branch | grep -v $1 | xargs -I "{}" git updatefix "{}" $1' -

With those in place, I just just type git updatefixes master, and that will rebase all my branches, both locally and on github. Use at your own risk!

Thats all folks

To summarise, here is the whole of the alias section of my .gitconfig file:

[alias]
        cp     = cherry-pick
        ff     = merge --ff-only
        cpfix  = !sh -c 'git fetch -p origin && git checkout -b $1_$2 MOODLE_$2_STABLE && git cherry-pick origin/master..origin/$1 && git push origin $1_$2' -
        update = !sh -c 'git checkout MOODLE_$1_STABLE && git fetch moodle && git merge --ff-only moodle/MOODLE_$1_STABLE && git push origin MOODLE_$1_STABLE' -
        killbranch = !sh -c 'git branch -d $1 && git push origin :$1' -
        killmerged = !sh -c 'git checkout $1 && git branch --merged | grep -v $1 | xargs -I "{}" git killbranch "{}"' -
        updatefix = !sh -c 'git checkout $1 && git rebase $2 && git checkout $2 && git push origin -f $1' -
        updatefixes = !sh -c 'git checkout $1 && git branch | grep -v $1 | xargs -I "{}" git updatefix "{}" $1' -

There is limited documentation for this on the git config man page. There is more on the git wiki.

Fixing a bug in Moodle core: the mechanics

Several people at work have asked me about this, so I thought I would write it out as a blog post. In this post, I want to focus on the mechanics of using git and the Moodle tracker to prepare a bug-fix and submit it. Therefore I need a really simple bug. Fortunately one was reported recently: MDL-32039. If you go and look at that, you can see that the mistake is that I failed to write coherent English when working on the code to upgrade from Moodle 2.0 to 2.1.

What we need to do

This bug was introduced in Moodle 2.1, and affects every version since then. Since it is a bug, it needs to be fixed in all supported versions, which means on the 2.1 and 2.2 stable branches, and on the master branch.

My development set-up

I need to fix then test code on three branches. The way I handle this is to have a separate install of Moodle for each stable branch, and one for master.

I use Eclipse as my IDE, so these three copies of Moodle are separate projects in my Eclipse workspace .../workspace/moodle_head, .../workspace/moodle_22, and .../workspace/moodle_21. Each of these folders is a git repository. In each repositor, I have two remotes set up, for example:

timslaptop:moodle_head tim$ git remote -v
moodle git://git.moodle.org/moodle (fetch)
moodle git://git.moodle.org/moodle (push)
origin git@github.com:timhunt/moodle.git (fetch)
origin git@github.com:timhunt/moodle.git (push)

One is called moodle and points to the master copy of the code on moodle.org. The other is called origin and points to my area on github, where I publish my changes.

In order to test the code, I have three different Moodle installs, each pointing at one of these copies of the code. Each install uses an different database prefix in config.php, so they can share one database.

Getting ready to fix the bug on the master branch

The way I normally work is to fix the bug on the master branch first, and then transfer the fix to previous branches.

The first thing I need to do is to make sure my master branch is up to date, which I do using git:

timslaptop:workspace tim$ cd moodle_head
timslaptop:moodle_head tim$ git fetch moodle
remote: Counting objects: 1442, done.
remote: Compressing objects: 100% (213/213), done.
remote: Total 817 (delta 624), reused 790 (delta 602)
Receiving objects: 100% (817/817), 197.21 KiB | 111 KiB/s, done.
Resolving deltas: 100% (624/624), completed with 225 local objects.
From git://git.moodle.org/moodle
   8925a12..09f011a  MOODLE_19_STABLE -> moodle/MOODLE_19_STABLE
   1cd62bf..a280d40  MOODLE_20_STABLE -> moodle/MOODLE_20_STABLE
   a7899ca..c54172b  MOODLE_21_STABLE -> moodle/MOODLE_21_STABLE
   a81e8c4..58db57a  MOODLE_22_STABLE -> moodle/MOODLE_22_STABLE
   c856a1f..a280078  master     -> moodle/master
timslaptop:moodle_head tim$ git checkout master
Switched to branch 'master'
timslaptop:moodle_head tim$ git merge --ff-only moodle/master
Updating c856a1f..a280078
Fast-forward
 ... lots diff --stat output ...
timslaptop:moodle_head tim$ git push origin master
Everything up-to-date

(Why is everything already up-to-date on github? because I was fixing bugs at work today, and so had already updated my github space from there.)

Since there were updates, I now need to go to http://localhost/moodle_head/admin/index.php and let Moodle upgrade itself.

Fixing the bug on the master branch

First, I want to create a new branch for this bug fix, starting from where the master branch currently is. My convention is to use the issue id as the branch name, so:

timslaptop:moodle_head tim$ git checkout -b MDL-32039 master
Switched to a new branch 'MDL-32039'

Other people use other conventions. For example they might call the branch MDL-32039_qeupgradehelper_typos. That is a much better name. It helps you see immediately what that branch is about, but I am too lazy to type long names like that.

To fix this bug it is just a matter of going into admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php and editing the two strings that were wrong.

Except that, if I screwed up those two strings, it is quite likely that I made other mistakes nearby. I therefore spent a bit of time proof-reading all the strings in that language file (it is not very long). That was worthwhile. I found and fixed two extra typos. This sort of thing is always worth doing. When you see one bug report, spend a bit of time thinking about and checking whether other similar things are also broken.

OK, so here is the bug fix:

timslaptop:moodle_head tim$ git diff -U1
diff --git a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php b/admin
index 3010666..7bd7c13 100644
--- a/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
+++ b/admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
 $string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or 
+$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
 $string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
 $string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';
+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
 $string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
 $string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
 $string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 
+$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never

(Note that:

• I would not normally use the -U1 option. That just makes the output smaller, for the benefit of this blog post.
• The diff is chopped off at 80 characters wide, which is the size of my terminal window.)

Now I need to test that the fix actually works. I go to Site administration ▶ Question engine upgrade helper in my web browser, and verify that the strings now look OK.

OK, so I have a good bug-fix and I need to commit it:

timslaptop:moodle_head tim$ git add admin/tool
timslaptop:moodle_head tim$ git status
# On branch MDL-32039
# Changes to be committed:
#   (use "git reset HEAD ..." to unstage)
#
# modified:   admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
#
timslaptop:moodle_head tim$ git commit -m "MDL-32039 qeupgradehelper: fix typos in the lang strings"
[MDL-32039 9e45982] MDL-32039 qeupgradehelper: fix typos in the lang strings
 1 files changed, 4 insertions(+), 4 deletions(-)

Notice that I followed the approved style for Moodle commit comments. First the issue id, then a brief indication of which part of the code is affected, then a colon, then a brief summary of what the fix was. This first line of the commit comment is meant to be less than about 70 characters long, which can be a challenge!

If this had been a more complex fix, I would probably have added some additional paragraphs to the commit comment to explain things (and so I would have typed the comment in my editor, rather than giving it on the command-line with the -m option). In this case, however, the one line commit comment says enough.

Now I need to publish this change to github so others can see it:

timslaptop:moodle_head tim$ git push origin MDL-32039
Counting objects: 15, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 653 bytes, done.
Total 8 (delta 5), reused 0 (delta 0)
To git@github.com:timhunt/moodle.git
 * [new branch]      MDL-32039 -> MDL-32039

Now I can go to a URL like https://github.com/timhunt/moodle/compare/master...MDL-32039, and see the bug-fix through the github web interface.

More to the point, I can go to the tracker issue, click Request peer review, and fill in the details of this git branch, including that compare URL.

If this was a complex fix, I would then want for someone else to review the changes and confirm that they are OK. In this case, however, the fix is simple and I will just carry on without waiting for a review.

Transferring the fix to the 2.2 stable branch

So, now I want to apply the same fix to my moodle_22 code. First I need to update that install. Since this is similar to what we did above to update master, I will not show the output of these commands, just what I typed:

timslaptop:moodle_head tim$ cd ../moodle_22
timslaptop:moodle_22 tim$ git fetch moodle
timslaptop:moodle_22 tim$ git checkout MOODLE_22_STABLE
timslaptop:moodle_22 tim$ git merge --ff-only moodle/MOODLE_22_STABLE
timslaptop:moodle_22 tim$ git push origin MOODLE_22_STABLE

Then I visit http://localhost/moodle_22/admin/index.php to complete the upgrade.

(This may seem a bit laborious, but look out for a future blog post where I intend to talk about how I automate some of this. I only typed out the commands in full this time because I was writing this blog post.)

I want to apply the bug-fix I did on master on top of MOODLE_22_STABLE, and fortunately the command git cherry-pick is designed to do exactly that. (Since we are back in new territory, I will start showing the output of commands again.)

timslaptop:moodle_22 tim$ git fetch -p origin
remote: Counting objects: 1138, done.
remote: Compressing objects: 100% (118/118), done.
remote: Total 586 (delta 451), reused 569 (delta 434)
Receiving objects: 100% (586/586), 189.65 KiB, done.
Resolving deltas: 100% (451/451), completed with 176 local objects.
From github.com:timhunt/moodle
 * [new branch]      MDL-32039  -> origin/MDL-32039
   2117dcb..a280078  master     -> origin/master
timslaptop:moodle_22 tim$ git checkout -b MDL-32039_22 MOODLE_22_STABLE
Switched to a new branch 'MDL-32039_22'
timslaptop:moodle_22 tim$ git cherry-pick origin/MDL-32039
[MDL-32039_22 2c92dc7] MDL-32039 qeupgradehelper: fix typos in the lang strings
 1 files changed, 4 insertions(+), 4 deletions(-)
timslaptop:moodle_22 tim$ git push origin MDL-32039_22
Counting objects: 15, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (6/6), done.
Writing objects: 100% (8/8), 662 bytes, done.
Total 8 (delta 5), reused 3 (delta 1)
To git@github.com:timhunt/moodle.git
 * [new branch]      MDL-32039_22 -> MDL-32039_22

Notice that the convention I use is to append _22 to the branch name to distinguish the branch for Moodle 2.2 stable from the branch for master. Other people use different conventions, but this one is simple and works for me.

Of course, in the middle of that, I checked that the fix actually worked in Moodle 2.2. In this case, there is not much to worry about, but with more complex changes, you really have to check. For example the fix you did on the master branch might have used a new API that is not available in Moodle 2.2. In that case, you would have had to redo the fix to work on the stable branch.

Transferring the fix to the 2.1 stable branch

Now I rinse and repeat for the 2.1 branch. (I will supress the command output again, until the last command, when something interesting happens.)

timslaptop:moodle_22 tim$ cd ../moodle_21
timslaptop:moodle_21 tim$ git fetch moodle
timslaptop:moodle_21 tim$ git checkout MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git merge --ff-only moodle/MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git push origin MOODLE_21_STABLE

timslaptop:moodle_21 tim$ git fetch -p origin
timslaptop:moodle_21 tim$ git checkout -b MDL-32039_21 MOODLE_21_STABLE
timslaptop:moodle_21 tim$ git cherry-pick origin/MDL-32039
error: could not apply 9e45982... MDL-32039 qeupgradehelper: fix typos in the lang strings
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

So, git cherry-pick could not automatically apply the bug fix. To see what is going on, I use git status to get more information:

timslaptop:moodle_21 tim$ git status
# On branch MDL-32039_21
# Unmerged paths:
#   (use "git add/rm ..." as appropriate to mark resolution)
#
# deleted by us: admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php
#
no changes added to commit (use "git add" and/or "git commit -a")

That may or may not make things clear. Fortunately, I know the history behind this. What is going on here is that in Moodle 2.1, this code was in local/qeupgradehelper, and in Moodle 2.2 it moved to admin/tool/qeupgradehelper, and this confuses git. Therefore, I will have to sort things out ourselves.

In this case, we can just move the altered file to the right place

timslaptop:moodle_21 tim$ mv admin/tool/qeupgradehelper/lang/en/tool_qeupgradehelper.php local/qeupgradehelper/lang/en/local_qeupgradehelper.php

Then use git diff to check the changes are just what we expect:

timslaptop:moodle_21 tim$ git diff -U1
diff --git a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php b/local/qeu
index ac883b5..7bd7c13 100644
--- a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
+++ b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
@@ -19,3 +19,3 @@
  *
- * @package    local
+ * @package    tool
  * @subpackage qeupgradehelper
@@ -50,3 +50,3 @@ $string['gotoresetlink'] = 'Go to the list of quizzes that can
 $string['includedintheupgrade'] = 'Included in the upgrade?';
-$string['invalidquizid'] = 'Invaid quiz id. Either the quiz does not exist, or 
+$string['invalidquizid'] = 'Invalid quiz id. Either the quiz does not exist, or
 $string['listpreupgrade'] = 'List quizzes and attempts';
@@ -57,5 +57,5 @@ $string['listtodo_desc'] = 'This will show a report of all the
 $string['listtodointro'] = 'These are all the quizzes with attempt data that st
-$string['listupgraded'] = 'List already upgrade quizzes that can be reset';
+$string['listupgraded'] = 'List already upgraded quizzes that can be reset';
 $string['listupgraded_desc'] = 'This will show a report of all the quizzes on t
-$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
+$string['listupgradedintro'] = 'These are all the quizzes that have attempts th
 $string['noquizattempts'] = 'Your site does not have any quiz attempts at all!'
@@ -82,2 +82,2 @@ $string['upgradedsitedetected'] = 'This appears to be a site t
 $string['upgradedsiterequired'] = 'This script can only work after the site has
-$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 
+$string['veryoldattemtps'] = 'Your site has {$a} quiz attempts that were never 

Actually, you can see that there is one wrong change there (the change to @package, so I need to undo that. The easy way to undo that would be to edit the file in Eclipse, but I want to show off another git trick:

timslaptop:moodle_21 tim$ git checkout -p local
diff --git a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
index ac883b5..7bd7c13 100644
--- a/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
+++ b/local/qeupgradehelper/lang/en/local_qeupgradehelper.php
@@ -17,7 +17,7 @@
 /**
  * Question engine upgrade helper langauge strings.
  *
- * @package    local
+ * @package    tool
  * @subpackage qeupgradehelper
  * @copyright  2010 The Open University
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
Discard this hunk from worktree [y,n,q,a,d,/,j,J,g,e,?]? y
@@ -48,16 +48,16 @@
 ... lots more diff output here ...
Discard this hunk from worktree [y,n,q,a,d,/,K,j,J,g,s,e,?]? q

Now, git diff will confirm that the change is just what we want, so we can test the change, and then finish up. (output suppressed again):

timslaptop:moodle_21 tim$ git add local
timslaptop:moodle_21 tim$ git commit
timslaptop:moodle_21 tim$ git push origin MDL-32039_21

Submitting the fix for integration

Now I have tested versions of the fix on all three of the branches where the bug needed to be fixed. So, I can go back to the Tracker issue and submit it for integration.

When I get to the bug, I see that Jim Tittsler, who reported the bug, has seen my request for peer review, and added a comment:

Fantastic! It really makes a difference when people follow-up on the bugs they report, and supply extra information, or just say thank you. In this case, although I intended to carry on without a peer reveiw, I got one.

Now I press the Submit for integration... button, and fill in the fix version, the details of the other branches, and most important, we write some testing instructions, so that someone else can test the bug next Wednesday as part of the integration process.

Finally, we are done. Now we sit back and wait for next Monday, when the weekly integration cycle starts. Our change will be reviewed, tested, and, all being well, included in the next weekly build of Moodle.

Reflection

Is this an overly laborious process? Well, it is if you try to describe every detail in a blog post! In normal circumstances, however, it really does not take long. In normal circumstances I could probably have done this fix in ten to fifteen minutes.

What usually takes the time is thinking about the problem, and writing and testing the code. This takes much longer than typing the git commands and completing the tracker issue. Writing the testing instructions can be laborious, particularly if it is a complex issue, but that is normally time well spent. It forces you to think carefully about the changes you have made, and what needs to be done to verify that they fix the bug that was reported without breaking anything else. As I said in my previous blog post, I think testing instructions are a really good discipline.

I hope this rather long blog post was interesting, or at least useful to somebody.

Keeping the discipline of not changing Moodle core

We have said in the past that at the OU we made too many changes to core code in our Moodle 1.9 system, and that as we moved to Moodle 2, we would make far fewer. The release of Moodle 2.1 provides a good opportunity to stop and reflect on how we are doing.

Exactly how many core changes we had made in 1.9 seems to depend on who you ask. It was something of the order of one or two thousand depending on how you count. As a result, every time there is a new Moodle 1.9.x release, someone (Derek) has to do a couple of days painstaking merging to upgrade to the new version.

Moodle 2.1 was released on Friday. On Monday afternoon we decided to try upgrading our development branch to it. The merge (literally git merge MOODLE_21_STABLE) only took a few hours, and that was most mostly a matter of thinking before typing git checkout --theirs to resolve most of the conflicts in favour of the Moodle 2.1 code. Then we had to test test install, upgrade, and basic functionality before pushing the merge to our central repository.

But, how many OU-specific changes do we have in core code right now? Well, the answer appears to be eight. Let me explain that number.

To control the core code changes, we use a simple approval procedure. Each change must be proposed by one of the leading developers. They do this by opening a special sort of ticket in our issue tracking system. That serves as a permanent record of the change, and is also a place to log any discussion. The other leading developers then review the proposal. For the change to be approved, at least one other leading developer must endorse it with a +1 vote, and there must not be any -1 votes. Votes are normally accompanied by an explanation of why that developer is voting that way.

After a suitable time for votes, the issue is declared either accepted or rejected. OU-specific changes can be rejected for two reasons.We may decide that it is not acceptable to change core to implement this feature, so we drop the feature; or we think of some devious way to achieve the feature without changing core code.

If a change is approved, then the code is written. Well, in some cases the code will already have been written, because you can have a much more informed debate about whether a certain change is a good idea if you can see exactly what the proposed change is. Once the code is written and approved, it is committed to out git repository and the issue moves into state 'Code committed'. Finally, we may find a way to get rid of the OU-specific change in future. The most common way that happens is if we contribute the change upstream to moodle.org. For example the new Moodle question engine was an ou-specific change as long as we were using it in Moodle 2.0, but now we have upgraded to Moodle 2.1, it is standard code. Therefore, that issue has now changed status to 'No longer required'.

Overall, our we, have 22 ou-specific change issues in our bug tracker. The break-down is:

Rejected: 2
New (under discussion): 4
Approved (but not yet implemented): 1
Code committed: 8
No longer required: 7

Most of the 'Code committed' changes are pretty boring. For example three of them are bug-fixes to the questionnaire module that we have submitted upstream, but which have not been reviewed and accepted by the questionnaire maintainers yet. Therefore, those three will almost certainly end up as 'No longer required' in due course. Another example is that we want to customise the "Database connection failed / It is possible that the database is overloaded or otherwise not running properly" page that you get when Moodle fails to connect to the database. If Moodle can't connect to the database, then it cannot load the configuration, and so cannot determine which theme to use to display the error. Therefore, the only way customise that page is to edit lib/setup.php.

The one 'serious' ou-specific change we have is some hacking around in course/lib.php to support one of our custom modules called 'subpage' (not released yet, but we hope to share it eventually). Given more time, we might be able to find a more elegant way to handle these changes, but we don't have that sort of time at the moment.

While we have controlled the core code changes, we do have written a lot of custom plugins. Those range from big things like forumng and ouwiki, to small things like a local plugin that just implements a single web-service function. I'm afraid I don't have a complete list, but we must have more than 50 plugins by now. As far as I am aware, the upgrade from 2.0 to 2.1 did not break any of them.

Avoiding duplication and increasing flexibility in software: what about version control?

These thoughts were catalysed by Section 4.2.4 "Once and only once" of PHP in Action where they preach the evils of duplicated code. This is something I strongly believe in. (My best exorcism to date is this Moodle commit.)

Unfortunately, the example they pick to illustrate their point utterly fails. Their example of what not to do is making a custom version of an application in a hurry by copying the whole thing and editing a few lines. Well, duh! Put the code in git and make a branch for the custom version. And, of course, when you do have time, come back and refactor, at which point git diff/merge/rebase ... will probably be a big help.

Now, this is a form of software engineering that open source developers do all the time (while having wild flame wars about which version control system is best). But, when computer scientists dream about paradigms like object orientation, design patters, refactoring, and so on, as ways to reduce the need for code duplication and increase the flexibility of software designs, do they consider the role of version control systems? I don't recall reading anything.


The point about flexibility is particularly interesting in the context of opens source software written in in an interpreted language. Given my background, I am of course thinking about Moodle, which is a PHP web application. Suppose, by way of example, you want to change some part of the processing that occurs when a student submits a quiz, and Moodle works out what score they deserve. The 'proper' design patterns way to do this is:
1. Check: Is the processing algorithm factored into a separate class, as recommended by the Strategy pattern? If not, refactor.
2. Subclass the default Strategy to implement your customisations.
3. Configure things so the factory methods instantiate your Strategy class, rather than the default one.

Alternatively, you could just hack the code. Preferably as a custom branch in your version control system. When a new Moodle version is released, just upgrade and merge (or pull and rebase). One could even try to claim that this way is better, because if the code you hacked changes between releases, the merge may fail, helping you realise that you need to rework your customisation. If you don't notice the problem until you get to testing (which may happen anyway) it will be much harder to find and diagnose.


Another example is this post in the Moodle quiz forum. Someone wanted to tweak something in Moodle, they were told to change the definition of a constant in a library file. Note that because there is no duplication of magic numbers throughout the code, it was only necessary to change the number in one place, so there is some proper design going on here.

One could, of course, have a separate configuration file for all the settings like this in Moodle, but does that really make anyone's life easier? If you think of all the things people might want to tweak, that would be one big file. Also, it would move the constant definition further from where it is used, reducing the cohesion in the code. You could also handle this by making this number an admin setting, stored in the database, and editable through the web interface. In this case, I would argue that that is bad UI design. 3 is a perfectly good default for almost everyone, and Moodle already has more than enough configuration settings. Sufficiently few people need to adjust this, that telling those that do to edit the code is an adequate interface.

You could call this approach "The whole code is a configuration file". In the abstract, you would not say it was good design, but for obscure configuration options like this, it may be the best way.


So, in a world with version control and interpreted languages, what is flexible and easy-to-modify software?


(To head off the obvious comments, I had better re-enforce that I do like nice clean software design, and design patterns, and so on; but I also spend most of my life working on Moodle where parts of the code are not like that; and somehow, most of the time, it just does not seem to matter. Millions of people around the world happily use and customise Moodle despite the lack of design patterns in the code. Should I be sad, or happy?)