Bug 1446236 - Add & use simpler method to check if an extension is present #35
Conversation
Add a new method to check if an extension is present and loaded.
Transition from previous explicit searches in the extension list to invoking the method added in the previous commit.
| } | ||
| return exists $cache->{extensions_hash}{$name}; | ||
| } | ||
|
|
There was a problem hiding this comment.
There is something wrong with this implementation, but I'm not sure yet what. After the hash is built, it contains only two elements on successive requests.
There was a problem hiding this comment.
That suggests to me there should be a test to capture correctness in extensions, which I gather this change would fail (and prompt an investigation of why).
There was a problem hiding this comment.
I figured out the problem, it was caused by a change in another pull request of mine.
Bugzilla.pm
Outdated
| @@ -248,6 +248,19 @@ sub extensions { | |||
| return $cache->{extensions}; | |||
| } | |||
|
|
|||
| sub have_extension { | |||
There was a problem hiding this comment.
I think has_extension() is a better name, as it maps to has_feature(). Not sure if it's grammatically correct or not though.
Bugzilla.pm
Outdated
| my ($class, $name) = @_; | ||
| my $cache = $class->request_cache; | ||
| if (!$cache->{extensions_hash}) { | ||
| my %extensions; |
There was a problem hiding this comment.
my %extensions = map { $_->NAME => 1 } @{ Bugzilla->extensions };
There was a problem hiding this comment.
Same problem with that code... but it is shorter.
Also using () does something really weird here. I thought it evaluates to undef, which should be slightly more efficient than using 1 or any other value.
There was a problem hiding this comment.
Found the problem: Bugzilla->extensions was being invoked recursively (as it initializes extensions as it populates the list), so the truncated list I was seeing was from the incomplete invocation higher in the stack trace.
There is no simple and obvious way to allow extensions to query the extension list as it is being constructed, so explicitly forbid this behavior. The trap will help avoid weird bugs caused by halfway-populated and cached extension lists.
Suggested-by: Dylan William Hardison <[email protected]>
Suggested-by: Dylan William Hardison <[email protected]>
|
Will update the other PRs to rename |
| @@ -245,9 +250,20 @@ sub extensions { | |||
| } | |||
| $cache->{extensions} = \@extensions; | |||
| } | |||
| $recursive = 0; | |||
There was a problem hiding this comment.
I don't understand why you are guarding against a recursive call. That at least needs a comment.
There was a problem hiding this comment.
There's some explanation in the commit message. Essentially it's an extension checking Bugzilla->extensions during its initialization, which is invalid as the extension list is still being constructed.
There was a problem hiding this comment.
Great! Looking at it now, my gut says it might not work completely right. I'd have thought it was more idiomatic to use not a state var, but a my var outside the sub, and use local to set the guard variable straight after the check, in order to use dynamic scoping rather than lexical.
| } | ||
| return exists $cache->{extensions_hash}{$name}; | ||
| } | ||
|
|
There was a problem hiding this comment.
That suggests to me there should be a test to capture correctness in extensions, which I gather this change would fail (and prompt an investigation of why).
Ideally these should probably all be refactored into extension-agnostic hooks, but for the time being simplify the code a little.
This adds
extensions_hashas a separate variable torequest_cache. An alternative would be to change$cache->{extensions}to a hash, however as hashes are unsorted, I was worried that it could introduce unwanted effects due to the iteration order being changed.