Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

Test/Block: Fixed an error with load_class. #75

Merged
merged 1 commit into from
May 30, 2014
Merged

Test/Block: Fixed an error with load_class. #75

merged 1 commit into from
May 30, 2014

Conversation

jagtalon
Copy link
Member

load_class was returning undef, which makes accessing trigger_block_type an invalid operation. Thanks to @nilnilnil for solving the problem.

It was happening on https://travis-ci.org/duckduckgo/zeroclickinfo-goodies/jobs/25999269#L850

`load_class` was returning `undef`, which makes accessing `trigger_block_type` an invalid operation. Thanks to @nilnilnil for solving the problem.
@moollaza
Copy link
Member

@jagtalon @nilnilnil can you explain how this problem occurred? Did Class::Load change? The tests that failed haven't changed and the related code hasn't so I'm just a little lost.

if ($plugin->triggers_block_type eq 'Words') {
push @words, $plugin;
} elsif ($plugin->triggers_block_type eq 'Regexp') {
push @regexp, $plugin;
Copy link
Member

Choose a reason for hiding this comment

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

@jagtalon these look basically identical. How does this fix the problem?

I'm genuinely curious -- does the use of a local var over the default $_ make a difference?

@nilnilnil
Copy link
Contributor

@moollaza that's a great question. I do not have a concrete answer for you. All I know is that the problem manifested on codeio for perl 5.18.2 and not on our usual deployment on 5.16.3. I'm unsure of everything in between. I do know that $_ is a special variable that, when used in this context in codeio, was changing from a string delineating the class name to undef. Switching to an explicit SV in the loop changed that. I am unsatisfied with why it's resolved by this fix as well.

@moollaza
Copy link
Member

@nilnilnil thanks for explaining! Certainly some odd behaviour. I tried to see if Unicornify and EmailValidator have any common modules and although they both use Email::Valid I don't believe that's the root of this problem. I also tried to replicate the issue on codio and my machine and was unable to, even after running the same commands that Travis does to update/install the various dependencies.

FWIW this is failing in Travis for both Perl 5.16.3 (what we have on our dev machines) and 5.18.2 so I'm really confused as to what the problem is and why I can't recreate it.

Anyways if this works I guess we should go ahead with it, but I'd love to know what the problem really is.

@mintsoft
Copy link
Collaborator

@moollaza @nilnilnil This could be changes that were brought in about explicit/lexical $_ in 5.18 (http://perldoc.perl.org/perl5180delta.html#Lexical-%24_-is-now-experimental). However it looks like the load_class($_); probably isn't changing $_ in the scope of this function, I know I've had similar/totally-different issues with attempting to effectively pass $_ by reference and change it in functions that works when iterators are made explicit.

Definitely weird!

@nilnilnil
Copy link
Contributor

I don't have much time to look at this, but I think looking into Module::Runtime::check_module_name might lead to some more answers.

@jagtalon
Copy link
Member Author

@nilnilnil Thanks for that--will take a look at it. @moollaza @mintsoft Is it failing on your machines?

@moollaza
Copy link
Member

@jagtalon nope. I can't recreate the problem on my dev machine or on codio. If it fixes the problem though we should merge this in so travis can get back to a passing state.

@jagtalon
Copy link
Member Author

@moollaza Yeah, let's do that first. Thanks!

@jagtalon
Copy link
Member Author

Alright, merging this now. I'll investigate a bit more on why it's actually failing because it might be a more general issue. CC @moollaza @nilnilnil @mintsoft

jagtalon pushed a commit that referenced this pull request May 30, 2014
Test/Block: Fixed an error with load_class.
@jagtalon jagtalon merged commit 5e0ad55 into master May 30, 2014
@jagtalon jagtalon deleted the jag/load branch May 30, 2014 14:27
@moollaza
Copy link
Member

moollaza commented Jun 1, 2014

Interestingly this error popped up for a dev working on the duckpan-vagrant build. It seems this fix resolved the problem he was having, which was $_ being "undefined".

shedd/duckpan-vagrant#18 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants