280180 - Templates should provide a [% webpath %] token for a non standalone Bugzilla websites.

Status: RESOLVED DUPLICATE of bug 1497077

(Bugzilla :: Bugzilla-General, enhancement, P4)

Description

[Alexis Sukrieh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050123 Firefox/1.0 (Ubuntu) (Ubuntu package 1.0+dfsg.1-2ubuntu3)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050123 Firefox/1.0 (Ubuntu) (Ubuntu package 1.0+dfsg.1-2ubuntu3)

It would be better to add a [% webpath %] token wherever there is a link to the
installation path in templates.

For instance, 

link href="css/global.css" rel="stylesheet" type="text/css" 

could be:

link href="[% webpath %]css/global.css" rel="stylesheet" type="text/css"

To provide the same behaviour as before, webpath should be initialized to '/' by
default.

For the Debian needs, we would just have to set webpath to '/bugzilla/'.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Alexis Sukrieh]

Attachment: Patch for providing the webpath token.

Applying that patch to the toplevel source tree would provide a new Config
variable: $WEBPATH which would be used wherever the template token [% webpath
%] is used.

Comment 2

[Joel Peshkin]

Comment on attachment 172652 [details] [diff] [review]
Patch for providing the webpath token.

>diff -rubB Bugzilla/Config.pm ../../bugzilla-2.18/Bugzilla/Config.pm
>--- Bugzilla/Config.pm	2005-01-15 05:43:12.000000000 +0100
>+++ Bugzilla/Config.pm	2005-01-25 14:19:47.000000000 +0100
>@@ -81,6 +82,8 @@
> use Safe;
> 
> use vars qw(@param_list);
>+# Defaut webpath
>+our $WEBPATH = "/bugzilla/";
> 

This is not where parameter definitions go.  See defparams.pl
Also, please check for spelling - even in comments.

> # Data::Dumper is required as needed, below. The problem is that then when
> # the code locally sets $Data::Dumper::Foo, this triggers 'used only once'
>diff -rubB Bugzilla/Template.pm ../../bugzilla-2.18/Bugzilla/Template.pm
>--- Bugzilla/Template.pm	2004-07-10 16:51:23.000000000 +0200
>+++ Bugzilla/Template.pm	2005-01-22 11:33:09.000000000 +0100
>@@ -379,7 +379,11 @@
> 
>             # Bugzilla version
>             # This could be made a ref, or even a CONSTANT with TT2.08
>-            'VERSION' => $Bugzilla::Config::VERSION ,
>+            'VERSION' => $Bugzilla::Config::VERSION,
>+
>+	    # This is useful to enhance the way Bugzilla can be used 
>+	    # with a split of CGI and other files.
>+	    'webpath' => $Bugzilla::Config::WEBPATH,
>         },
> 

You don't need this.  The Param() function is exported to the templates
already.

In general, please explain the distinction between URLBASE and WEBPATH. (in the
bug)  It is not clear what you are trying to do.

Comment 3

[Marc Schumann [:Wurblzap]]

The default for the webpath should be empty, too, in order not to break new or
existing installations.

Comment 4

[Alexis Sukrieh]

Attachment: Patch for adding a "webpath" token in templates, with a good default value.

This patch will add a [% webpath %] entry to templates, which is empty by
default, to provide the same behaviour as before.

So that patch won't break anything but will allow to put CGI files in a
specific location and static files in another.

Comment 5

[Alexis Sukrieh]

Attachment: Patch for adding a "webpath" token in templates, with a good default value.

This is the same patch as before but with better comments.

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 172664 [details] [diff] [review]
Patch for adding a "webpath" token in templates, with a good default value.

Unfortunately, I need to turn this down again :(
It's very close to r=wurblzap, though.

The patch doesn't apply to HEAD. (It applies to 2.18, but this will probably
not get approval2.18 on the grounds that it touches templates.)

This doesn't pass runtests.pl -- [% webpath %] isn't filtered. I don't think it
needs to be filtered, so instead of having [% webpath FILTER html %] I'd say
let's put appropriate entries into filterexceptions.pl.

>diff -ubBr Bugzilla/Config.pm ../../bugzilla-2.18/Bugzilla/Config.pm
>+# This is for Debian compat, in order to be able to use [% webpath %] in templates.

Nit: please put that into two lines, keeping line length below 80.

>+our $WEBPATH = "";

