Moodle

Hello Jerome,

need some help on this?

Hello, Jerome

in reality there is 98 globals on moodle. 191 is the number of calls to globals, and comments that metion globals use
I did the grep you used and cleaned up the repeated ones.
Here is the list:

gloabl $ewiki_id;
global $act
global $arguments;
global $argv;
global $aspell_opts;
global $aspell_prog;
global $authstrs;
global $basedir;
global $blogid; //hackish, but if there is a blogid it would be good to preserve it
global $cacheData
global $CFG;
global $choose;
global $cm
global $col
global $context_cache
global $context_cache_id;
global $course
global $day
global $db
global $debug;
global $dir;
global $diskCache;
global $empty_rs_cache;
global $ewiki_action
global $ewiki_author;
global $ewiki_auth_user
global $ewiki_binary_icons
global $ewiki_config;
global $ewiki_editor_content;
global $ewiki_errmsg
global $ewiki_id
global $ewiki_idf_url;
global $ewiki_link_case;
global $ewiki_links
global $ewiki_plugins
global $ewiki_ring
global $ewiki_spages;
global $ewiki_t
global $ewiki_title
global $ewiki_upload_sections;
global $ewiki_use_editor
global $extraws;
global $filename;
global $form;
global $fpdf_charwidths;
global $frm; // can be used to override submitted login form
global $groupid
global $gSQLBlockRows; // used by rs2html to indicate how many rows to display
global $id;
global $idcounter; // To make sure all radio buttons have unique ids. // JR 20 NOV 2007
global $incolumn;
global $input_separator;
global $jsarg
global $lifetime
global $matches; // not sure why I global this...
global $mdwp_hidden_tags
global $mdwp_placeholders;
global $mime_magic_data;
global $module;
global $mon
global $moodle_disable_camel_case;
global $moodle_format; // from wiki/view.php
global $next_url;
global $nomoodlecookie
global $params;
global $php_errormsg;
global $preferredtitle
global $qnum;
global $redirect; // can be used to override redirect after logout
global $restore; // ick
global $result
global $returnurl;
global $row
global $rssid
global $SESSION
global $settings;
global $shared;
global $site;
global $sort
global $standard_javascript;
global $strallowguests;
global $strrequireskey;
global $strs
global $tempfiledir;
global $template_globals;
global $textinputs;
global $upgradelogbuffer;
global $upgradeloghandle
global $url
global $usecheckboxes;
global $USER;
global $user; // can be used to replace authenticate_user_login()
global $userfields;
global $userid
global $wiki
global $wiki_entry,
global $wikipage
global $yr;

Thanks Daniel. Sure any help is welcome, I create this issue to catch any work done on global variable.
Some global variables are obviously not being removed ($CFG, $USER, ..., ), but some other should really disappear (I see a global $params in this list for example ;o).

Hello, Jerome.
Sorry for disappear, but i went to a 10 days-without-computer vacations, hehe.

Searching for the $params global i just see it in ./question/format/hotpot/format.php
It seems like a code to work on older versions of moodle:

