Moodle

New version of the patch from today ("MDL-14442-20080420.patch") with additional fixes:

+ When "url" type fields also have a name, it also gets imported. A complete url field within the CSV file should look like: url name

+ First round of inserting new data_content fields ensures all contents to be NULL, the actual values get filled in at the second round.

Tested successfully with these field types:
text, textarea, menu, multimenu, url, latlong

Next version of patch takes care when the content1 value (the name of an url field) contains spaces.

Assigning to Dongsheng... can you please, test latest patch (MDL-14442-20080421.patch (3 kb) and apply it (with MD confirmation).

Thanks a lot by your continued work, Robert! B-)

Addressing this for Moodle 2.0 (mainly to be paired with MDL-8407 ). Martin, can you confirm fixfor version? Sounds like an interesting option to be present ASAP but... FYC.

Ciao

Moodle 1.9.1 may be released next Wednesday, so it would be nice to see a progress of this issue before.

Thanks,
Robert

Hi, Rebert, sorry for delay(I didn't work last Friday), I will confirm this issue with Martin next Monday, and merge your patch to 1.9.1 before Wednesday, so no worries.

Thanks Martin for giving green light for inclusion in 1.9.1. I have committed the patch to MOODLE_19_STABLE now.

Best regards,
Robert

Hi Robert,

some comments about your commit to 19_STABLE:

• You need to commit it too to HEAD (any commit performed in branches must go to HEAD), marking the 19_STABLE files as merged once performed the commit.
• If I'm not wrong this was paired with MDL-8407 (the option to export). So both should be there, if I'm not wrong.
• 100% np... but we use to comment here before performing commits assigned to anybody else (Dongsheng in this case). I insist, 100% np... only trying to comment "courtesy" norms. I re-insist, 100% np! :-D

Thanks and ciao

Dear Eloy,

sorry for not having being compliant to the usual way - this actually was my first commit to the Moodle code so I'm still a newbie on these norms .

Allowing to import a CSV file properly also makes sense without an option to export, so it does not harm yet not having the export option, too. When the export ability will be committed, the existing import ability already works well with it. There are still some things to do on the export side. I will post an updated patch at MDL-8407 soon.

Dongsheng, would you like to do what I also should have done (committing to HEAD)? And please don't hesitate to make changes to my commit if you feel there should be done something.

Hi, Robert, thanks for you committing

Did you check out the HEAD? Go to the HEAD version, then merge the changes from MOODLE_19_STABLE, in this case, use this command:

cvs update -kk -j MOODLE_19_MERGED -j MOODLE_19_STABLE import.php

This command will merge your change to the local copy, you should commit this change to HEAD.
Update the floating merge tag (MOODLE_19_MERGED) for the affected files so that this process can be repeated next time.

See more: http://docs.moodle.org/en/CVS_for_Developers

I guess you are not online, so I merged the changes to HEAD for you, I hope you don't mind, thanks

Can you please add some error checking on the return values for update_record and insert_record calls?

Something like this (MDL-14442_error-checking.patch)?

Yep, thanks!

OK, committed to MOODLE_19_STABLE. Dongsheng, I think merging it into HEAD cannot be automatically, since I think the function error() must be replaced by print_error(). Would you like to do this?

Thanks,
Robert

I have replaced the error(), thanks Robert.

Fixed and merged into HEAD, please review, feel free to reopen if you find any problem.

This patch (MDL-14442_addslashes.patch) should also be applied - otherwise the import process fails at the first occurence of a field name or value containing quotes.

The addslashes patch should only add backslashes to $value (not to $name). Attached is the corrected patch (MDL-14442_addslashes_2.patch). tested successfully.

Hi Robert, well spotted!

Are you going to commit this? Else I'll do in 19_STABLE and HEAD (1.9.1 is really near and I think it's ok and safe to send the fix before release).

