Review the uses of course shortname and make sure it is formatted consistently.

This will help resolve MDL-28305, MDL-28636, MDL-28697, and MDL-28698

Hi guys,

Putting this up for peer-review now.
All three branches ensure that course shortname is now consistently formatted throughout Moodle.
As well as that the master branch also increases the size of course.shortname so that it is actually usable for multilang formatting.

Please note: I haven't added or changed formatting of course shortname when it is being used by PAGE->set_heading, PAGE->set_title, and navigation_node->get_content as there 3 functions are being evil and formatting strings internally themselves.
There is an issue to see this fixed later on.

Cheers
Sam

Rebased to weekly

Replace "Browse around the site and make sure the correct course shortname is always shown." with instructions telling the tester to check their user's language in their profile then check the locations before switching languages and rechecking. And explaining that they probably need to install a language pack to give them a 2nd language to select and that the 2nd language in their multi-lang course name must use this other language.

What is the reason for always providign format_string() with a course context instead of allowing it to use the page context?

Spelling error (form/from) in this comment in the upgrade code
// Define index shortname (not unique) to be dropped form course

Actually would you mind changing that comment to something like this?
// MDL-29189 Increase course short name length to accommodate multi-lang strings. Requires dropping and readding an index.

That way the comment tells you why this is being done rather than just telling you what is being done. Including the bug number is handy for the future.

Avoid comments like the below one. You can read the code to figure out the what is being done so comments like this are more or less worthless.

// Launch change of precision for field shortname
$dbman->change_field_precision($table, $field);

Hi Andrew,

Thanks for looking at that patch for me.

In regards to the points that you picked up, I will take a look at the instructions and will see if I can improve them. The user installing a second language and switching languages to check the filtering isn't required, the multilang filter strips all translations except for the user's current language or if there is no translation for that english. The example string I've got there will work perfectly for testing without regardless of the languages the user has installed, or what language they are using. Perhaps if I clarify things to say that they should check that NZ is not displayed anywhere in the page.

Providing a context for format_string and format_text calls is a requirement. Internally those function resort to the PAGE context which aids us in ensuring backwards compatibility of those functions (implemented when we added context sensitive filtering) however the PAGE context is only correct if it happens to be the same context that the string was created within.
When calling format_string and format_text we should always be providing the context that the string was created within, the reason being that the string may be created to take advantage of the filters applicable to the context and without those filters will likely result in garbage as is the case with the multilang filter.
As an example imagine you have the multilang filter turned on within a specific course but no where else and the course shortname contains a multilang string when we print the frontpage course list if we don't provide the courses context format_string will resort to the PAGE->context which is the frontpage context where the filter is not turned on a consequently the multilang string won't be formatted as the user would have expected.

As for the comments I had a wee chuckle about those, the comments you have picked up there are all comments generated automatically by the XMLDB editor (including the spelling mistake).
I'll improve the comments shortly and will create an issue to see the typo in the XMLDB editor fixed up.
The only thing that I won't do is add the bug number, I am very much against adding the bug number to a comment unless it is REALLY needed such as a TODO, the reason being that the bug number is only accurate until the next time the code is edited at which point anything can be going on and that comment will quickly become misleading. Personally I'd suggest always using a history tool such as git gui blame to work out the issues that have affected the code.

Cheers
Sam

Hi Sam,
That's a nice big patch , I've gone through it and here's my notes:

• enrol/authorize/locallib.php ln 57=>66, 70 and elsewhere where $coursecontext is used is actually CONTEXT_USER - seems there's some mix up between contexts there and the variable seems misnamed too.
• /lib/db/upgrade.php , comment '100 characters if practically useless' has typo.
• /mod/assignment/lib.php ln 125, incorrect context variable used.
• /mod/chat/gui_header_js/index.php ln 67 & /mod/chat/gui_sockets/index.php ln 63: whitespace before format_string()
• mod/feedback/analysis_course.php ln 131 looks like incorrect variable used
• /mod/feedback/mapcourse.php ln 119 has incorrect context variable

cheers,
Aparup

Good spotting thanks Apu!
I've made an additional commit to fix those, have cherry-picked it to all branches, and have pushed them up.
All ready to go again

Cheers
SAm

Linking to MDL-29240.

Just a quick rundown on this issue I choose to format course.shortname consistently for this patch and in the mast branch increase the field length from 100 => 255 chars.

Before this patch course shortname is formatted 50% of the time, and 50% not. Choosing to not format it would mean we have to find a solution for the moodle_page set_title + set_heading, and navigation_node::add which automatically format their input.

Cheers
Sam

My +1 for master and 21_STABLE (I would keep 20_STABLE out). It is a monster, I know. But we already had dozes of places supporting course->shortname being formatted. So why not extend it everywhere?

About MDL-29240 it will need to be more carefully decided. I'm divided about who should have the responsibility of formatting. Let's discuss there and @ HQ.

Ciao

PS: Same applies to sister issues 100% (course->fullname, sections...)

This has been integrated, woot!
Eloy, I've kept it out of 20 purely based on your +1, not even understood your reason. :-D

Works Great
Thanks for fixing this Sam.

YTC !

(aka, yay, thanks and ciao ) Closing.