David Mudrak's blog

Current stable version 2.0.1+

Total of 64 patches were accepted by the integration and testing teams during this week for Moodle 2.0 stable branch (which still lives on Git ‘master’). This is a new record in terms of number of pull requests. Also, with only 5 pull requests rejected, this is the best accepted/rejected ratio achieved so far. It also means I am no longer able to provide an overview of all accepted changes so I will focus only on those I helped to test or review.

Moodle can now be installed on PostgreSQL server with ‘standard_conforming_strings’ option enabled. This option is on by default since PostgreSQL 9.1 and Bruce Momjian’s blog post explains why (MDL-26351). ♦ It has been known for a while that MSSQL and Oracle databases do not like SELECT DISTINCT if the list of returned columns contains some TEXT ones. This is tricky especially in a case like SELECT DISTINCT t.*, if the table t contains some TEXT fields. As most Moodle developers use PostgreSQL or MySQL as a part of their environment, some affected queries still survive in Moodle code. Eloy Lafuente found and fixed couple of them, using a nice inner join with subselect solution (MDL-26371). ♦ Mark Nielsen spotted a bug in the implementation of print_collapsible_region() function. Glenn Ansley took this as an opportunity for his first contribution to Moodle, created a branch at his moodle.git clone at github.com and prepared a patch there (MDL-26131). ♦ Andrew Davis fixed a really nasty bug in blogging subsystem that was causing accident removal of all recent records in the table ‘post’. This table holds not only blog posts but also user notes too, for example (MDL-26010). A regression of a recent change in the upgrade code of SCORM module was fixed by Eloy Lafuente (note for myself: next time I test SCORM upgrade code, I shall have some SCORM module instance actually created at the site… MDL-26361) ♦ And finally, images embedded into forum posts that are sent by email to the forum subscribers are now correctly displayed in the email clients. Given that the client has access to the forum either via the current user’s session (in case of web email clients) or the course grants access for anonymous hosts (MDL-25944).

Previous stable version 1.9.10+

All 9 submitted patches were accepted and they landed on MOODLE_19_STABLE branch. ♦ Petr Škoda fixed a problem with redirecting to a login page on https protocol and a bug mediaplugin filter. ♦ Eloy Lafuente found some queries using SELECT DISTINCT from TEXT fields in 1.9 version, too. ♦ Dan Marsden fixed a bug in a pop-up form in SCORM module. ♦ Aparup Banerjee fixed the number of glossary entries being displayed in the recent activity block. ♦ Tim Hunt fixed a bug in quiz that used to display overall feedback based on the rounded grade instead of the real value.

Quotes of the week

“We will ALWAYS have blockers and critical issues, they never end. If we tried to solve them all, we’d never release anything.”
– Martin Dougiamas has a pragmatic view on release policy

“Any code you write yourself should follow all the coding guidelines perfectly. However, when looking at old code, you should be tolerant of what you find.”
– Tim Hunt in a discussion on a policy of naming database tables in Moodle

“Never trust user input.”
– Jonny Barnes at an excellent page summarizing what should a developer know before building a public web site

Is it there yet?

If you are watching some issues in the tracker, you probably know those emails informing you about the change of the issue status. If you receive an email that your favourite issue has been resolved or closed, it is good to understand what exactly it means.

As you probably know, the current Moodle development workflow is based on so called fork and pull model. All developers submit their patches into separate branches in their own public forks (clones) of the official Moodle git repository. Then they ask the integration team for pulling the changes from these forks into the official repository. Although most Moodle contributors seem to use github.com as a place to publish their repositories, we do not use pull requests feature offered by Github. Instead, a PULL issue in Moodle tracker must be created for each submitted branch (patch). In the created PULL issue, the contributor describes the submitted patch and provides information for the integration team necessary for including the patch.

The relevant MDL issue being fixed by the submitted patch is marked as resolved immediately after the PULL request is created. The contributor is supposed to link the PULL issue with the MDL issue and resolve the MDL issue. So resolved means there is a patch available for this issue, but the patch is not part of Moodle package yet.

Every week on Monday, the integration team goes through the list of submitted PULLs and reviews the patches. If the patch is accepted, it is merged into integration.git repository and the status of the PULL issue is changed to “ready for testing”. When all submitted patches are reviewed (that is they are either accepted or rejected), the testers come and test all changes. If the test passes, the tester closes the MDL issue (alternatively, the release manager closes all MDL issues with their tests passed at the end of testing). So closed MDL means the patch has been accepted by both integration team and testing team. But the patch is still not part of the official Moodle package.

