Closed Bug 254351 Opened 16 years ago Closed 15 years ago

Add a cookiedomain parameter

Categories

(Bugzilla :: Bugzilla-General, enhancement)

2.18
enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: erh+mozilla, Assigned: erh+mozilla)

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.5) Gecko/20031219 Firebird/0.7
Build Identifier: Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.5) Gecko/20031219 Firebird/0.7

I have some changes to add a cookiedomain paramter.  This is similar to the
cookiepath parameter, but lets you set the domain value of a cookie.
Also included is some cleanup of unused cookiepath param lookups.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Version: unspecified → 2.18
Attachment #155214 - Flags: review?(bugreport)
Comment on attachment 155214 [details] [diff] [review]
Patch to add cookiedomain param. 

>diff -udr o/bugzilla-2.18rc2/Bugzilla/CGI.pm bugzilla-2.18rc2/Bugzilla/CGI.pm
>--- o/bugzilla-2.18rc2/Bugzilla/CGI.pm	2004-07-22 02:08:46.000000000 -0500
>+++ bugzilla-2.18rc2/Bugzilla/CGI.pm	2004-08-04 17:29:01.000000000 -0500
>@@ -175,6 +175,10 @@
> 
>     # Add the default path in
>     unshift(@_, '-path' => Param('cookiepath'));
>+	if (defined Param('cookiedomain'))
>+	{
>+    	unshift(@_, '-domain' => Param('cookiedomain'));
>+	}

This doesn't follow the indentation standard in this file. I'm also unsure if
you want the "defined" check there. Maybe just:

  if (Param('cookiedomain')) {
    ...
  }

I'm in principle happy with this change because I've seen it done in client
sites (in an uglier way than this :-)
Attachment #155214 - Flags: review?(bugreport) → review?
Assignee: justdave → erh+mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 2.20
Sorry, I didn't notice you guys don't use tab indentation.

I was thinking about doing it with 2.16, but I'm glad someone else factored out
all the cookie setting code. :-)

You're right, defined should be omitted so an empty but present string doesn't
cause the domain param to be set.
Attachment #155214 - Flags: review?
Tabs-are-evil <wink>. Read the developer's guide at
http://www.bugzilla.org/docs/developer.html for more details.

Assigning to you since you seem to be on it.
Status: NEW → ASSIGNED
We already have problems with people mis-setting cookiepath; is this going to
increase them? Can we auto-set it based on cookiepath unless the admin sets it
explicitly, or something like that?

Gerv
(In reply to comment #5)
> We already have problems with people mis-setting cookiepath; is this going to
> increase them? Can we auto-set it based on cookiepath unless the admin sets it
> explicitly, or something like that?

a) this has nothing to do with cookiepath, it's a separate entity
b) unlike cookiepath, you only set this if you need it, and if you leave it
blank, it behaves like it always used to.
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Same change as previous patch, but with corrected formatting and based on the
most recent code.
Attachment #155214 - Attachment is obsolete: true
Flags: blocking2.20?
Comment on attachment 161969 [details] [diff] [review]
Patch to add cookiedomain param.  Diff based on CVS HEAD as of Oct 13th.

>+    if (defined Param('cookiedomain'))
>+    {
>+        unshift(@_, '-domain' => Param('cookiedomain'));
>+    }

Hadn't we agreed to not use "defined" here as per comment XXX
Attachment #161969 - Flags: review-
Uhm, sorry, hit that button too fast. As per comment 4.

I think this is an awesome change, by the way -- I have it hand-hacked here
locally to set to a specific domain for certain clientes.
Attachment #161969 - Attachment is obsolete: true
um.. ok, so it's assigned to me.  I don't believe I have commit privileges.  So,
now what?

Re. tabs, I certainly don't believe they are evil.  They just have to be used
carefully.  Basically, don't assume that a tab is the same width as anything
other than another tab.  In my own personal code I use tabs to line up the
beginnings of lines, and spaces within them to make something line up within the
line, such as a continuation of the line.  e.g. this fragment:
        if (foo &&
            bar)
would be entered like this (assuming mapping of 4 spaces to a tab) by me:
<tab><tab>if<space>(foo<space>&&
<tab><tab><space><space><space><space>bar)
Comment on attachment 162011 [details] [diff] [review]
Once again, without "defined".

>Index: defparams.pl
blank.	' .
>+           'If you website is at "www.foo.com", setting this 

Typo: if your. 

>+           'Bugzilla cookies.  This is useful if you have more than ' .
>+           'one hostname pointing at the same web server.',

and you want them to share the Bugzilla cookie.

r=kiko with those changes (can be fixed while checking in).

Nice cleanup of cookiepath sillyness. Re. tabs, we're not open to discussing
that. :-)
Attachment #162011 - Flags: review+
Flags: approval?
Summary: Patch to add a cookiedomain paramter → Patch to add a cookiedomain parameter
Some basic documentation about this would be cool to have! :)
Flags: documentation?
holding approval for 2.20 to branch, this can go on the trunk as soon as the
branch happens.
Flags: blocking2.20? → blocking2.20-
OS: NetBSD → All
(In reply to comment #14)
> Some basic documentation about this would be cool to have! :)

No place to *put* documentation as yet; if I'm reading all the flags right, 
this is not going in until 2.22, and the most advanced docs we've got are for 
2.19.1 (soon to be 2.20rc1 when it branches).

Adding a patch here would be cool, but -- like the original enhancement -- 
there's no place to land it just yet.

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Flags: approval? → approval+
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.270; previous revision: 1.269
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.142; previous revision: 1.141
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.95; previous revision: 1.94
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.15; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Summary: Patch to add a cookiedomain parameter → Add a cookiedomain parameter
Possible places to mention this parameter:

http://www.bugzilla.org/docs/tip/html/trbl-relogin-everyone.html
http://www.bugzilla.org/docs/tip/html/parameters.html
QA Contact: mattyt-bugzilla → default-qa
Doc fixed as part of bug 390603.
Flags: documentation?
Flags: documentation3.0+
Flags: documentation+
You need to log in before you can comment on or make changes to this bug.