Skip to content

Bug 1446236 - Add & use simpler method to check if an extension is present #35

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 8, 2018
Merged

Bug 1446236 - Add & use simpler method to check if an extension is present #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba55018
Bugzilla: Add have_extension
CyberShadow Mar 31, 2018
537dfba
Use Bugzilla->have_extension
CyberShadow Mar 31, 2018
352401c
Bugzilla::extensions: Forbid recursive invocations
CyberShadow Apr 3, 2018
df7fde8
Bugzilla::have_extension: Use shorter code
CyberShadow Apr 3, 2018
9e3deeb
Rename Bugzilla::have_extension to has_extension
CyberShadow Apr 3, 2018
4deaf55
Bugzilla::extensions: Add comment explaining the recursion guard
CyberShadow Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Bugzilla.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,15 @@ sub template_inner {

sub extensions {
my ($class) = @_;

# Guard against extensions querying the extension list during initialization
# (through this method or has_extension).
# The extension list is not fully populated at that point,
# so the results would not be meaningful.
state $recursive = 0;
die "Recursive attempt to load/query extensions" if $recursive;
$recursive = 1;

my $cache = $class->request_cache;
if (!$cache->{extensions}) {
my $extension_packages = Bugzilla::Extension->load_all();
Expand All @@ -245,9 +254,20 @@ sub extensions {
}
$cache->{extensions} = \@extensions;
}
$recursive = 0;
Copy link

Choose a reason for hiding this comment

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

I don't understand why you are guarding against a recursive call. That at least needs a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.

Copy link

Choose a reason for hiding this comment

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

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 $cache->{extensions};
}

sub has_extension {
my ($class, $name) = @_;
my $cache = $class->request_cache;
if (!$cache->{extensions_hash}) {
my %extensions = map { $_->NAME => 1 } @{ Bugzilla->extensions };
$cache->{extensions_hash} = \%extensions;
}
return exists $cache->{extensions_hash}{$name};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured out the problem, it was caused by a change in another pull request of mine.

sub cgi {
return $_[0]->request_cache->{cgi} ||= new Bugzilla::CGI();
}
Expand Down
6 changes: 2 additions & 4 deletions Bugzilla/Search.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use Date::Format;
use Date::Parse;
use Scalar::Util qw(blessed);
use List::MoreUtils qw(all firstidx part uniq);
use List::Util qw(any);
use POSIX qw(INT_MAX);
use Storable qw(dclone);
use Time::HiRes qw(gettimeofday tv_interval);
Expand Down Expand Up @@ -803,8 +802,7 @@ sub data {
# BMO - to avoid massive amounts of joins, if we're selecting a lot of
# tracking flags, replace them with placeholders. the values will be
# retrieved later and injected into the result.
state $have_tracking_flags = any { $_->NAME eq 'TrackingFlags' } @{ Bugzilla->extensions };
if ($have_tracking_flags) {
if (Bugzilla->has_extension('TrackingFlags')) {
my %tf_map = map { $_ => 1 } Bugzilla::Extension::TrackingFlags::Flag->get_all_names();
my @tf_selected = grep { exists $tf_map{$_} } @orig_fields;
# mysql has a limit of 61 joins, and we want to avoid massive amounts of joins
Expand Down Expand Up @@ -867,7 +865,7 @@ sub data {
$self->{data} = [map { $data{$_} } @$bug_ids];

# BMO - get tracking flags values, and insert into result
if ($have_tracking_flags && @{ $self->{tracking_flags} }) {
if (Bugzilla->has_extension('TrackingFlags') && @{ $self->{tracking_flags} }) {
# read values
my $values;
$sql = "
Expand Down
3 changes: 1 addition & 2 deletions extensions/BugModal/Extension.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,7 @@ sub template_before_process {
return if exists $bug->{error};

# trigger loading of tracking flags
state $have_tracking_flags = any { $_->NAME eq 'TrackingFlags' } @{ Bugzilla->extensions };
if ($have_tracking_flags) {
if (Bugzilla->has_extension('TrackingFlags')) {
Bugzilla::Extension::TrackingFlags->template_before_process({
file => 'bug/edit.html.tmpl',
vars => $vars,
Expand Down
3 changes: 1 addition & 2 deletions votes.cgi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ use lib qw(. lib local/lib/perl5);
use Bugzilla;
use Bugzilla::Error;

my $is_enabled = grep { $_->NAME eq 'Voting' } @{ Bugzilla->extensions };
$is_enabled || ThrowCodeError('extension_disabled', { name => 'Voting' });
Bugzilla->has_extension('Voting') || ThrowCodeError('extension_disabled', { name => 'Voting' });

my $cgi = Bugzilla->cgi;
my $action = $cgi->param('action') || 'show_user';
Expand Down