You must wait until all integrated patches are tested and the patches are pushed from integration.git repository into the official moodle.git repository. At the same time, ZIP and TGZ packages are regenerated and can be downloaded. This typically happens on Wednesday afternoon (European time).

As an example, let us say I work on some bug. On Thursday morning, I commit a fix into my repository at github.com. Then I go to the tracker, create a new PULL issue and resolve the linked MDL issue. On Monday, a member of the integration team reviews my patch and if I am lucky enough, they accept it. On Tuesday, a member of testing team follows the steps I provided in the PULL requests and hopefully confirms that the patch really fixes the reported issue. The linked MDL issue is closed then. Therefore the next weekly build generated on Wednesday will contain my fix.

Current stable version 2.0.1+

Total of 27 submitted patches for Moodle 2.0 were accepted this week. Eight pull requests were rejected. To highlight some of the accepted patches, Andrew Davis fixed a blocker issue reported by Aaron Cowell causing errors in RSS feeds generated by Moodle (MDL-24870). ♦ Dongsheng Cai fixed a bug in Database activity module’s templates editor (MDL-25671). ♦ Eloy Lafuente prepared an emergency fix of a regression blocking the upgrade at Oracle servers.

Previous stable version 1.9.10+

There is a single accepted patch for Moodle 1.9 from the last week. Eloy Lafuente fixed a regression in Database activity module reported by Paul Nijbakker. Some module functions redirected user to the first record in the database instead of to the correct one (MDL-26052). The second submitted patch for Moodle 1.9 improving inefficient computation of tag correlations had to be rejected because the included SQL statement was not cross-platform and worked under MySQL only (MDL-24355).

Quotes of the week

“I think we need to become serious and start rejecting any commit with [trailing] whitespace.”
– Eloy Lafuente strongly recommends all developers and contributors to set up their IDE properly

“Open source sometimes actually works, even without doing the programming yourself.”
– Sam Marshall knows that reporting a bug is sometimes enough to make the thing fixed

Testing Moodle on various databases

Moodle 2.0 officially supports four major database systems: PostgreSQL, MySQL, MSSQL and Oracle. I have PostgreSQL and MySQL installed on my Linux workstation and I use PostgreSQL 8.3 at the moment as the major platform for the development – simply because I believe that if a code works at PostgreSQL, it is generally more cross-platform than a code that was tested on MySQL. Though there are exceptions, of course.
To help with testing patches related to MSSQL, I decided to set up MSSQL server for me. I have successfully installed and set up MSSQL Server 2008 R2 Express edition. The server runs in a virtual machine on my notebook. I use VirtualBox from Oracle (Yes, Oracle made VirtualBox and we’ve always been at war with Eastasia) networked via a bridge between the Linux host and Windows guest. That allows me to have Moodle installed in my Gentoo being connected to the MSSQL database in Windows via freetds driver. To test the new sqlsrv driver, I downloaded Moodle Windows package to the guest and was able to install it there, too.

There were 53 patches submitted for integration this week; 10 of them were rejected, 43 survived both review and testing and got into the official repository. Regular weekly builds were released on Wednesday.
Today on Friday morning, Tomaz Lasic spotted a critical regression at School Demo Moodle site. Due to recent changes in modinfo library, Workshop module’s navigation stopped working completely, throwing coding exception because the new modinfo API was not backward compatible by mistake. This issue was promptly fixed by Sam Hemelryk and emergency weekly build of Moodle 2.0 was re-released.

Current stable version 2.0.1+

