There are some places in Koha where we use mailto: links with a subject line. An example is the email link added by bug 36595. I believe we need to take a closer look at making sure we are encoding the subject line correctly. Also need to make sure to not include untranslatable and therefore hardcoded bits into the subject messages.
So, on my quick research I reckon we need a new TT filter to make sure subjects are properly RFC2047 encoded. We should be able to add that fairly trivially using Encode::MIME::Header I think.
Created attachment 171518 [details] [review] Bug 37918: Add email_header template toolkit filter This patch adds a new email_header TT filter to properly encode strings according to RFC2047 as per the email specification. This allows developers to set default subjects in mailto links with the correct encoding.
Created attachment 171994 [details] [review] Bug 37918: Add email_header template toolkit filter This patch adds a new email_header TT filter to properly encode strings according to RFC2047 as per the email specification. This allows developers to set default subjects in mailto links with the correct encoding. Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Assuming I only needed to make sure the test passes for the sign-off. Should we include refactoring of mailto with subject in this bug?
Yes indeed, that was the next step here.
Created attachment 176576 [details] [review] Bug 37918: Remove the new dependency
Do we really need the new dep?
24 use Encode::MIME::Header; but you are not using it?
Would PQA this, if it weren't for these 2 lines: > Should we include refactoring of mailto with subject in this bug? > Yes indeed, that was the next step here.
(In reply to Jonathan Druart from comment #8) > 24 use Encode::MIME::Header; > > but you are not using it? That module is what adds the 'MIME-Header' support to 'encode'.. without it we're not encoding in MIME-Header format... see line 55 of the patch.
If someone wants to follow-up here and apply the filter to all the current email subjects that would be great.. I'm unlikely to have a chance to continue here myself any time soon.
So should we change the bug description and scope this bug to the filter itself? Then we at least have it. Also quickly looked again at: https://metacpan.org/pod/Encode::MIME::Header I don't like it when libraries work that way, but it is what it is.
Created attachment 176869 [details] [review] Bug 37918: Add filter to existing email subjects
Created attachment 176870 [details] [review] Bug 37918: Add email_header template toolkit filter This patch adds a new email_header TT filter to properly encode strings according to RFC2047 as per the email specification. This allows developers to set default subjects in mailto links with the correct encoding. Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 176871 [details] [review] Bug 37918: Remove the new dependency Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Sent a keyboard interrupt by accident.. Fixing.
Hopefully that's all the cases covered.. must admit, I've not actually tested that last patch
Created attachment 176873 [details] [review] Bug 37918: Add email_header template toolkit filter This patch adds a new email_header TT filter to properly encode strings according to RFC2047 as per the email specification. This allows developers to set default subjects in mailto links with the correct encoding. Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 176874 [details] [review] Bug 37918: Remove the new dependency Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 176875 [details] [review] Bug 37918: Add filter to existing email subjects Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This patch set raises a few questions: [1] How is [% something | email_header %] supposed to work? Where does email_header link to the EmailHeader module? I think this just returns nothing. I would expect [% something| $EmailHeader %] Since this was tested by two people, please explain. (No test plan in the third patch.) [2] Are you sure that you are doing the right thing here? We are in an HTML context, not working with email headers directly but representing them in html. Please clarify. Why =?UTF-8?B?SGVsbG8gV29ybGQ=?= instead of Hello%20World for instance? Should the script sending a mail subject to the html template be responsible? [3] How do you prevent double encoding?
You are correct Marcel, I was careless especially with that last patch. I had a conversation with Martin during QA and didn't run the qa script when quickly signing off that last patch (which would've caught the incorrect usage). Thanks for catching my mistake. In regard to the reasoning: 1) Since the tests passed (and there was no usage at that point during QA), I didn't even look at the usage in the POD. You are correct, the correct usage should be [% something | $EmailHeader %]. 2) This needs further investigation, I can't even reproduce the root issue that this bug was about with unfiltered korean. 3) Valid concern.
Created attachment 177103 [details] [review] Bug 37918: Correct POD
Created attachment 177104 [details] [review] Bug 37918: Correct usage
Here's at least the usage corrected, but it would be nice to see a failing example.