155388 - Site navigation bar link broken in 2.16

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Toms Baugis]

Site navigation bar links broken in 2.16
Reasons:

1) edit.html.tmpl passes header_html parameter to header template (wich doesn't
have such parameter)

2) it seems to me, that TT doesn't allow to call functions as it is done in
edit.html.tmpl

Comment 1

[Jouni Heikniemi]

Right. I'll take a look at this.

Note: this didn't regress from 2.14, but rather from an pre-templatization
version of 2.16.

Comment 2

[Jouni Heikniemi]

Attachment: v1: Try this

Well, this was apparently broken by templatization. This patch does the
following:

<link> stuff is passed into the html template as a string. A sub ref is not
passed.

navigation_links() in CGI.pl now takes a array ref instead of the colon
separated list. That's much cleaner anyway. Also removed some redundant logic
with the cookie list handling.

Comment 3

[Jouni Heikniemi]

Gerv, can you review this? It's your code mostly, I believe.


[e-mail]: As your statement about header.html.tmpl not having
"header_html", that's not the case. But it's true that the interface comment
doesn't mention the param. I'll patch that separately.

Comment 4

[Gervase Markham [:gerv]]

If you are going to fix this (and it does need doing), then the <link> elements
should be directly in the template, rather than created by the
navigation_links() function. 

$vars->{'buglist'} = \@buglist;

Then, in the template:
<link rel="First" href="show_bug.cgi?id=[% buglist.first %]">
<link rel="Last" href="show_bug.cgi?id=[% buglist.last %]">
...

(This is pseudo-code; it's probably a bit more complicated than this, but not much.

Gerv

Comment 5

[Jouni Heikniemi]

I agree that this is the correct solution, but do you really consider that to be
the minimal impact fix you for 2.16? 

I have no problem doing it, but I think that templatizing this and properly
fixing up the links could be left for bug 155389 which has even more uses for
these elements.

Comment 6

[Dave Miller [:justdave]]

let's do minimum impact for 2.16 and fix it right (in the other bug) for 2.18.

trunk has officially departed from the 2.16 branch, so we need to wrap up 2.16
before we confuse ourselves. :-)

Comment 7

[Jouni Heikniemi]

Gerv?

Comment 8

[Gervase Markham [:gerv]]

If we want a minimum-impact fix, we shouldn't be doing this:

"navigation_links() in CGI.pl now takes a array ref instead of the colon
separated list. That's much cleaner anyway. Also removed some redundant logic
with the cookie list handling."

The minimum-impact fix looks like this to me:

+    # Create the <link> elements for browsing bug lists
+    $vars->{'navigation_links'} = navigation_links();

Gerv

Comment 9

[Jouni Heikniemi]

Attachment: v2: KISS

You're right. Here is a simplified version. How do you feel about this, Gerv?

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 90278 [details] [diff] [review]
v2: KISS

r=gerv. Seems correct to me.

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 90278 [details] [diff] [review]
v2: KISS

looks fine to me

r=bbaetz

Comment 12

[Jouni Heikniemi]

Checked into trunk and branch:

--
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.96; previous revision: 1.95
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.12; previous revision: 1.11
done

Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.93.2.1; previous revision: 1.93
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.7.2.4; previous revision: 1.7.2.3
done

Comment 13

[Gervase Markham [:gerv]]

Jouni: can you open a 2.18 bug for the full fix?

Gerv

Comment 14

[Jouni Heikniemi]

Bug 155389 is precisely that thing. :-)