Bug #89630 for Dist-Zilla-Plugin-ReadmeAnyFromPod: Character encoding will break under forthcoming Dist::Zilla v5

Sat Oct 19 23:20:57 2013 dagolden [...] cpan.org - Ticket created

Subject:

Character encoding will break under forthcoming Dist::Zilla v5

Forthcoming Dist::Zilla version 5 will manage file encodings. Since Pod data comes from 'content' it will be in characters, not octets, so Pod::Text needs to turn off its encoding handling. The result can be set back to 'content' without further modification and Dist::Zilla will ensure the characters are properly encoded while the file is written to disk. See https://github.com/fayland/dist-zilla-plugin-readmefrompod/pull/1 for how this can be fixed for ReadmeFromPod. You will want to apply similar fixes to all your Pod translators. You should not release a fix until after Dist::Zilla v5, but a -TRIAL release is advised shortly to allow users to start testing with it.

Sun Oct 20 00:47:28 2013 RTHOMPSON [...] cpan.org - Correspondence added

On Sat Oct 19 23:20:57 2013, DAGOLDEN wrote: Show quoted text

> Forthcoming Dist::Zilla version 5 will manage file encodings. Since > Pod data comes from 'content' it will be in characters, not octets, so > Pod::Text needs to turn off its encoding handling. The result can be > set back to 'content' without further modification and Dist::Zilla > will ensure the characters are properly encoded while the file is > written to disk. > > See > for how this can be fixed for ReadmeFromPod. You will want to apply > similar fixes to all your Pod translators. > > You should not release a fix until after Dist::Zilla v5, but a -TRIAL > release is advised shortly to allow users to start testing with it.

Thank you for the notification. I will see about fixing my module.

Sun Oct 20 00:47:28 2013 The RT System itself - Status changed from 'new' to 'open'

Sun Oct 20 00:47:28 2013 RTHOMPSON [...] cpan.org - Taken

Sun Oct 20 02:02:26 2013 ether [...] cpan.org - Cc ETHER added

Mon Oct 21 02:06:21 2013 RTHOMPSON [...] cpan.org - Correspondence added

I've created a new branch for dzil v5 compatibility fixes: https://github.com/DarwinAwardWinner/Dist-Zilla-Plugin-ReadmeAnyFromPod/tree/dzil5 So far I've ported the fix from ReadmeFromPod to the plaintext and HTML generators, but I'm unsure how to do so for the Markdown and Pod parsers, since they use a different pattern. Also needed are tests for proper encoding handling.

Mon Oct 21 12:00:59 2013 ether [...] cpan.org - Correspondence added

On 2013-10-20 23:06:21, RTHOMPSON wrote: Show quoted text

> I've created a new branch for dzil v5 compatibility fixes: > https://github.com/DarwinAwardWinner/Dist-Zilla-Plugin- > ReadmeAnyFromPod/tree/dzil5 > > So far I've ported the fix from ReadmeFromPod to the plaintext and > HTML generators, but I'm unsure how to do so for the Markdown and Pod > parsers, since they use a different pattern. > > Also needed are tests for proper encoding handling.

Ryan, I've still got the tests I wrote a few weeks ago when I was poking into how the encoding was done -- and I also corresponded with one of the Pod::Simple maintainers about how the encoding information can be fetched. I'll finish up the phase shift branch I was working on, and then look at the encoding stuff!

Mon Oct 21 12:32:49 2013 dagolden [...] cpan.org - Correspondence added

On Mon Oct 21 02:06:21 2013, RTHOMPSON wrote: Show quoted text

> So far I've ported the fix from ReadmeFromPod to the plaintext and > HTML generators, but I'm unsure how to do so for the Markdown and Pod > parsers, since they use a different pattern.

Pod::Markdown uses Pod::Parser which (I think) doesn't do anything about encoding. So you're taking content (decoded), turning that into an IO::String, which is a bit wonky, but probably OK since I suspect Pod::Parser is reading lines and not bytes. Then the content you get back from it is probably text (still decoded) so you can pass that back to content safely. For Pod, you're just returning the content again and that seems fine. What looks wrong, though, is when you write a file to root, you're opening the handle :raw but passing content. You need to be writing it with :encoding(UTF-8) so it gets encoded correctly. That doesn't give users a choice to have a different encoding, but it probably won't be an issue in practice.

