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

NEW
Unassigned

Status

()

Bugzilla
Bugzilla-General
P4
enhancement
13 years ago
10 years ago

People

(Reporter: Alexis Sukrieh, Unassigned)

Tracking

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 172652 [details] [diff] [review]
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.
(Reporter)

Updated

13 years ago
Blocks: 280179
(Reporter)

Updated

13 years ago
Attachment #172652 - Flags: review?

Comment 2

13 years ago
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.
Attachment #172652 - Flags: review? → review-
The default for the webpath should be empty, too, in order not to break new or
existing installations.
(Reporter)

Updated

13 years ago
Attachment #172652 - Attachment description: Path for providing the webpath token. → Patch for providing the webpath token.
Attachment #172652 - Attachment is obsolete: true
(Reporter)

Comment 4

13 years ago
Created attachment 172663 [details] [diff] [review]
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.
Attachment #172663 - Flags: review?
(Reporter)

Updated

13 years ago
Attachment #172663 - Attachment is obsolete: true
(Reporter)

Comment 5

13 years ago
Created attachment 172664 [details] [diff] [review]
Patch for adding a "webpath" token in templates, with a good default value.

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

Updated

13 years ago
Attachment #172664 - Flags: review?
Attachment #172663 - Flags: review?
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.
Attachment #172664 - Flags: review? → review-

Comment 7

13 years ago
So...  if the code can find the data directory...  why can't this be done as a
reular PARAM value??
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

13 years ago
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?

Updated

13 years ago
Assignee: general → sukria
OS: Linux → All
(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

13 years ago
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.
(Reporter)

Comment 11

13 years ago
Created attachment 172793 [details] [diff] [review]
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.
Attachment #172664 - Attachment is obsolete: true
Attachment #172793 - Flags: review?

Comment 12

13 years ago
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 .
Attachment #172793 - Flags: review? → review-

Comment 13

13 years ago
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
(Reporter)

Comment 14

13 years ago
Created attachment 172795 [details] [diff] [review]
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.
Attachment #172793 - Attachment is obsolete: true
Attachment #172795 - Flags: review?
(Reporter)

Updated

13 years ago
Status: NEW → ASSIGNED
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 :)
Attachment #172795 - Flags: review? → review-
(Reporter)

Updated

13 years ago
Assignee: sukria → wurblzap
Status: ASSIGNED → NEW
Created attachment 173873 [details] [diff] [review]
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.
Attachment #172795 - Attachment is obsolete: true
Attachment #173873 - Flags: review?
how about making the css files templates, and having them built by checksetup,
instead of static?

Comment 18

13 years ago
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. ;)
Attachment #173873 - Flags: review? → review-
Created attachment 183137 [details] [diff] [review]
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...
Attachment #173873 - Attachment is obsolete: true
Attachment #183137 - Flags: review?(LpSolit)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22

Comment 20

13 years ago
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.
Attachment #183137 - Flags: review?(LpSolit) → review-
Created attachment 184675 [details] [diff] [review]
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.
Attachment #183137 - Attachment is obsolete: true
Attachment #184675 - Flags: review?(LpSolit)

Comment 22

13 years ago
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.
Attachment #184675 - Flags: review?(LpSolit) → review-
Created attachment 193549 [details] [diff] [review]
Patch 2.3

Addressed comments.
Attachment #184675 - Attachment is obsolete: true
Attachment #193549 - Flags: review?(LpSolit)

Updated

12 years ago
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

Comment 24

12 years ago
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.
Attachment #193549 - Flags: review?(LpSolit) → review-
Priority: -- → P4

Comment 25

11 years ago
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".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Created attachment 253516 [details] [diff] [review]
Current work in progress

This is where this bug is at.
I'm uploading this in order to...
Attachment #193549 - Attachment is obsolete: true
... 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.
Assignee: wurblzap → general
Status: ASSIGNED → NEW
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/" ?).
Attachment #253516 - Flags: review-

Comment 29

10 years ago
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.
Target Milestone: Bugzilla 3.2 → ---
You need to log in before you can comment on or make changes to this bug.