Moodle

File areas need to include:

1. Images, etc, embedded in question text (including answers and feedback).
2. Images, etc, embedded in the student's response to essay questions, or other similar types.
3. Images, etc, embedded in the teacher's comment on a student's question attempt.
4. Images in the quiz intro and overall feedback.

2. and 3. may be best combined. We need to consider each attempt builds on last here. Another decision:
A. should the file area belong to the question_session, or
B. should the quiz (or whatever activity) be responsible for creating a file_area for the quiz_attempt, and passing it in to the questionlib.php functions that need it. I feel that the second possibility would make it easier to do the right access checks, but then we would need a solution for question preview.

Each attempt builds on last - we could always create the file area for the second attempt by copying the one for the first attempt.

See also MDL-15573 (temp files built during question export).

When doing 1. We should also get rid of the separate image dropdown on the question editing forms, and the question.image field. Any questions that currently have an image should have the image added to the end of the question text.

Note to self. See Dongsheng's document at http://docs.moodle.org/en/Development:Convert_Draftarea_Files#editor_element.

Hi Tim!

How is this one looking? You sound pretty flat out but do you have any time to work on this for Moodle 2.0?

If not, are you even able to provide an updated todo list/spec and perhaps some naming conventions to follow (keeping in mind a migration to your whole new quiz questions stuff later)?

OK, there is a fairly detailed todo list at http://docs.moodle.org/en/Development:File_storage_conversion_Quiz_and_Questions. Please feel free to discus it with me.

Petr, we'd really appreciate you looking at this and coming up with a detailed spec.

add some comments on calculated, multinaswer and numerical on http://docs.moodle.org/en/Development:File_storage_conversion_Quiz_and_Questions

I have just updated http://docs.moodle.org/en/Development:File_storage_conversion_Quiz_and_Questions after discussing this with Petr. It would be really great to get a design review from Petr some time, now I have finished.

Then, Martin, you need to decide who implements this.

Petr could you please have a quick look at this to make sure it makes sense? Then Dongsheng can implement it.

Hi, Petr,

Can you please review the doc when you get time?

please do not commit anything file related yet, I am preparing extensive changes in the whole File subsystems

Petr, can you please review this ASAP after getting the new file stuff into CVS? Time is very short.

I think the file serving url is not correct, there should be t?o separate urls:
1/ when editing the questions you could use the
$CFG->wwwroot/pluginfile.php/$questioncontextid/qtype_match/stem/$questionid/$itemid/path/to/file.ext
2/ when student takes the quiz
$CFG->wwwroot/pluginfile.php/$quizcontextid/qtype_match/stem/$questionid/$attemptid//$itemid/path/to/file.ext

The new pluginfile is able to find out the location of plugin and call qtype_match_pluginfile(), there does not need to be any question file related code in mod/quiz at all any more.

Is this enough for you Dongsheng, can you ask questions if not? We need to get this sorted ASAP for backup/restore too.

Petr,
how to get this url work?
$CFG->wwwroot/pluginfile.php/$quizcontextid/qtype_match/stem/$questionid/$attemptid//$itemid/path/to/file.ext

file_rewrite_pluginfile_urls always build url by $file/$contextid/$component/$filearea/$itemid/$pathname_in_text

How we add extra questionid and attempted in url?

good question, let me think a bit

2/ $CFG->wwwroot/pluginfile.php/$quizcontextid/qtype_xxxx/yyyy/$questionid/$attemptid/$itemid/path/to/file.ext

I suppose the easies way would be to add quiz_rewrite_question_urls() or something like that, the logic is quite simple and you need it only when printing the questions in quizes - you would search for @@PLUGINFILE@@ and replace it with the $CFG->wwwroot/pluginfile.php/$quizcontextid/qtype_match/stem/$questionid/$attemptid/$itemid/

1/ the question editing is a bit more tricky, ideally there should be just the one itemid and no questionid and internally you would calculate the question id from question type, area name and that itemid - this means standard functions would work just fine and storage would be trivial

$CFG->wwwroot/pluginfile.php/$questioncontextid/qtype_xxxx/yyyy/$itemid/path/to/file.ext

Does it make sense?

yep, I will try it in this way.

Hi all, here is the patch for quiz/question file conversion, which includes:

