-
Bug
-
Resolution: Fixed
-
Blocker
-
3.7.5, 3.8.2, 3.9
-
MOODLE_37_STABLE, MOODLE_38_STABLE, MOODLE_39_STABLE
-
MOODLE_39_STABLE
-
MDL-68645-master-earlyoutputinit -
Notes on the origin of the issue
This issue was discovered during test of MDL-68563 where the filter_emoticon plugin was changed to be enabled by default.
It was originally reported as "Data generators use incorrect COURSE, PAGE and OUTPUT vars" and it was to implement the the third option in the below choices. The tl;dr of which is:
Update data generators to create new page globals ($PAGE, $OUTPUT, and $COURSE) during data generation to better simulate the user actions when we create content.
However, it turned out that there is nothing wrong with tests setup. As was discovered in comments below, the primary cause of the problems is that some activity modules call format_module_intro() with enabled filtering when they create calendar events. That call triggers the output initialisation because filters may need a theme which in turn need the page to be set up.
The details below are copied/pasted from a comment in MDL-68563 and they may help to understand how we got here.
Explanation
The issue is two-fold...
The filter_emoticon needs the current theme in order to get the set of emoticons that it should use.
When we fetch the theme we initialise the theme and output.
That behaviour is 100% correct. By the time that the theme is set up things cannot change and we can't change the theme once it is set in the $PAGE.
The things which require that the theme has not yet been set include:
- $PAGE->set_course();
- $PAGE->set_category_by_id();
- $PAGE->force_theme();
- $PAGE->set_cm();
In all of these cases it's because setting a specific context can change the theme - i.e. because there is a course, or course category theme (or because a page has explicitly set the theme for that page).
With the change in MDL-68563, filter_emoticon is now enabled by default. It therefore gets called any time that format text is called.
It just so happens that the process of creating a new activity/resource includes a call to format_text() which, of course, now includes filtering through filter_emoticon.
It also just so happens that many of our unit tests include data generation such as the creation of modules.
In most of our unit tests that isn't a problem because the unit tests do not do anything that would cause any of the above methods to be called, and they often do not do anything to touch anything else relating to $OUTPUT or $PAGE. In the cases where we do so deliberately we usually already know about this limitation (because we're doing things like testing moodle_page and we are therefore already mocking or re-creating it.
So with the change in MDL-68563, we have inadvertently moved the init of theme to a much earlier point in most unit tests - but as I say, no real problem there.
However... this does become a problem when we have a unit test which calls a function like require_login, or require_course_login, because these are both responsible for calling some of these functions on $PAGE (mostly set_course and set_cm. In these cases we are typically creating test data (like activities) as part of a test set up, then doing some more preparation, then calling the function under test, which calls require_login(), which tries to set the current course/cm, which ensures that the theme is not set, and then throws an Exception.
It's worth pointing out that the filter_emoticon, the moodle_page functions, the require_login calls, and the data generators are all behaving correctly, but we hit this when they are used together because of the unusual order when they are used together.
Possible solutions
There are a few things that we can do to work around this:
Disable the filter
We could disable the filter for unit tests.
Note: see MDL-68563 for more info.
Modify the filter
We can modify the filter to take the current page state into account and not initialise the global $PAGE if it has not been started.
We probably only want to do this for unit tests, because in normal circumstances this could give us incorrect behaviour. A patch like this would probably be adequate:
Note: see MDL-68563 for more info.
Update data generators to create a new global $PAGE during generation
Essentially this would mean that every time we create things like activities (and possibly blocks) as part of the test data setup, we push the current $OUTPUT and $PAGE variables onto a stack, and generate new ones, and then after the test data is setup, we restore them.
Ideally we would also then set the current course on the $PAGE as part of that setup.
Strictly speaking this is a much more correct approach anyway. There's a chance that this issue has the ability to cause other weird bugs in unit tests, though probably only for those specifically unit testing a theme, or those unit testing features such as course and category theming. Since both of these are atypical we probably aren't currently affected, though those in the community may be.
Pros
- Unit tests should pass
- Is a more 'correct' approach as we will always be using the correct $PAGE variable with the correct course and therefore the correct course theme
Cons
- More complex
- May cause slowdown on unit tests (hopefully minor).