Total of 40 accepted pull requests affect Moodle 2.0 branch. ♦ Andrew Davis added new index to the table user_info_data consisting of userid and fieldid fields to increase performance when searching over custom user fields (MDL-17201). ♦ Dan Marsden fixed several SCORM module bugs and cleaned up its code. ♦ Dongsheng Cai fixed a bug in file picker. Internet Explorer was trying to download repository_ajax.php instead of executing it via AJAX request when users had .php files associated with an external editor. Dongsheng also fixed a bug in Wiki module’s upgrade code, dealing with wikis with the same title (some issues with Wiki upgrade seem to be still open – see comments in PULL-196 for details). ♦ Eloy Lafuente submitted a set of patches, mainly dealing with database access – wrong usages of get_text() and get_recordset_xxx() and using words reserved in Oracle as bind placeholders. ♦ John Stabinger fixed various issues in several Moodle 2.0 themes. ♦ Petr Å koda submitted a bulk of patches fixing many areas of the code. ♦ Rossiani Wijaya improved Multimedia plugins filter so that it now supports YouTube playlists (MDL-25573). ♦ Sam Hemelryk improved performance of the new navigation system (MDL-25291). The reporter Petr Å koda noticed that the new administration block is loaded for navigation purposes at almost every page even though most users (students) can not see anything in it. Sam implemented a new session flag to indicate whether there is actually something to load for the administration block. Not trying to load an empty block makes quite a noticeable performance difference in number of included files, RAM consumption and get_string() calls. ♦ Sam Marshall submitted another patch leading to the proposed improvements of get_fast_modinfo() function. ♦ Tim Hunt submitted three patchsets, all dealing with issues in Quiz module and Question bank.

Previous stable version 1.9.10+

Three accepted pull requests affect Moodle 1.9 branch. ♦ Andrew backported MDL-17201 discussed above. ♦ Dan Marsden backported a SCORM issue MDL-25320. ♦ Petr Å koda fixed a bug in an enrolment plugin – sites using IMS Enterprise file for enrolments should upgrade.

Quotes of the week

“Ooh duh – somebody actually documented it. I wasn’t expecting that ”
– Sam Marshall was lucky enough to find up-to-date information at a documentation page for developers

“git blame or blame git – that’s the question”
– Eloy Lafuente comments on Sam Hemelryk’s experience in MDL-25791

Performance matters, women say

Although Moodle developers focus on performance these days, not many performance related patches landed this week yet. And I can understand why. From my own experience, working on performance is pretty time consuming job requiring a lot of comparison tests on production data to get valid results. Luckily, Moodle 2.0 has inbuilt support for xhprof – a profiling tool written by Facebook developers. Large sites admins and Moodle developers can use this tool to compare how various tiny improvements save milliseconds of execution time and to analyse the complex function call graphs, identifying performance bottle necks.
From what we could see so far, the performance of the new navigation system (used at almost every page) and the recently improved filtering system (which makes major part of frequent format_text() function) should be our primary targets now. I started to study the current implementation of text filters and the mechanism of how they are chained, looking for places to improve. While reading the code, some important questions emerge that will need design decision soon.
One of them is a well known “nolink” feature. Moodle users know this as a button in HTML editor that is supposed to “prevent automatic linking” within the selected block of text. This feature was introduced back in 2003 as a part of glossary terms auto-linking feature. Later the feature was used by other parts of Moodle – activity names linking, database records linking etc. As all these automatic linking features are now implemented as standard filters, the concept became less clear. Is the text wrapped by “nolink” marks supposed to be processed by other filters that do not actually create links? Shouldn’t it be replaced with some “do not apply any filter on this block of text” feature?
Beside this conceptual question, there is an issue with the actual implementation of the feature, too. Originally, the part of the text where no automatic linking was supposed to happen had to be wrapped by <nolink> and </nolink> tags. Later as a part of transition to full XHTML support, these tags were replaced with valid <span class=”nolink”>…</span> pair. It works pretty well for short strings – typically glossary terms. But this syntax fails badly when the inner text contains another span tags as it becomes difficult (if not impossible) to identify the correct substring by regular expressions (we can not afford full DOM parsing for obvious performance reasons). Also, using spans makes it impossible to start the protected block in the middle of one paragraph and end it in the middle of the second one because HTML tags must nest correctly.
The solution I am about to suggest is to introduce a new syntax using a couple of empty span tags. The start of the protected block would be defined using something like <span class=”nofilterstart”></span>. The block would end at the place where <span class=”nofilterend”></span> is found. XHTML specification recommends not to use the minimized tag syntax for empty span elements, so even if shorter <span class=”xxx” /> would work (and Moodle will support it probably), our HTML editor would produce the syntax with closing tag. This way, both issues with proper nesting and inner spans would be solved.
You see – I got into a completely different area than the performance of format_text() I started to work on But it is quite related in fact. If the filter manager is able to detect some common parts of the text that no filters are expected to modify, it can save time spent on search & replace job that every filter does. Any feedback, idea or contra-proposal is welcome, of course.