Mon Oct 21 13:09:22 2013 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #89630] Character encoding will break under forthcoming Dist::Zilla v5
Date:	Mon, 21 Oct 2013 10:09:05 -0700
To:	David Golden via RT <bug-Dist-Zilla-Plugin-ReadmeAnyFromPod [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Mon, Oct 21, 2013 at 12:32:50PM -0400, David Golden via RT wrote: Show quoted text

> Pod::Markdown uses Pod::Parser which (I think) doesn't do anything about encoding. So you're taking content (decoded), turning that into an IO::String, which is a bit wonky, but probably OK since I suspect Pod::Parser is reading lines and not bytes. > Then the content you get back from it is probably text (still decoded) so you can pass that back to content safely.

Remember that filehandles can't be opened against scalar refs without declaring the binmode; 'wide character' warnings will be rampant. Show quoted text

> For Pod, you're just returning the content again and that seems fine. > What looks wrong, though, is when you write a file to root, you're opening the handle :raw but passing content. You need to be writing it with :encoding(UTF-8) so it gets encoded correctly. That doesn't give users a choice to have a different encoding, but it probably won't be an issue in practice.

I would suggest re-using whatever encoding the original file came in - which Pod::Simple has an interface method to provide. I have a test for a pod file that declares "=encoding Latin1". Also iirc Pod::Simple only provides the data back as *bytes*, not characters, so it needs to be decoded first before being munged.

Mon Oct 21 20:47:57 2013 RTHOMPSON [...] cpan.org - Correspondence added

RT-Send-CC:

ether [...] cpan.org

Show quoted text

> I would suggest re-using whatever encoding the original file came in - > which Pod::Simple has an interface method to provide. I have a test > for a > pod file that declares "=encoding Latin1".

But in dzil v5, my module is receiving the already-decoded content, so how do I figure out the encoding of the source file? Show quoted text

> Also iirc Pod::Simple only provides the data back as *bytes*, not > characters, so it needs to be decoded first before being munged.

Does this still apply in dzil v5?

Mon Oct 21 22:31:24 2013 RTHOMPSON [...] cpan.org - Correspondence added

Show quoted text

> But in dzil v5, my module is receiving the already-decoded content, so > how do I figure out the encoding of the source file?

Never mind this question, I found RJBS's post on dzil v5 with the details. Files now have an encoding attribute.

Tue Oct 22 01:02:53 2013 RTHOMPSON [...] cpan.org - Correspondence added

I've pushed some more changes to my dzil5 branch that I think should make this fully compatible. However, I need to install dzil v5 to test it.

Tue Oct 22 01:57:15 2013 RTHOMPSON [...] cpan.org - Correspondence added

On Tue Oct 22 01:02:53 2013, RTHOMPSON wrote: Show quoted text

> I've pushed some more changes to my dzil5 branch that I think should > make this fully compatible. However, I need to install dzil v5 to test > it.

Ok, on my system, b5b9706 passes all the existing tests when run with dzil v5. I think I'll see about cherry-picking some of Karen Etheridge's unicode tests before I decide this is ready to release.

Tue Oct 22 18:05:57 2013 RTHOMPSON [...] cpan.org - Correspondence added

On Tue Oct 22 01:57:15 2013, RTHOMPSON wrote: Show quoted text

> I think I'll see about cherry-picking some of Karen > Etheridge's unicode tests before I decide this is ready to release.

I have created what I think is a valid unicode test in 61b711f. The module passes, as hoped. I guess I should make a trial release now?

Tue Oct 22 23:23:53 2013 ether [...] cpan.org - Correspondence added

Show quoted text

> I have created what I think is a valid unicode test in 61b711f. The > module passes, as hoped. I guess I should make a trial release now?

I'm looking at what it would take to make this work on either version of dzil... set file content to Dist::Zilla->VERSION < 5.0 ? encode($encoding, $content, Encode::FB_CROAK()) : $content _get_source_encoding should return undef when not $file->can('encoding') utf8::upgrade($pod) if not utf8::is_utf8($pod) ...I think.

Wed Oct 23 18:06:49 2013 ether [...] cpan.org - Correspondence added

I'd suggest switching from File::Slurp to Path::Tiny to write out files... File::Slurp has a lot of outstanding issues and is sloppy with encoding, and we know where the Path::Tiny author lives :)

Wed Oct 23 18:16:38 2013 RTHOMPSON [...] cpan.org - Correspondence added

On Tue Oct 22 23:23:53 2013, ETHER wrote: Show quoted text

> I'm looking at what it would take to make this work on either version > of dzil...

Fell free to submit a pull request for this. As I said, I have no expertise in encoding issues, so I have no idea how to make sure this module does the right thing under dzil v4. Hence, I will make no attempt at this myself. Did you want me to hold off on a dzil v5 compatible release until you can make it work with both, or should we wait until later for that?

Wed Oct 23 18:21:52 2013 ether [...] cpan.org - Correspondence added

CC:	dagolden [...] cpan.org, ether [...] cpan.org
Subject:	Re: [rt.cpan.org #89630] Character encoding will break under forthcoming Dist::Zilla v5
Date:	Wed, 23 Oct 2013 15:21:36 -0700
To:	"Ryan C. Thompson via RT" <bug-Dist-Zilla-Plugin-ReadmeAnyFromPod [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Wed, Oct 23, 2013 at 06:16:38PM -0400, Ryan C. Thompson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=89630 > > > On Tue Oct 22 23:23:53 2013, ETHER wrote:

> > I'm looking at what it would take to make this work on either version > > of dzil...

> > Fell free to submit a pull request for this. As I said, I have no expertise in encoding issues, so I have no idea how to make sure this module does the right thing under dzil v4. Hence, I will make no attempt at this myself.

ok! it's on my list :) Show quoted text

> Did you want me to hold off on a dzil v5 compatible release until you can make it work with both, or should we wait until later for that?

I have no objection to you releasing now as long as you release as --trial -- if you release as a stable release, that would force users to upgrade to dzil 5.x trial (as well as fill your inbox with lots of failing cpantesters reports, as it won't be able to resolve the dependencies on unindexed versions).

Wed Oct 23 20:20:31 2013 RTHOMPSON [...] cpan.org - Correspondence added

I have now made a trial release, v0.132973, which I believe works correctly under dzil v5. Please let me know if there are any issues with it, and let me know when dzil v5 is release so I can do the real releasr ASAP afterward.

Mon Nov 25 00:14:09 2013 KENTNL [...] cpan.org - Correspondence added

Dzil 5 has been out in non-trial now for about 2 weeks.

I look forward to a stable release of ReadmeAnyFromPOD

☺

Mon Nov 25 00:14:10 2013 KENTNL [...] cpan.org - Fixed in 0.133060-TRIAL added

Mon Nov 25 00:24:25 2013 rct [...] thompsonclan.org - Correspondence added

Subject:	Re: [rt.cpan.org #89630] Character encoding will break under forthcoming Dist::Zilla v5
Date:	Sun, 24 Nov 2013 21:24:11 -0800
To:	bug-Dist-Zilla-Plugin-ReadmeAnyFromPod [...] rt.cpan.org
From:	Ryan Thompson <rct [...] thompsonclan.org>

Oops, I haven't kept on top of this. I'll bump the version and release shortly. On Nov 24, 2013 9:14 PM, "Kent Fredric via RT" < bug-Dist-Zilla-Plugin-ReadmeAnyFromPod@rt.cpan.org> wrote: Show quoted text

> Queue: Dist-Zilla-Plugin-ReadmeAnyFromPod > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=89630 > > > Dzil 5 has been out in non-trial now for about 2 weeks. > > I look forward to a stable release of ReadmeAnyFromPOD > > ☺ > >

Mon Nov 25 03:44:40 2013 RTHOMPSON [...] cpan.org - Correspondence added

Ok, I've made a new release, so I'm marking this resolved.

Mon Nov 25 03:44:40 2013 RTHOMPSON [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Nov 25 09:56:08 2013 RTHOMPSON [...] cpan.org - Fixed in 0.133290 added

Mon Nov 25 09:56:08 2013 RTHOMPSON [...] cpan.org - Fixed in 0.133060-TRIAL deleted