|
Description
Joonas Kylmälä
2021-04-26 14:50:02 UTC
I haven't verified yet, but that would be a killer for any German installation... This works:
use utf8;
use Encode qw(encode encode_utf8);
use Data::Dumper;
use YAML::XS;
binmode STDOUT, ':encoding(UTF-8)';
my $string = YAML::XS::Load(encode('UTF-8',"---\r\n hello: 'heÄllo'"));
print STDOUT Dumper($string);
$string = YAML::XS::Load(encode_utf8("---\r\n hello: 'heÄllo'"));
print STDOUT Dumper($string);
1;
(In reply to Tomás Cohen Arazi from comment #2) > This works: I wanted to add that I totally reproduced this following the test plan, with the first record that comes with KTD (Russian). (In reply to Tomás Cohen Arazi from comment #2) > This works: > > use utf8; also adding use "utf8;" to the debug statement makes things work but I actually get different results with them: YAML::Load: > --- > hello: heà > llo YAML::XS > --- > hello: heÄllo Anyway, if I'm not mistaken the use utf8 is just for the source code encoding, here we load the string from database. But good note this down. Maybe our input / biblio title is not properly utf8 encoded or something. (In reply to Joonas Kylmälä from comment #4) > also adding use "utf8;" to the debug statement makes things work but I > actually get different results with them: Meant to say the debug statement I posted in the first comment here. Created attachment 120196 [details] [review] Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly I was going to attach this patch:
diff --git a/C4/Message.pm b/C4/Message.pm
index 659ff96aeb2..d9bb945abb6 100644
--- a/C4/Message.pm
+++ b/C4/Message.pm
@@ -283,7 +283,7 @@ sub metadata {
$self->content($self->render_metadata);
return $data;
} else {
- return YAML::XS::Load(Encode::encode_utf8($self->{metadata}));
+ return YAML::XS::Load($self->{metadata});
}
}
So Joonas proposes to store octets in metadata by added missed "decoding to octets", and Jonathan proposes to store strings so to remove excessive in this case "encoding", I would agree to have more "just normal Perl strings around" and walk away as much as possible from "encode/decode", ... if this works (this might be uh :) ). (In reply to Jonathan Druart from comment #7) > I was going to attach this patch: > > diff --git a/C4/Message.pm b/C4/Message.pm > index 659ff96aeb2..d9bb945abb6 100644 > --- a/C4/Message.pm > +++ b/C4/Message.pm > @@ -283,7 +283,7 @@ sub metadata { > $self->content($self->render_metadata); > return $data; > } else { > - return YAML::XS::Load(Encode::encode_utf8($self->{metadata})); > + return YAML::XS::Load($self->{metadata}); > } > } Why I didn't do this is because to me it looks to be incorrect in this scenario: 1. Load the object with find_last_message() -> metadata is now perl string 2. Call $message->append() -> it then calls my $metadata = $self->metadata; and we would be calling > + return YAML::XS::Load($self->{metadata}); which means we are calling Load() with a Perl string and not UTF-8 octets which is wrong. right. Quote:
USING YAML::XS WITH UNICODE
Handling unicode properly in Perl can be a pain.
YAML::XS only deals with streams of utf8 octets
so seems this is that case when I want to have less encode/decode, but is impossible to avoid.
Jonathan, then: Joonas did right
The > sub enqueue { looks buggy as well. > $letter->{metadata} = Dump($metadata); We should decode it to be a Perl string as well because in C4::Letters::EnqueueLetter which is called next and this is passed there it calls $sth->execute() with it. At least the other execute params I see are perl strings and not utf-8 octets so if it is not buggy it is unintuitive at least. but this enqueue bug is not related to this bug just wanted to mention because it is similar and might cause badly encoded emails. Created attachment 120202 [details] [review] Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly I re-made the patch now so that the enqueue() function is free of this bug as well. Created attachment 120203 [details] [review] Bug 28230: Add tests Created attachment 120204 [details] [review] Bug 28230: Add tests And those tests prove you are right Joonas :) There is a warning displayed when the tests are run but I am suspecting an issue in t::lib::TestBuilder::builder_sample_biblio. (In reply to Jonathan Druart from comment #16) > And those tests prove you are right Joonas :) > > There is a warning displayed when the tests are run but I am suspecting an > issue in t::lib::TestBuilder::builder_sample_biblio. build_sample_biblio See bug 28234. Created attachment 120210 [details] [review] Bug 28230: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 120211 [details] [review] Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Joonas Kylmälä from comment #13) > I re-made the patch now so that the enqueue() function is free of this bug > as well. +1 Created attachment 120227 [details] [review] Bug 28230: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 120228 [details] [review] Bug 28230: Store C4::Message->{metadata} as Perl string always When loading C4::Message->{metadata} we load it as perl string in all other cases, see e.g. the subroutine find_last_message(). In this one case we incorrectly populate the C4::Message->{metadata} as an UTF-8 octet instead of a Perl string. This causes a problem later on because encode_utf8 later on in the line: YAML::XS::Load(Encode::encode_utf8($self->{metadata})); excepts a perl string and not a UTF-8 octet (please refer to the functions perldoc). This breaks the encoding and causes an internal server error: To test the error is gone: 1. Create biblio with "AE" (bugzilla doesn't let me write the real letter here, see it in the bugzilla comments) in title 2. Enable RenewalSendNotice 3. Enable "Item checkout and renewal" message preference for patron 4. Checkout an item from biblio "AE" to patron 5. Try to renew the patron's loan and notice the renewal fails 6. Apply patch and restart plack 7. Notice renewal works now and message h's the AE letter displayed correctly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Good catch guys, that's a nasty bug. Man I hate encoding issues.. All works as expected for me now.. passing QA Created attachment 120230 [details] [review] Bug 28230: Add execute bit to test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Pushed to master for 21.05, thanks to everybody involved! Depends on Bug 22824 not in 20.11.x |