• replace htmleditor elements with editor elements
• Added accordingly format field to each text field, including quiz, question, and sub types
• Implemented fileareas to qtype_*, question and quiz
• Upgrade {question}.image from 1.9, get rid of this field, and removed the code stilling try to find image by $question->image
• quiz feedback using editor elements, which require current context, but when quiz_add_instance being called, context hasn't been created, so I have to add a callback function to course/modedit.php, not sure if this is the right thing to do.
• added move_files method to qtypes, it will be used when moveing questions to another category, category changed so contextid changed

Things not sure and todo
seems the modulename in attmpts table is always quiz, and I added question_check_file_access, it will be used when preiview page being accessed, Tim, can you please review see if I miss anything.
Not sure if we need qtype_xxx::check_file_access, because the all the access check done in question/type/*/lib.php

Please remind me if I miss anything.

File areas overview

Quiz Module

Added feedbacktextformat field to quiz_feedback table.
Implemented feedback area, added quiz_pluginfile() to serve quiz files

Question Banks

General areas:
quesiontext and generalfeedback areas are served by question_plugin() in lib/questionlib.php

questiontypes areas

calculated

component: qtype_calculated
filearea: feedback, instruction
served by: question/type/calculated/lib.php

calculatedmulti

component: qtype_calculatedmulti
filearea: feedback, correctfeedback, partiallycorrectfeedback, incorrectfeedback
served by: question/type/calculatedmulti/lib.php

calculatedsimple

component: qtype_calculatedsimple
filearea: feedback, instruction
served by: question/type/calculatedsimple/lib.php

description

no files

essay

component: qtype_essay
filearea: feedback
served by: question/type/essay/lib.php

match

component: qtype_match
filearea: subquestion, feedback
served by: question/type/match/lib.php

missingtype

no files

multianswer

no fileareas here

multichoice

component: qtype_multichoice
filearea: feedback, correctfeedback, partiallycorrectfeedback, incorrectfeedback
served by: question/type/multichoice/lib.php

numerical

component: qtype_numerical
filearea: feedback, instruction
served by: question/type/numerical/lib.php

random

no files

randomsamatch

no files

shortanswer

component: qtype_shortanswer
filearea: feedback
served by: question/type/shortanswer/lib.php

truefalse

component: qtype_truefalse
filearea: truefeedback, falsefeedback
served by: question/type/truefalse/lib.php

1/ I do not think the new modedit callback is necessary, I was facing the same problem in mod/folder/lib.php function folder_add_instance($data, $mform), I did some changes there and it should be possible now to get the context there.

2/ do not require_once($CFG->libdir . '/questionlib.php'); in main upgrade.php - always use only sql modifications and basic accesslib and file api

3/ if (substr(strtolower( is not compatible with unsupported unicode names

4/ I could not find proper require_login() in the file serving - both branches

5/ file_save_draft_area_files() is used without the $options parameter - this is a potential security problem

6/ recently I was fixing problem in lesson - the upgrade adds FORMAT_MOODLE to existing data, but the data was previously modified by the HTML editor - is it the same problem here?

7/ you can NOT use trusttext editor option when the trusttext field is not in database! Either we would need to add new db fields or use noclean+XSS risk; if we use trusttext we MUST force download for security reasons. If you use trusttext then you MUST use format_text() accordingly - noclean is not acceptable there in this case!

8/ the essay is still using print_textarea(), it could be converted to new editor without file support too

1/ fixed
2/ removed
3/ I use textlib instead, truefalse question still use php native substr/strtolower, because the answer always true/false in English
4/ question_pluginfile and quiz_pluginfile already use it, added require_login to qtypes
5/ I added fileoptions static variable to default_questiontype, and all questiontypes start to use it
6/ does this mean I should change format to FORMAT_HTML to some fields, such as feedback text?
7/ removed trusttext
8/ this one is tricky, because it is not a moodle form, question engine will print the question content in its own form. Petr what is your suggestion to this?

Another patch here.

Fixed the problems except 6 and 8 which need suggestion

6/ the new default should be editors_get_preferred_format(); the upgrade is using something like
if ($CFG->texteditors !== 'textarea') { $intro = text_to_html($candidate->intro, false, false, true); $introformat = FORMAT_HTML; } else { $intro = $candidate->intro; $introformat = FORMAT_MOODLE; }

8/ if that is difficult we can solve it separately. Ideally we should migrate to new editor element, but the priority is to get the rest into cvs first.

A/ question_states.answer is not user input that is processed by format_text. The column question_states.answerformat should not exist.

B/ I think generalfeedbackformat should default to equal questiontextformat, not 2. But actually, Petr's last answer about 6/ may supersede this.

C/ A function called quiz_rewrite_question_urls should not be in questionlib.php. What exactly is this function doing (that is different from file_rewrite_pluginfile_urls)? (In other words, please improve the PHP doc comment.

D/ In question_pluginfile, I think you should use get_component_directory, rather than hard-coding quiz. This comment also applies to all the qtype_XXX_pluginfile functions. Why do we have all the duplicated code?

E/ Why does quiz_check_file_access use the course context, not the quiz context? Actually, the logic in this function is wrong. When it is called for file areas belonging to questions, it needs to do what I say on http://docs.moodle.org/en/Development:File_storage_conversion_Quiz_and_Questions#Core_question_bank - Compute $state and $options, and call a question_type method.

F/ In quiz_feedback_for_grade, you have added at least two database queries to each call. Is that a performance problem? It is pretty bad on index.php. Please can you change the function so that it takes $quiz, $cm and $context as arguments. (I think they are all available everywhere this function is called.)

G/ I don't understand the addition in question/editlib.php. When is that needed.

(I see that partiallycorrectfeedbackformat only just fits in the 30-character limit on column names!)

(In a patch this big, it does not help to have unrelated whitespace fixes, although in general I think it is good to clean up whitespace.)

H/ In answer to the TODO in question/type/calculated/edit_calculated_form.php, yes, correct/partiallycorrect/incorrect feedback should use the editor.

I/ In question/type/calculated/edit_calculated_form.php, why have you removed the call to parent::set_data? That looks like a bad idea.

J/ In question/type/calculated/lib.php, typo in a comment: quesiton

K/ Also, a similar typo in a variable name: $instruciton

(I have got to about line 1350 of the patch, and I need to go home now. More later.)

Thanks Tim.

Starts to look these issues.

Adding a calculated question i got this:

1) Invalid get_string() identifier: 'atleastonewildcard' or component 'qtype_datasetdependent' line 240 of \question\type\calculated\edit_calculated_form.php: call to get_string()

2) Image to display: No images have been uploaded to your course yet — Maybe i want to upload an image using filepicker, or take images from my privates files.
http://www.screencast.com/users/Dariem/folders/Jing/media/5a5b4d99-da6a-44b3-9465-c930c160057c

A/ removed question_states.answerformat
B/ will be Included in the new patch
C/ added phpdoc, which file do you suggest me to place this function?
D/ the modulename field is quiz, not mod_quiz, have to add mod_ prefix to use get_component_directory, is modulename always quiz? if it is, can we just hard-coded quiz path there?

Moodle will find the qtype_xxx_pluginfile by pluginfile.php, it is using get_component_directory

E/ It was using course context all the time, I passed module context, if it is empty, it will respect the old behavior. Anyway, I changed it to module context. I am checking the logic
F/ fixed
G/ it should be implode(',', $questionids); I cannot remember where, but I failed to move the question without it. I should keep notes
H/ ok
I/ I didn't stop calling parents's set_data, the parent set_data will call data_preprocessing, the parent method is always being used, and it processes the return value from data_preprocessing
J/ fixed typo
K/ fixed typo

Dariem

2) Image to display: No images have been uploaded to your course yet — Maybe i want to upload an image using filepicker, or take images from my privates files.

seems you failed to apply the patch, the image element has been removed from question/type/edit_question_form.php

i've been doing some testing with this patch, it seems that the files api might have reintroduced a possibility to cheat.

for one, it seems that for the 'matching' quiz type, when i have images(files) in every answer, the file urls can enable me somewhat to at least ascertain the sequence that the answers where created in.

although the choices select drop down on the right that i'm supposed to match the images with are randomized, if they literally express some sort of order, i could use the file urls sequence to cheat.

for example:
Question : "Match the pricing of items displayed with the likely cost of it in the market."
say the answer choices are displayed in random order ($10, $1, $100 ) and the images displayed in the left matching column have urls:
http://aparup.moodle.local/m20/dev2/moodle/pluginfile.php/16/qtype_match/subquestion/0/13/12/IMG_198456.JPG
http://aparup.moodle.local/m20/dev2/moodle/pluginfile.php/16/qtype_match/subquestion/0/13/13/IMG_198486576.JPG
http://aparup.moodle.local/m20/dev2/moodle/pluginfile.php/16/qtype_match/subquestion/0/13/11/IMG_1983465.JPG

using the id right before the file name, i can tell that
11 -> $1
12 -> $10
13 -> $100

as long as i assumed that the answers were input in a numerical sequence.

i think we need to randomize the answers sequence when storing them.

( or store the files with an id that isn't directly related to the choices(answers).
or let quiz creators know that putting answers in sequence can be a big hint..)

hope this helps!

hm or we could display quiz files as system's draft files so that theres no clues? - just brain dumping.

Fixed the itemid problems in truefalse/multichoice types when preparing draft files, thanks Aparup for the bug.

I will attach another patch until get more comments from Tim.

Tim, in your doc:
question_answers.answer: "answers are HTML for multichoice and truefalse, but not for numerical or shortanswer"

but in ui, the multichoice type uses text field and truefalse type seems always has True/False value in db, it is more like FORMAT_MOODLE, am I wrong?

Hi, Tim

I re-describe my questions here (my above comments becoming messy):

• in this patch thing_check_file_access will return $question, $state and $options, Tim, can you please explain what options will be? And the $question and $state are passed to $QTYPES[type]>check_file_access, but I am not sure what need to be done with question and state, cannot find clue from print_question_formulation_and_controls, it uses $state>responses[''], but I don't know why check_file_access needs question and state variables

• in question_pluginfile, I think you should use get_component_directory, rather than hard-coding quiz.

  but the modulename value doesn't come with 'mod_' prefix, which cannot be used by get_component_directory, so is it ok that I fix question_new_attempt_uniqueid function to use mod_quiz? (and fix the existing db records in upgrade script)

• question_answers.answer: "answers are HTML for multichoice and truefalse, but not for numerical or shortanswer"

  in ui, the multichoice type uses text field and truefalse type seems always has True/False value in db, it is more like FORMAT_MOODLE, am I wrong?

• Where should I place quiz_rewrite_question_url? I introduce this function because file_rewrite_pluginfile_urls cannot include questionid and attemptid in url, please see Petr's previous comment

I made a branch in git http://github.com/timhunt/moodle/commits/quizfiles so that I could see what had changed between the two patches. I hope this is helpful. Have you been converted to git yet?

question_answers.answer. The the multichoice editing form uses text boxes. That is because with the old HTML editor, having lots of editors on one page was a good way to crash web browsers. However, the content typed into those text boxes was treated as HTML.

Ah, sorry, I am wrong. I remembered that HTML works there, but actually it is FORMAT_MOODLE.

One point of view is that those fields should be converted to be editor elements. However, that will make the form even more ridiculously long, and therefore harder to use. This is something that can probably only be solved properly when we rewrite formslib.

Have you tried echo get_component_directory('quiz'); ? It works. Note the call to the normalize_component function in the middle of get_component_directory.

Given the way the file_rewrite_pluginfile_urls it would just work to call file_rewrite_pluginfile_urls with $itemid = "$questionid/$attemptid/$itemid". However, that is probably a bad idea.

However, it is an even worse idea to have a function called quiz_rewrite_question_urls that is almost a completely copy-and-paste of file_rewrite_pluginfile_urls.

I am doing further comments rather directly, by making additional commits to github at http://github.com/timhunt/moodle/commits/quizfiles

for example http://github.com/timhunt/moodle/commit/7dab046d77b997dec0c1a9dcb1f04df72edc01dc

Truefalse question type does not have fileareas truefeedback and falsefeedback as you state in a comment above. This feedback is in the question_answers table, so the file area should be answerfeedback, or something.

The bit about the right way to check file permissions is hard, so I have just tried to implement it. I think writing code is the clearest way to explain exactly what I mean. See http://github.com/timhunt/moodle/commit/8ce18accab8223698af77d8947ecb5848cfede02

Petr, please can you review that. I am still not very confident I understand the files API.

Note that it adds a special case for qtype file areas, like for blocks and modules. The advantage of this is that there is no longer any need for a lib.php file in each qtype.

I have only change the truefalse question type to demonstrate what I mean. Dongsheng will have to do all the other qtypes. (Please)

Note that I have not tested this code yet. Sorry. However, I think it makes it clear what I mean.

I hope the changes in question/type/truefalse/questiontype.php make if clear what I mean by copying the test for whether something should be visible from the print_question_formulation_and_controls method into the check_file_access method.

You may think it is a bit silly to create a whole new file (question/previewlib.php) for one function. However, I expect that in future, more things will be added to that file to clean up preview.php (http://github.com/timhunt/Moodle-Question-Engine-2/blob/new_qe)/question/previewlib.php

please no changes in pluginfile.php - I think it is better to add the lib.php to all qtypes instead

OK, so we need a lib.php file in each question type, like http://github.com/timhunt/moodle/commit/b35f5cea12fc328bb57a3b9c4c0accc57d047c7f

Hi, Tim

Just started my branch on github: http://github.com/dongsheng/moodle/tree/qfiles
I use git merge quizfiles to merge your changes, I committed to my branch, then you could review and merge my changes, is this the workflow of git?

Dongsheng, you will see I have made a lot of minor comments on the commits in http://github.com/dongsheng/moodle/commits/qfiles

Overall, this is really great work, and it looks like you are getting close to being finished.

It would be really good to get this into CVS soon. We can always do further polishing there. So, +1 from me to commit, when you are happy with the state the code has reached.

Of course, if you would like me to review anything else, please ask.

Hi guys - while testing MDLQA-245 I received a debugging error which I cannot see here so in case you need it:
Moodle 2.0 Preview 4+ (Build: 20100805)

I export questions via the Questions page - when I attempt to download from the 'Click to download the exported category file' (moodlesite/file.php/8/backupdata/quiz/quiz-features-default%20for%20features-20100806-1332.xml?forcedownload=1) from the export screen the debugging shows:

Sorry, the requested file could not be found
More information about this error
Stack trace:

• line 324 of /lib/setuplib.php: moodle_exception thrown
• line 1454 of /lib/filelib.php: call to print_error()
• line 97 of /file.php: call to send_file_not_found()

Apologies if you already know this
Teresa

Hi Teresa, I didn't know this problem, looks like the file.php script forbid to download files from legacy area.

Tim:
I think the question fromat plugins need fix too, I am not sure where should I store the exported file for users to download? Probably user's draft area? Or, is it possible to hook with backup/restore system?
About commit:
I forked moodle in github from your branch, but looks like your branch is not update-to-date, so I cannot update by git-pull too (am I wrong? new to git), not sure how to move the git code to cvs, here is what I am planing to do, please let me know if I am wrong:
1. on cvshead branch, do git pull to get latest head
2. switch to qfiles branch, git merge cvshead (qfiles branch needs update to latest head)
3. switch to cvshead branch, git merge qfiles (merge question/quiz changes)
4. use git diff to generate a patch against cvshead
5. apply the patch to new cvs checkout

I was wrong, it is much esier than what I thought:

• on my branch:
  git remote add upstream git://github.com/moodle/moodle.git
• git fetch upstream
• git pull upstream cvshead
• fix conflicts
• git commit -a
• git diff upstream/cvshead

Yes, that git usage looks correct.

You are right that question export needs to be fixed. There is a separate bug about that: MDL-15573

If you have any time to work on that, I would be really grateful, because I have plenty of other quiz bugs to work on, and you probably understand files better than me.

As you say, the question is, what is the 'right' place to store these export files. Somewhere in the user context probably.

Is it possible to make a separate 'exported questions' file area inside the user context, and have that available for browsing through the file-picker?

The export script is question/export.php. You will see that at the moment, the logic is a bit weird, because in 1.9 we normally store the exports in the backup file folder, but not all users can access that. So, there is a second branch of code to cover that case. Obviously, once the exports go into a file area, we don't need an if like that.

How finished is this? Is it all in CVS?

Have you seen MDLQA-245 ?

Surely this can be marked fixed now.

OK, marking this resolved as it all seems to be in CVS.

Thanks Dongsheng and Tim - good work guys! Turned out bigger than expected (but then when doesn't it).

reopening,
1/ question/contextmove.php and question/contextmoveq.php are still trying to access all course files in dataroot.
2/ function importimagefile() from question/format.php is not converted yet

Going to add there coding errors for now so that it is not forgotten

Not sure if must be considered part of this but, after creating one quiz and a bunch of questions, adding files to question->questiontext, question-> generalfeedback and question_answers->feedback... I'm seeing various records in the mdl_files table (that seems ok, haven't analyzed them and their component/fileareas)... but the contents of the fields commented above are pointing to draftfile files and not to their final destination @PLUGINFILE@ or whatever.

So, apparently the images are displayed ok (because draftfiles continue existing) but they aren't pointing to the correct URL.

Needs review for sure, ciao

Hi, Eloy

I just tested answerfeedback generatefeedback questiontext for following question types, they all point to the correct @PLUGINFILE@

• Calculated
• Calculated Simple
• Calculated multichoice question
• essay
• matching (found a bug in check_file_access, fixed)
• Multiple choice
• short answer
• true/false

Eloy, can you please check where did you see the draftfile url in text?

I did found a lot of draft files in files in mdl_files table, apparently file_save_draft_files only copied files but didn't remove draft files, not sure if it is designed in this way.

Aha, found it!

It is the "save as new question" the one missing the @PLUGINFILE@ conversion !

To reproduce:

• create quiz
• add true/false question with images in question text, general feedback and true and false feedback (save it)
• edit the question and without changing anything, click on "save a new question"

Expected result:

• One new question is created with all the images using @PLUGINFILE@

Current (wrong) result:

• One error about URL not found happens (I commented that yesterday at MDL-16345).
• One new question is created but all the images are using "draftfiles" URLs (the conversion to @PLUGINFILE@ doesn't happen)

I guess the same will happen with other qtypes, I only tested the true/false one.

Ciao

I create a new bug for the cloze question and will take charge of it MDL-24168

Although it is done, changing the form->elements names so they became different form the question->elements name in the database is not necessarely a good idea.

• if (empty($form->image)) {
• $question->image = '';
  + if (empty($form->questiontext['text'])) { + $question->questiontext = ''; } else { - $question->image = $form->image; + $question->questiontext = trim($form->questiontext['text']);; }
  + $question->questiontextformat = !empty($form->questiontext['format'])?$form->questiontext['format']:0;

• if (empty($form->generalfeedback)) {
  + if (empty($form->generalfeedback['text'])) { $question->generalfeedback = ''; } else { - $question->generalfeedback = trim($form->generalfeedback); + $question->generalfeedback = trim($form->generalfeedback['text']); }
  + $question->generalfeedbackformat = !empty($form->generalfeedback['format'])?$form->generalfeedback['format']:0;

even if on edit_form we should use the default edit form for standard question there are othe case when we need to save questions that are not coming form an edit form.
The multianswer subquestions are one case and probably not the only one.

function save_question($question, $form, $course) { which was used as
$wrapped = $QTYPES[$wrapped->qtype]->save_question($wrapped,
$wrapped, $question->course);
I need to build build a different structure for $wrapped1 .

furthermore the code does is not clearer or shorter

$form->generalfeedback['format'] could be replaced by $form->generalfeedbackformat without any information lost.

Is it to late to do the modifcations and get back to

$question->questiontext =$form->questiontext ;

We should also consider the support for the already existing plug-in question types.
Evolution to 2,0 should not beseen as revolution 2,0...

An additional argument is code coherence.

in type/questiontype.php
line 312
function save_question($question, $form, $course) {
global $USER, $DB, $OUTPUT;
line 327
if (empty($form->questiontext['text'])) { $question->questiontext = ''; } else { $question->questiontext = trim($form->questiontext['text']);; }
$question->questiontextformat = !empty($form->questiontext['format'])?$form->questiontext['format']:0;
then in the same function line 387

$form->questiontext = $question->questiontext;
$form->questiontextformat = $question->questiontextformat;
// current context
$form->context = $context;

$result = $this->save_question_options($form);

Oups!!!

        $questiontext = file_prepare_draft_area($draftid, $this->context->id, 'question', 'questiontext', empty($question->id)?null:(int)$question->id, null, $questiontext);

        $question->questiontext = array();
        $question->questiontext['text'] = $questiontext;
        $question->questiontext['format'] = empty($question->questiontextformat) ? editors_get_preferred_format() :    $question->questiontextformat;
        $question->questiontext['itemid'] = $draftid;

is confusing

The names used on the form should be at least changed to something like $form->questiontextarray so that there is no no name confusion with the $question or $answer parameters

 $question->questiontextarray['text']
 $question->questiontextarray['format']
 $question->questiontextarray['itemid']

Hi, all

Since moodle 2 RC1 will be released tomorrow, Martin asked me to close this issue, and solve remaing bugs in sperate issue:

For contextmoveq.php trying to access legacy course files in dataroot, it will be fixed by rewriting export/import: MDL-15573

For "save as new question" failing to store files, I created MDL-24293.

Please create new issues for bugs, and remining bugs will be fixed in linked issues

Actually, contextmoveq.php is not related to import/export. It is to do with moving quetsions around the question bank. However, I think that script is no longer necessary (it was a work-around for not having the file API).

I created MDL-24297 for deleting this file after a review.

Tim
You were right, formt.php is the one actually related to export/import.

Tim
Since moodle 2 RC1 will be released tomorrow, I resolve the cloze question i.e. question can be created and modified and set MDL-24168 as minor bug.
So we can live with the actual name convention

Argh! the upgrade code is totally buggered up.

Code like

if ($CFG->texteditors !== 'textarea') {
                if (!empty($record->questiontext)) {
                    $record->questiontext = text_to_html($record->questiontext);
                }
                $record->questiontextformat = FORMAT_HTML;
                // conver generalfeedback text to html
                if (!empty($record->generalfeedback)) {
                    $record->generalfeedback = text_to_html($record->generalfeedback);
                }
            } else {
                $record->questiontextformat = FORMAT_MOODLE;
            }

Is just total crap.

To start with, the Files API has been around for MONTHS. Please can we have some documentation about the right way to write upgrade code.

(When you are writing that documentation, please explain what the hell $CFG->texteditors !== 'textarea' is about)

Second $record->generalfeedback is almost certainly already HTML, so why are we breaking it like this?

Third, if we must call text_to_html, why aren't we calling it with all the dangerous arguments set to false, so that it does not add crappy <div class="text_to_html"> everywhere.

If someone can explain to me how this is supposed to work, I can probably make time to fix the code.

We will have to tell people who have already upgraded, and who have any questions at all, that they need to re-upgrade.

Tim
Thanks for your intervention in the process.
in lib/editorlib.php

    if (empty($CFG->texteditors)) {
        $CFG->texteditors = 'tinymce,textarea';
    }

However, as usual, ready to come back to the drawing board when things will be settled

Petr, can you please examine this and sort it out so we can close it?

Sorry, but texteditors !== file api. I was planning to rewrite the text formats completely, but there was no time for that so I had to use brute force hacks to make it work and somehow upgrade data.

The adding of format fields is tricky, you need to study the old code and guess format it is in - FORMAT_MOODLE or FORMAT_HTML? If html editor was disabled then probably it is in FORMAT_MOODL, but that may not be true because the code might have changed.

If you do not like the current hacks feel free to come up with better solution or documentation, I do not know how to do that myself.

—

• If you need to convert FORMAT_MOODLE to FORMAT_HTML you can use old trick text_to_html($something, false, false, true); - this will be definitely deprecated in 2.1
• if ($CFG->texteditors !== 'textarea') tells you if html editor was enabled in 1.9x, but you still need to review the original code what format_text() parameters were used in 1.9.x

Right, I think I have reviewed everything and fixed everything. However, Petr, please can you carefully review the changes I have just committed, and fix up anything that seems wrong.

Also, if anyone is in a position to test and upgrade of a Moodle 1.9 site with a lot of questions in it, I would be very grateful.

Also, Petr, if you have not already done so, please review http://docs.moodle.org/en/Development:Text_formats_2.0.

Side comment, David discovered some mins ago this: (line 74 of numerical/db/upgrade.php)

https://github.com/moodle/moodle/blob/MOODLE_20_STABLE/question/type/numerical/db/upgrade.php#L74

Seems copy/paste bug...ciao

Hi,

I notice this patch introduces adds a new column to the question table when upgrading, called "oldquestiontextformat", but that field was not added to install.xml.

Hence there is a DB schema difference between upgraded vs fresh install instances. That just doesn't seem right. If the column is only temporary related to the upgrade, why not use a temporary table or something to not permanently alter the schema? At at minimum, it should be in install.xml for consistency.

You are right. That column should no longer be there. Please create a new tracker issue about removing it.

The problem was that the question table is managed by lib/db/upgrade.php, but this information was needed in question/type/*/db, which is later in the upgrade process. So we had to make a temporary column, and then there was no suitable place to get rid of it again. And there is no need for this column on new installs, so I did not want to add it to install.xml.

Removing it in 2.2 seems like the best way forward from here in my mind.