libwww-perl / WWW-Mechanize

Handy web browsing in a Perl object
https://metacpan.org/pod/WWW::Mechanize
Other
68 stars 53 forks source link

upgrade to HTML::TreeBuilder 5 to get weak references #262

Closed simbabque closed 5 years ago

simbabque commented 6 years ago

This might break people's code on really old Perls.

Closes #251

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 223


Files with Coverage Reduction New Missed Lines %
lib/WWW/Mechanize.pm 18 92.28%
<!-- Total: 18 -->
Totals Coverage Status
Change from base Build 222: 0.02%
Covered Lines: 725
Relevant Lines: 782

💛 - Coveralls
oalders commented 6 years ago

This might break people's code on really old Perls.

Could you elaborate?

simbabque commented 6 years ago

This might break people's code on really old Perls.

Could you elaborate?

Sure. In https://metacpan.org/pod/HTML::Element#Weak-References it says

Because HTML::Element stores a reference to the parent element, Perl's reference-count garbage collection doesn't work properly with HTML::Element trees. Starting with version 5.00, HTML::Element uses weak references (if available) to prevent that problem. Weak references were introduced in Perl 5.6.0, but you also need a version of Scalar::Util that provides the weaken function.

I merely paraphrased this. 5.6.0 is Old of course, and I did not check if Mech can work there at all. But I thought I'd at least mention it in the commit. I did change the minimum version of Scalar::List::Util from any to the earliest that brings weaken, as far as I could tell. The history is a bit strange as there are no version numbers that far back.

Anyway, I believe we are not going to seriously break anyone's code. It is just a backwards compatibility change that should probably be mentioned.

simbabque commented 6 years ago

It's also interesting that the travis builds failed because of perltidy. What happened there?

oalders commented 6 years ago

It's also interesting that the travis builds failed because of perltidy. What happened there?

We need to add a test dep on Perl::Tidy

oalders commented 6 years ago

Anyway, I believe we are not going to seriously break anyone's code. It is just a backwards compatibility change that should probably be mentioned.

Thanks for bringing that up. With this module, I don't think we care about going back that far. With some of the other modules in this org, we might be more conservative, but that's seriously old.

oalders commented 5 years ago

@skaji are you happy with this pull request?

skaji commented 5 years ago

@oalders Yes, I am. @simbabque Thanks for this PR.

oalders commented 5 years ago

Thanks @simbabque and @skaji!