search

libwww-perl / URI

The Perl URI module
https://metacpan.org/pod/URI
Other
41 stars 48 forks source link

Bugfix for issue#102 -- "5.11 breaks HTML::FormatExternal" #103

Closed Perlbotics closed 2 years ago

Perlbotics commented 2 years ago

Test case added. Skipping attempt to unescape empty authority part.

See: https://github.com/libwww-perl/URI/issues/102

Perlbotics commented 2 years ago

Hi Olaf (@oalders), seems that an entry is missing in Changes? Can I edit this file myself or is that task performed by a tool like dzil somewhere upstream?

oalders commented 2 years ago

That can be done manually so feel free to add the entry. 😀

Perlbotics commented 2 years ago

Thank you, Olaf (@oalders). Changes updated and pushed.

Perlbotics commented 2 years ago

Hi Olaf (@oalders), I identified schemes that do not have an authority part and thus do not need a special treatment of the host part (768e8e6). Early return for these. Not quite necessary, but I think the intention is cleaner now.

The second patch (84176ec) is more important. 'mailto:' has a complex format that needs special treatment. Since I have no solution for now, the previous 5.10 encoding is restored. I also added test cases to t/mailto.t that would have helped to detect this bug earlier.

Please consider to publish the corrections or to revert to 5.10 since the current bug (file:///) might have security implications or could cause havoc on the file system.

Update: regression tests in t/file.t were only verified for Linux. See https://github.com/libwww-perl/URI/pull/103#discussion_r917234611

oalders commented 2 years ago

Thanks for being a good sport through all of these iterations @Perlbotics! I've merged this manually into master as 725fbfb6d59fbe8.

Closes #102.

Perlbotics commented 2 years ago

Thank you Olaf (@oalders), Julien (@simbabque), and Graham (@haarg) for your patience and support. This was the first time, I participated in a github patch activity and while finding great support here, I also learned a lot.

BTW: If you want to improve test coverage, try to run everything once more with something like: URI_HAS_RESERVED_SQUARE_BRACKETS=1 prove -l I've done this manually here w/o problems but perhaps it is worth an extra author test?

Thanks.

oalders commented 2 years ago

This was the first time, I participated in a github patch activity and while finding great support here, I also learned a lot.

Congratulations! 🎊 I'm glad it was a positive experience.

If you want to improve test coverage, try to run everything once more with something like: URI_HAS_RESERVED_SQUARE_BRACKETS=1 prove -l

We can do this in the GitHub workflow, which is at https://github.com/libwww-perl/URI/blob/master/.github/workflows/dzil-build-and-test.yml

Maybe add a job which needs: build-job and just runs the tests again on Linux using Perl 5.36. I don't think we necessarily want to run the test suite twice for the entire matrix. If we just add that one extra run, I think that would be good enough.

simbabque commented 2 years ago

Maybe add a job which needs: build-job and just runs the tests again on Linux using Perl 5.36. I don't think we necessarily want to run the test suite twice for the entire matrix. If we just add that one extra run, I think that would be good enough.

@Perlbotics if you need help with this, give us a shout. I'm happy to explain how these actions work. :)