Question: why don't you move that a little bit further up in the file to
$libpath, $datadir and friends, calling it $webpath instead of $WEBPATH? I
agree it contains part of an URI and not part of a file system location, but I
think it is strongly related to them, or am I wrong? Maybe separate it from
them by an empty line?

>diff -ubBr Bugzilla/Template.pm ../../bugzilla-2.18/Bugzilla/Template.pm
>+	    # This is for Debian compat, in order to be able to use [% webpath %] in templates.

Nit: same as above: please put that into two lines.

Comment 7

[Joel Peshkin]

So...  if the code can find the data directory...  why can't this be done as a
reular PARAM value??

Comment 8

[Vlad Dascalu]

If some webdata is going to be in URLBASE, and some webdata (.cgi) is going to
be in another place, let's not call that place "webdata". Maybe CGIBASE? or
something similar?

Comment 9

[Marc Schumann [:Wurblzap]]

(In reply to comment #8)
> If some webdata is going to be in URLBASE, and some webdata (.cgi) is going to
> be in another place, let's not call that place "webdata". Maybe CGIBASE? or
> something similar?

The variable WEBPATH he's introducing affects .css files, .js files and
ant.jpg... And it's not necessarily a base, as it may be a relative path (such
as ''). So it'd rather be STATICFILEPATH?

Btw, since the path to ant.jpg is specified in a .css file since 2.19 (?), it
won't need to be moved as long as the skins/ directory moves as a whole.

(In reply to comment #7)
> So...  if the code can find the data directory...  why can't this be done as a
> reular PARAM value??

Because at this stage, we imo don't want people to fiddle with it :)
Let's start doing this not sooner than having moved the affected files out of
the bugzilla directory.

Comment 10

[Max Kanat-Alexander]

It does need to be filtered -- it may sometimes need to be urlencoded to be a
valid URL, or htmlencoded so that it doesn't break the surrounding HTML.

It doesn't need to be encoded for security reasons, just for technical reasons.

Comment 11

[Alexis Sukrieh]

Attachment: Added a [% staticpath %] token instead of the [% webpath %] one.

This patch tries to follow all the comments: 
 - the token is named "staticpath" instead of webpath.
 - every lines are less than 80 characters long.
 - the $staticpath variable is just under $datadir and friends.
 - the $staticpath is written in lower case.

Comment 12

[Vlad Dascalu]

Comment on attachment 172793 [details] [diff] [review]
Added a [% staticpath %] token instead of the [% webpath %] one.

See comment #10 - this needs filtering.

We have a script called runtests.pl that evaluates the current code. It's kind
of a testing suite. With your patch applied, the code breaks due to lack of
filtering.

You can run it yourself (in the Bugzilla directory). It has 9 tests, and a mode
where it displays verbose output.

For example:

./runtests.pl
./runtests.pl 8
./runtests.pl --verbose 8

(the last 2 examples run only the test number 8)

Filtering is used in templates in order to properly quote some elements.
Instead of "[% staticpath %]" you should use "[% staticpath FILTER filtername
%]", where filtername is the filter you want to use.

In comment #10 mkanat suggested urlencoded (htmlencoded might be overkill). For
a sample of the current filters you can take a look at Bugzilla/Template.pm.

Needless to say, any patch CVS commited needs to pass runtests.pl. If you
really need something unfiltered, it must be added as an exception to
template/en/default/filterexception.pl .

Comment 13

[Vlad Dascalu]

By the way, I forgot: the output that I got from running runtests.pl is:

#     Failed test (t/008filter.t at line 131)
not ok 4 - (en/default) template\en\default\sidebar.xul.tmpl has unfiltered dire
ctives:
#   38: staticpath
#   39: staticpath
# --ERROR

Comment 14

[Alexis Sukrieh]

Attachment: static path is now filtered "url_quote", test #8 passed.

All [% staticpath %] token are replaced by [% staticpath FILTER url_quote %]

The test #8 now runs well.

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 172795 [details] [diff] [review]
static path is now filtered "url_quote", test #8 passed.

This is still a 2.18 patch :)  Please make your patch not against 2.18, but
against a current HEAD revision (see http://www.bugzilla.org/download/#cvs).
This is important for the person who is going to check your patch into CVS.

Please make sure to have runtests.pl test what you've written before submitting
a patch. This one has test 5 complain, meaning that there are tabstops in your
code :)