(another tip: if one resolved/closed bug needs further action - like this one - feel free to reopen it. That way, it will ping on each developer list of pending bugs, else it can be forgotten easily. Or alternatively, create a new bug, as you want. Personally I prefer to reopen if it's directly related and has been resolved/closed 1-2 days ago).

Reopening this... thanks!

Ciao

Thanks Eloy for all your tips

I have committed the fix to 19_STABLE and HEAD. Let's hope 1.9.1 will get it.

Eloy, just FYI, I cannot change the status of this issue.

Thanks Robert, feel free to reopen

I've made you a developer in the tracker now, Robert, so you can assign things.

Thanks Martin and congratulations for the 1.9.1 release

addslashes() is not enough. htmlspecialchars() must also be applied to each field value before importing it. Otherwise, clean_param($value, PARAM_NOTAGS) would cut off everything beginning from a < sign (but not from a > sign).

Having a field "sample description" for example "fraction > 200µm" would currently work, but "fraction < 200µm" would be cut off to "fraction ". With the attached patch (MDL-14442_htmlspecialchars.patch), all variants will work. clean_param($value, PARAM_NOTAGS) would no more be needed because after a htmlspecialchars() no literal < or > can exist any longer, however, I have kept it to be double-safe.

Tested successfully. If I get green light, I will commit it into MOODLE_19_STABLE and HEAD.

new patch posted above needs to be applied

Uhm... I'm not really sure if that's the best solution. Some fields must be able to handle proper HTML and using htmlspecialchars() will kill content, for sure.

Instead, the followed approach should be to "relax" the validation of the field to PARAM_RAW (looking for analogies with the cleaning performed in forum->posts or glossary->entries) and applying trustext functions to it. Or use, simply, PARAM_CLEAN, that performs basic protection.

Petr, any insight ?

Trusttext is very tricky my -100 for that here.
The fields are responsible for the XSS protection, in general the Data mod allows teacher to use JS in templates, but it must prevent JS in data that was received from students. Normally we do the cleaning before the display, which means there should not be issues during import of actual entries. In any case only the field implementation knows what to do, we should let it handle this if needed.

Eloy,

PARAM_RAW does not do any cleaning/processing at all, so its using it would be completely useless.

Regarding PARAM_CLEAN, a comment in 'moodlelib.php' reads as follows:
/**

• PARAM_CLEAN - obsoleted, please try to use more specific type of parameter.
• It was one of the first types, that is why it is abused so much
  */

The only field type that may contain HTML tags is 'textarea'. Thus, htmlspecialchars() could safely be applied at least to all other field types. How should we proceed?

You do not know what some 3rd party fields do with the data, hardcoded htmlspecialchars() here is imho a bad solution. I really think that by default it should do nothing and if needed let fields decide what to do.

Petr,

so how would your proposed patch look like? Thew current implementation (clean_param($value, PARAM_NOTAGS)) is not good.

Unfortunately I can not help you now, will be back working on moodle code on Thursday

The new attached patch (MDL-14442-clean_param.patch) tries to solve this problem better. I have completely removed clean_param($value, PARAM_NOTAGS) since it neither allows HTML nor literal < or > symbols. As mentioned above, the only field type where HTML is possible is 'textarea', thus I now treat this type differently than all the others:

To keep HTML in 'textarea' fields, clean_param($value, PARAM_CLEANHTML) cleaning will be performed. This is a compromise since it still strips something like "fraction < 200µm" to "fraction " (but keeps "fraction > 200µm" as is). The workaround is to replace those symbols in the CSV file prior to use it for importing.

In all other fields, literal < and > symbols will be replaced by their HTML entities (but not & - so htmlspecialchars() would in fact be too much). I have entensively tested this patch together with MDL-8407_20080518.patch and both work fine for me.

Eloy, what do you think?

I am planning to commit MDL-14442-clean_param.patch to MOODLE_19_STABLE and HEAD in a few days, since there has yet not been any response against it. I have tested it on importing large sets of records, with all possible field types, and it works fine (in contrast to the currently used PARAM_NOTAGS on all field types). I don't see a reason to keep known buggy implementations for a long time when there are tested working solutions available. If someone is against the commit, please give feedback.

Dongsheng, since you have been assigned as QA assignee, could you pls verify my last commit, so we could reclose this bug?

Reclose, Thanks