search

studentquiz / moodle-mod_studentquiz

Moodle-Plugin
GNU General Public License v3.0
38 stars 37 forks source link

StudentQuiz: Fix shorten text in comment to display correctly. #406

Closed vuvanhieu143 closed 2 years ago

vuvanhieu143 commented 2 years ago

Hi Tim, I have removed the custom shorten text in SQ and using the core function.

Could you please help me to peer-review again?

timhunt commented 2 years ago
  1. Moodle code should not use htmlspecialchars. It should use Moodle's equivalent, which is s(). However, should we even be doing this? Why are we not doing the escaping when the text is put into the HTML? That would be more normally, and avoids double-escaping bugs. Anyway, the code is not self-evidently right, so it would be nice to see the automates tests covering this.
  2. (The code would be easier to review if anything about the convert_to_object function (name, PHPdoc comment) explained what this function was acutally doing.)
  3. When calling funcitons, don't pass final optional arguments that match the default. (e.g. , false, '...' for shorten_text). If you are using PHP storm, it warns you about this.
vuvanhieu143 commented 2 years ago

1) Moodle code should not use htmlspecialchars. It should use Moodle's equivalent, which is s(). However, should we even be doing this? Hi @timhunt We have an edge case where the text contains an escaped HTML entity: Example: "Test shortened text with html enity<br>" After we run through the html_to_text, it will change the "<br>" to
so I need to s() to escape it again. "Why are we not doing the escaping when the text is put into the HTML?" The shortened text should only contain the plain text version of our full text. I don't think we need to escape this, but I found we didn't use format_text when we output to the template, so I add it to the code. Is that ok? 2) I just add more comments to the docs.Is it make more sense now? 3) I have removed it. Sorry for the stupid question, but is there any setting I need to add to my PHPStorm? My PHPStorm did show the hint or warning at all.

timhunt commented 2 years ago

You clearly did not understand my comment about it was best to do escaping, but I am not going to debate it any more now. The code probably works without missing or double-excaping, so let's merge an move on.