49 // get import file name
50 global $params;
51 if (! empty($this->realfilename)) { 52 $filename = $this->realfilename; 53 } else if (isset($params) && !empty($params->choosefile)) { 54 // course file (Moodle >=1.6+) 55 $filename = $params->choosefile; 56 } else { 57 // uploaded file (all Moodles) 58 $filename = basename($_FILES['newfile']['tmp_name']); 59 }

I really didn't know if this is needed. This script is called by ./question/import.php, in this point:

90 // work out if this is an uploaded file
91 // or one from the filesarea.
92 if (!empty($form->choosefile)) {
93 $importfile = "{$CFG->dataroot}/{$COURSE->id}/{$form->choosefile}";
94 $realfilename = $form->choosefile;
95 if (file_exists($importfile)) { 96 $fileisgood = true; 97 } else { 98 print_error('uploadproblem', 'moodle', $form->choosefile); 99 }
100 } else {
101 // must be upload file
102 $realfilename = $import_form->get_importfile_realname();
103 if (!$importfile = $import_form->get_importfile_name()) { 104 print_error('uploadproblem', 'moodle'); 105 }else { 106 $fileisgood = true; 107 }
108 }
109
110 // process if we are happy file is ok
111 if ($fileisgood) {
112
113 if (! is_readable("format/$form->format/format.php")) { 114 print_error('formatnotfound','quiz', $form->format); 115 }
116
117 require_once("format.php"); // Parent class
118 require_once("format/$form->format/format.php");
119

I don't have any hotpotatoes file here to test. What do you think? Is there oficial and active mantainers of this modules?

Hi Daniel, after a quick look I dont get this global $params too. That the unique one in all Moodle (so it doesn't seem to be set anywhere??). It is used in this readquestions() function. I attach a hotpotatoes quizz to this issue if you wanna have a look. There is definitively something weird here but I don't have the time to look more.
Gordon is in charge of the hotpotatoes module (http://docs.moodle.org/en/Credits), I make him a watcher for this issue.
thanks

Jerome,
I understand that you think the use of "global $params;" is weird.

Here is the code in context:

// get import file name
global $params;
if (! empty($this->realfilename)) {
    $filename = $this->realfilename;
} else if (isset($params) && !empty($params->choosefile)) {
    // course file (Moodle >=1.6+)
    $filename = $params->choosefile;
} else {
    // uploaded file (all Moodles)
    $filename = basename($_FILES['newfile']['tmp_name']);
}

Looks to me like the code is using the presence of the $params->choosefile to detect whether the import file is one selected from the course files or not. If you have a suggestion for an alternative way of doing this, please go ahead and implement it.

> I really didn't know if this is needed.

If we leave it like it is, then there is only one script to maintain for all Moodles. This script has worked very well for several years, so unless you feel it is broken, I don't see that there is a "critical" need to change it.

Gordon

I think that keep this kind of compability in a piece of code that is on the moodle three is not a really good idea. If the mantainers of the module choose to keep developing for older version of moodle, this should be done in separate branches.

ps.: it is really not a critical issue

Jerome,
are you able to arrange for the readquestions ($lines) method to be passed the filename as well as the lines from the file? Or perhaps have the filename set as a method of $this? Then we would not need the workaround to get the file name using the $params and $_FILES variables.

Alternatively, can you devise a way of determining the filename without using the $params and $_FILES variables?

Just to note: the $arguments global variable appears just in admin/fixuserpix.php
May be it could be passed via parameter to the form, but i will not look at this now. I will search for another global that is called in more places.

Here is a patch (day.mon.yr.diff) that remover three globals: day, mon, yr. They were used only in calendar/view.php

Here is a patch that removes the "basedir" global (basedir.diff) that was used in files/index.php . It is also used on ./lib/editor/htmlarea/coursefiles.php but i will keep working on some other global in files/index.php

Gordon: thank you for having a look at it. No worries, I wrote this issue as a reminder to remove the "bad" global variables. We should only keep the needed one ($CFG, $DB,...). The priority issue is not specific to this $params global variable.
I had a quick look at this $params global variable. I know it is used but I don't find where it is declared. With the variable name, it is the second thing I though weird about it. Anyway if it's too much a hassle to remove this global variable, I don't feel any harm to keep it like that.

Daniel: can you write two new issues for theses patches, selecting calendar and htmleditor components. They should be automatically assigned to the good person. Thank you

Hello Jerome,

i have created the two new issues and linked them here. The issue about the calendar was assigned to "Nobody" and and the one about the "HTML editor" (don't really know if it should be about htmleditor, i guess files/index.php is the file upload screens) was assigned to "moodle.com". Someone will really looks at them?

no problem Daniel, Eloy is doing some triage every week. He will assigns the bug during his next triage.

Please only real bugfixing in STABLE, no code cleanups should happen there

np, closing it.