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

Localize $@ when evaling #30

Merged
merged 1 commit into from Dec 29, 2015

Conversation

Projects
None yet
2 participants
@symkat
Member

symkat commented Nov 18, 2015

We experienced an issue at $work where trying to check a javascript:// set $@, and exception code on our side later died with the URI exception.

@karenetheridge karenetheridge self-assigned this Nov 18, 2015

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge Dec 27, 2015

Member

This test passes for me, even without applying your patch:

use strict;
use warnings;

use Test::More;
use URI;

plan skip_all => 'this test assumes that URI::javascript does not exist'
    if eval 'require URI::javascript';
plan tests => 2;

my $uri = URI->new('javascript://foo/bar');

is($@, '', 'no exception when trying to load a scheme handler class');
ok($uri->isa('URI'), 'but URI still instantiated as foreign');

So it looks like $@ is already being cleared out?

Member

karenetheridge commented Dec 27, 2015

This test passes for me, even without applying your patch:

use strict;
use warnings;

use Test::More;
use URI;

plan skip_all => 'this test assumes that URI::javascript does not exist'
    if eval 'require URI::javascript';
plan tests => 2;

my $uri = URI->new('javascript://foo/bar');

is($@, '', 'no exception when trying to load a scheme handler class');
ok($uri->isa('URI'), 'but URI still instantiated as foreign');

So it looks like $@ is already being cleared out?

@symkat

This comment has been minimized.

Show comment
Hide comment
@symkat

symkat Dec 27, 2015

Member
#!/usr/bin/env perl
use warnings;
use strict;
use Test::More;
use URI;

plan skip_all => 'this test assumes that URI::javascript does not exist'
    if eval 'require URI::javascript';

URI->new( 'javascript://foo/bar' );
is ($@, '', 'no exception when trying to load a scheme handler class');

my $uri = URI->new( 'javascript://foo/bar' );
is ($@, '', 'no exception when trying to load a scheme handler class, a second time.');
ok( $uri->isa( 'URI' ), 'URI instantiated as foreign' );

done_testing;

I trigger it when an object is instantiated two or more times as it is.

Member

symkat commented Dec 27, 2015

#!/usr/bin/env perl
use warnings;
use strict;
use Test::More;
use URI;

plan skip_all => 'this test assumes that URI::javascript does not exist'
    if eval 'require URI::javascript';

URI->new( 'javascript://foo/bar' );
is ($@, '', 'no exception when trying to load a scheme handler class');

my $uri = URI->new( 'javascript://foo/bar' );
is ($@, '', 'no exception when trying to load a scheme handler class, a second time.');
ok( $uri->isa( 'URI' ), 'URI instantiated as foreign' );

done_testing;

I trigger it when an object is instantiated two or more times as it is.

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge Dec 28, 2015

Member

Aha, we need to go around twice, because after the first attempt to load URI::javascript, we then load URI::_foreign, which clears $@.

Member

karenetheridge commented Dec 28, 2015

Aha, we need to go around twice, because after the first attempt to load URI::javascript, we then load URI::_foreign, which clears $@.

@karenetheridge karenetheridge merged commit 23d01eb into libwww-perl:master Dec 29, 2015

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge Dec 29, 2015

Member

Thanks!! I added a test case, and then changed the local to a a save-and-restore, as in Try::Tiny/Moo. Released as 1.70_001; if this smokes well it can go stable in a week or two!

Member

karenetheridge commented Dec 29, 2015

Thanks!! I added a test case, and then changed the local to a a save-and-restore, as in Try::Tiny/Moo. Released as 1.70_001; if this smokes well it can go stable in a week or two!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment