Closed Bug 160557 (bugzillaunderscore) Opened 22 years ago Closed 22 years ago

products that start with _ do not show up properly in query.cgi


(Bugzilla :: Query/Bug List, defect)

Not set



Bugzilla 2.18


(Reporter: pbaker, Assigned: pbaker)




(1 file, 2 obsolete files)

If you have a product name that starts with a underscore (_), the product's
components/versions/milestones do not show up on the query.cgi page when that
product is selected from the listbox.

1. create a product with a name like "_misc".
2. add some components to this product.
3. now go to the query page.
4. select the _misc (or whatever you named) it product in the list.

Actual Results:
the component, version, and target list boxes become empty.

Expected Results:
the component, version, and target list boxes should be populated with the
components, version, and milestones that correspond to the _misc product.

Additional Information:
Looking at the html source, shows in the javascript:

  cpts[0] = [  ];
  vers[0] = [  ];
  tms[0]  = [  ];

Which is why the boxes are empty.

Also I put this as major because it is causing major problems for my company's
installation of bugzilla because we have the product called _customer_issue
which tracks all customer reported problems. We have a component for everyone of
our clients (yes there are a ton of components). It is our largest product, so
trying to search for customer issue bugs without limiting it to the particular
customer we want is a major pain.

We could change the name to just 'customer-issue' I suppose, but we have the _
in front so that it always sorts to the top of all lists. (it's used the most so
it should be at the top shouldn't it.) That also doesn't change the fact that
this is a bug.
Alias: bugzillaunderscore
This is a "feature" of Template Toolkit as noted in the man page for
Template::Manual::Variables under the "Hash Array References" section:

       Any key in a hash which starts with a '_' or '.' character
       will be considered private and cannot be evaluated or
       updated from within a template.  The undefined value will
       be returned for any such variable accessed which the Tem-
       plate Toolkit will silently ignore (unless the DEBUG
       option is enabled).

The question is whether we want to allow users to create products
(and components and ???) with leading underscore characters, and
if so, how do we make Template Toolkit behave like we want it to.
I know how to fix this. I'm about to go into a meeting, but I'll have a patch
ready soon after I get out. Should I reassign this bug to myself then I guess?
This fixes it on my bugzilla installation.
Keywords: review
Keywords: patch
ddk says to assign it to myself since I gave the patch. so here goes...
Assignee: endico → pbaker
Reassigning to patch author.
I don't think that this patch will work. We really don't want to restrict
ourselves to not using hashes, which is where this will end up (since anything
can start with a _).

Maybe we can find a way to disable this feature? TT uses it internally though,
so probably not.

My original thought from many many months ago was to not use TT for the template
engine, but I guess it's a little late for that...</troll>

Unless you can convince TT upstream to some how make that a disableable feature,
I  can't think of any other way to handle this situation.

TT also does teh same thing for names starting with '.', probably because
hash..elem stuffs up the parser. How do other tempalte systems deal with that?
Without getting into to much of a template engine flamewar, the template engine
I suggested while back was HTML::Template, mainly because it does not all you to
do any kind of coding in the templates like TT does. This would have meant that
more dirty work would have to be done in the actual bugzilla code to use H:T
instead of being able to take shortcuts in the templates. It is also more array
based than hash based, so that would be one reason it wouldn't have a _|. key
problem^H^H^H^H^H^H^Hfeature like TT does (see comment#2). But it's a little
late to change template engines so I won't spend any more time on that.

So really don't have much of an answer. Who has the best relationship talking to
the TT maintainers?
In TT can we do something like this:

my @products = ();

foreach my $p (@legal_products) {

  my %p = { product => $p,
            components => \@comps,
            versions => \@vers,
            mstones  => \@mstones, };
  push @products, \%p;


$::vars{'products} = \@products;

Then in the template do something like this:

[% FOREACH p products %]
  cpts = [
   [% FOREACH c p.components ]% yadda yadda... [% END %]
[% END %]

Pardon for the bad template syntax above I don't have the syntax memorized yet.
But do you think that might be a doable solution? If you want I could go ahead
and maybe do a new patch that does like that. Instead of 3 or 4 arrays, it would
be one array of hashes.
This is an alternative patch. Instead of creating 4 arrays (products,
components, versions, milestones) it creates a products array that contains a
hash with values for each thing instead. It's also different from the original
code that created instead 4 hashes. Not to mention it also decreases the number
of lines (even with the 2 lines of comments I added) and the template looks a
bit cleaner as well.

Didn't mark this as obsoleting my original patch, because the both work. It's
just a question of which one you guys choose...or if.
> ... I can't think of any other way to handle this situation.

Preventing Bugzilla products starting with _ comes to mind.
Comment on attachment 93649 [details] [diff] [review]
alternate patch that uses array of hashes

I like this interface change; the names like "componentsbyproduct" were signs
we were doing something wrong anyway. The fact that it fixes the other issue is
a bonus.

>+for (my $i = 0; $i < @products; ++$i) {
>+    my $p = $products[$i];
>+    ## create hash to hold attributes for each product.
>+    my %p = (
>+        name => $p,
>+        components => [ sort { lc($a) cmp lc($b) } @{$::components{$p}} ],
>+        versions   => [ sort { lc($a) cmp lc($b) } @{$::versions{$p}}   ]
>+    );
>+    $p{milestones} =  [ sort { lc($a) cmp lc($b) } @{$::target_milestone{$p}} ]
>+            if Param('usetargetmilestone');
>+    ## assign hash back to product array.
>+    $products[$i] = \%p;
> }

Please use Bugzilla coding style, as outlined in the Developers' Guide on the
website. Specifically, single-hashes precede comments, and you should turn the
"usetargetmilestone" check into a proper if clause, as it doesn't all fit on
one line.

Also, while you are there:
- can we use %product instead of %p? 
- we don't need the temporary $p at all
- please quote hash keys using single quotes ( $p{'milestones'} ).

But this patch is generally good :-)

Attachment #93649 - Flags: review-
Attached patch Patch v.1Splinter Review
Here's a version with the review nits picked. I'm not stealing your patch -
it's just I want to get this in quickly in order to make further modifications
to that area of code. :-)

Attachment #93615 - Attachment is obsolete: true
Attachment #93649 - Attachment is obsolete: true
Comment on attachment 94772 [details] [diff] [review]
Patch v.1

OK, this looks good, and its cleaner, too

r=bbaetz x2

I'd ask you to update the INTERFACE comments, but this template doesn't
actually have one...
Attachment #94772 - Flags: review+

Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.102; previous revision: 1.101
Checking in template/en/default/search/form.html.tmpl;
<--  form.html.tmpl
new revision: 1.11; previous revision: 1.10

Closed: 22 years ago
Resolution: --- → FIXED
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
*** Bug 196896 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.