Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 11, 2018

Conversation

simbabque
Copy link
Contributor

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

Closes #251

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

Closes libwww-perl#251
@coveralls
Copy link

coveralls commented Nov 2, 2018

Pull Request Test Coverage Report for Build 223

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 18 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 92.711%

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

💛 - Coveralls

@@ -834,13 +834,16 @@ sub text {
my $self = shift;

if ( not defined $self->{text} ) {
require HTML::TreeBuilder;
unless ( exists $INC{'HTML::TreeBuilder'} ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you skip checking version and importing -weak if exists $INC{'HTML::TreeBuilder'}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am checking the version, but it has to be done after the require call. It's easy to do it with use, but the code we have here explicitly loads the module at run-time. In the require perldoc it explains

If the VERSION argument is present between Module and LIST, then the use will call the VERSION method in class Module with the given version as an argument:

use Module 12.34;

is equivalent to:

BEGIN { require Module; Module->VERSION(12.34) }

I put the %INC lookup in because without it, it will do the version check and import call every single time this method is called. I would have liked to tie it to require, but it always returns true even if the module has already been loaded. Under the hood it also checks in %INC as far as I understand, so doing that myself before attempting the version check and import seemed like the right thing to do.

I'm happy to add a test that proves this works.

@oalders
Copy link
Member

oalders commented Nov 2, 2018

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

Could you elaborate?

@simbabque
Copy link
Contributor Author

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
Copy link
Contributor Author

simbabque commented Nov 5, 2018

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

@oalders
Copy link
Member

oalders commented Nov 5, 2018

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
Copy link
Member

oalders commented Nov 5, 2018

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
Copy link
Member

oalders commented Nov 10, 2018

@skaji are you happy with this pull request?

@skaji
Copy link
Member

skaji commented Nov 11, 2018

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

@oalders
Copy link
Member

oalders commented Nov 11, 2018

Thanks @simbabque and @skaji!

@oalders oalders merged commit e5ed735 into libwww-perl:master Nov 11, 2018
@simbabque simbabque deleted the pr/gh251 branch November 12, 2018 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use HTML::TreeBuilder 5 -weak;
4 participants