We're getting there, though :)

Comment 16

[Marc Schumann [:Wurblzap]]

Attachment: Head patch

Because Bugzilla uses static CSS files, the references to other static files
from there (most notably images) do not get the [% staticpath %] prefix. The
same goes for the static Quicksearch pages and the static duplicates.xul -- the
references from there don't get the prefix.

Comment 17

[:glob ✱]

how about making the css files templates, and having them built by checksetup,
instead of static?

Comment 18

[Frédéric Buclin]

Comment on attachment 173873 [details] [diff] [review]
Head patch

What a surprise, this patch is bitrotten. Marc, unbitrot it and ask me to
review it. Else it will remain in the queue for another 3 months. ;)

Comment 19

[Marc Schumann [:Wurblzap]]

Attachment: Unrotted head patch

Thanks for taking the review, Fred.

(In reply to comment #17)
> how about making the css files templates, and having them built by
checksetup,
> instead of static?

I think there is even a bug for this already somewhere...

Comment 20

[Frédéric Buclin]

Comment on attachment 183137 [details] [diff] [review]
Unrotted head patch


>+# This is for Debian compat: Debian needs to be able to use [% staticpath %]
>+# in templates.
>+our $staticpath = '';

Nit: Maybe should we add a comment here saying that quicksearch.html,
quicksearchhack.html and duplicates.xul need to be modified by hand as they are
static files.


Moreover, you missed <link href="skins/standard/buglist.css" rel="stylesheet"
type="text/css"> in template/en/default/list/list-simple.html.tmpl at line 45.

Else the patch looks good.

Comment 21

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.2

(In reply to comment #20)
> Nit: Maybe should we add a comment here saying that quicksearch.html,
> quicksearchhack.html and duplicates.xul need to be modified by hand as they
> are static files.

Changed comment considerably.

> Moreover, you missed <link href="skins/standard/buglist.css" rel="stylesheet"

> type="text/css"> in template/en/default/list/list-simple.html.tmpl at line
45.

Right, it's included now. I added a line for the skins/custom directory as
well.

Comment 22

[Frédéric Buclin]

Comment on attachment 184675 [details] [diff] [review]
Patch 2.2

>--- head/Bugzilla/Config.pm	2005-05-12 22:22:49.077185600 +0200
>+++ patched/Bugzilla/Config.pm	2005-05-27 14:22:58.180539000 +0200

>+our $staticpath = '';

What I told you on IRC on Friday was correct: this parameter has to be exported
in the 'locations' tag (%Bugzilla::Config::EXPORT_TAGS) in order to be used by
checksetup.pl when creating the .htaccess files. checksetup.pl already calls
Config.pm, see line 444:

use Bugzilla::Config qw(:DEFAULT :admin :locations);

In checksetup.pl, you also have to convert all paths to the new path
'$staticpath . " skins"' instead of "skins". Ditto for the .js files.

Comment 23

[Marc Schumann [:Wurblzap]]

Attachment: Patch 2.3

Addressed comments.

Comment 24

[Frédéric Buclin]

Comment on attachment 193549 [details] [diff] [review]
Patch 2.3

Bitrot in Config.pm. Do we still want this bug? There has been no special request to get it reviewed earlier.

Comment 25

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 26

[Marc Schumann [:Wurblzap]]

Attachment: Current work in progress

This is where this bug is at.
I'm uploading this in order to...

Comment 27

[Marc Schumann [:Wurblzap]]

... drop the ball on this bug. Maybe if one or two Debian folks pick it up, there may be enough momentum to find a reviewer to pull this through.

Comment 28

[Dave Miller [:justdave]]

Comment on attachment 253516 [details] [diff] [review]
Current work in progress

ok, one thing I notice on here...  the path used in the URLs isn't necessarily going to match the path used on the filesystem.  We can't use the same replacement on both the file permissions in checksetup.pl and in the web pages.  They won't necessarily match (and on both Debian and Fedora they probably won't).  Since the static files will most likely be in a different directory from the location of checksetup.pl, the distros are probably going to need a full qualified path ("/usr/share/bugzilla/html" ?) for setting the file permissions, and a docroot-relative one to use in the templates ("/bugzilla/" ?).

Comment 29

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.

Comment 30

[Kohei Yoshino [:kohei]]

We have added basepath and conveted all the paths and redirects to use it.