From 74a0365b0a6123eb362df7e991f873dfa1402a35 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 29 Feb 2024 05:55:05 -0600 Subject: [PATCH] This is another attempt at fixing `student_ans` HTML escaping issues. This backs up one step from what was done in #1004. Instead of calling `protectHTML` on `$ans->{student_ans}` in the `cmp_error` method, that is instead called in the `cmp_parse` method in the case that the `$ans->{student_value}` is undefined. With #1004, if `student_value` was defined, and then a problem author calls `Value::Error` in a custom answer checker, it results in the `student_ans` being escaped twice because it is escaped in `cmp_parse` and then again after the custom answer checker is called. With this approach the `student_ans` is escaped when `cmp_parse` is called in either case regardless if if `student_value` is defined, and then not again later (if `student_value` is defined and thus the checker is called) This is also before the `format_matrix` method is called for array answers. This fixes an issue also caused by escaping those already HTML formatted answers. In that case the `student_ans` is completely redefined after this from the `student_formula`. With #1004, the `student_ans` is first HTML formatted in this case, and then later escaped which results in an incorrect display. This of course still fixes the issue that #1004 attempted to fix, because the `student_value` is not defined, and so the `protectHTML` now called in `cmp_parse` covers this case. I am sure that this is not the end of this issue, but is better than both before #1004 and after. I think that something along the lines of @dpvc's suggestion in https://github.com/openwebwork/pg/pull/990#issuecomment-1930494530 will be needed to fully resolve this issue. I suggest that rather than having `student_ans_text`, `student_ans_html`, `student_ans_latex`, and `preview_ans_text` as suggested there, we use `student_ans`, `preview_latex_string`, and `preview_text_string`. `student_ans` would always remain a text answer, and never be HTML formatted, `preview_ans_latex` would stay the same and be LaTeX, and `preview_text_string` would always be HTML. The naming is not the best since `preview_text_string` is actually HTML, but it is the closest to the current usage, and so would take the least work. Note that currently `preview_text_string` is not actually used for anything. Even though it is documented in `lib/AnswerHash.pm` as being what is supposed to be displayed for the preview. So this proposal just aligns that answer hash key with its intended use. Test cases: ``` DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); Context('Matrix')->flags->set(requireConstantVectors => 1); Context()->variables->are(s => 'Real', t => 'Real'); $ans = Formula('s<1, 0> + t<0, 1>'); $ans_cmp = $ans->cmp( checker => sub { my ($c, $s, $self) = @_; Value::Error('Try again') unless $c == $s; return 1; } ); BEGIN_PGML Enter [`[$ans]`]: [_]{$ans_cmp} END_PGML ENDDOCUMENT(); ``` With this problem enter `+t<0,1>`. Prior to #1004 the first vector would disappear from the displayed answer because `student_ans` was not escaped. After #1004 and still with this pull request this is displayed correctly. Also with this problem enter `s<1,0>+t<0,2>`. Prior to #1004 this displayed correctly. With #1004 it is displayed as `s*<1,0>+t*<0,2>`. With this pull request is is displayed correctly. ``` DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl'); Context('Matrix'); $A = Matrix([ [ 1, 0 ], [ 0, -1 ] ]); BEGIN_PGML Enter the matrix [`[$A]`]: [_]*{$A} END_PGML ENDDOCUMENT(); ``` With this problem enter `S, -1, 1, 0` for the matrix entries. Prior to #1004 this displayed correctly. With #1004 this is escaped twice, and so you see the HTML code displayed. With this pull request this is again displayed correctly. --- lib/Value/AnswerChecker.pm | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Value/AnswerChecker.pm b/lib/Value/AnswerChecker.pm index d8c3941dc5..85ed004fb8 100644 --- a/lib/Value/AnswerChecker.pm +++ b/lib/Value/AnswerChecker.pm @@ -182,6 +182,7 @@ sub cmp_parse { $self->cmp_diagnostics($ans); } } else { + $ans->{student_ans} = protectHTML($ans->{student_ans}); $self->cmp_collect($ans); $self->cmp_error($ans); } @@ -337,8 +338,6 @@ sub cmp_error { . protectHTML(substr($string, $s, $e - $s)) . '' . protectHTML(substr($string, $e)); - } else { - $ans->{student_ans} = protectHTML($ans->{student_ans}); } $self->cmp_Error($ans, $message); }