Modernize the Developers' Guide

RESOLVED FIXED

Status

()

Bugzilla
bugzilla.org
P1
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
Right now we have various development standards that aren't actually listed in
the Developers' Guide. For example, the formatting of SQL queries, or the
standards around .pm files. There's a lot of stuff. In general, the Developers'
Guide could use a bit of overhaul.
(Assignee)

Updated

13 years ago
Priority: -- → P1
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

12 years ago
Created attachment 196158 [details] [diff] [review]
Work-In-Progress

This is a massive undertaking. You can see how far I've gotten on it, up to
this point. One thing I will have to go back and add is the standards about .pm
files, but so far I've cleaned up a lot of things and made them actually
current with reality. :-)

You can see this version (or however far I get after this) at:

http://landfill.bugzilla.org/bugzilla.org/docs/developer.html
(Assignee)

Updated

12 years ago
(Assignee)

Comment 2

12 years ago
Created attachment 196305 [details] [diff] [review]
Developer's Guide: Modernized

OK, here's a modernized Developer's Guide. Probably the only thing it's missing
is some guidelines on how to write .pm files, but we can add those in the
future.

Here's what I changed/updated:

+ The Table of Contents at the top is now much more complete.
+ I made some of the wording simpler here and there.
+ I added a series of "guiding principles" near the top of the Guide.
+ The "Perl Code" section was worked over to be slightly clearer, and I updated

  it in a few places.
+ I fixed a few slight errors in the "API Documentation" section, although I  
  left a lot of it unchanged.
+ The "SQL" section was basically entirely re-written, with many new parts
  added, with a lot of simple explanations.
+ The "Templates" section I pretty much left alone, except I removed
  a reference to CGI.pl.
+ I added a few lines to the Security section.

As always, you can see this version live at:

http://landfill.bugzilla.org/bugzilla.org/docs/developer.html
Attachment #196158 - Attachment is obsolete: true
Attachment #196305 - Flags: review?(justdave)
Comment on attachment 196305 [details] [diff] [review]
Developer's Guide: Modernized

I think I like it. Few comments came to mind when I read the new Guide:

>Index: docs/developer.html.tmpl
>===================================================================
>-[%# Developers' Guide %]
>+[%# Developer's Guide %]
..
> <h1>Developers' Guide</h1>

Maybe you should change the title here too?

>+   The test suite is used by our
>+   <a href="http://tinderbox.mozilla.org/showbuilds.cgi?tree=Bugzilla">Tinderbox</a>,
>+   to warn us of bad code, so anything that fails to pass the testing suite
>+   will be backed out immediately.</p>

Maybe make this two separate sentences, looks awfully long to read.

>   <li>The testing suite will check all Perl files have no errors. This is
>       done by using the -c command line switch.</li>

You didn't change this but I paused for a moment to think about what command
this "-c command line switch" is referring to. It's probably perl but why not
make it more obvious?

>+  <li><strong>Variable Names</strong> - If a variable is scoped globally 
>+       (<code class="perl">$::variable</code> - we really shouldn't have any 
>+       of these, don't add any new ones) its name should be

Make this "no globals, please" more prominent by adding new list bullet for it?

>+    <p>You can find the list of various errors in the templates 
>+       <code>global/user-error</code> and <code>global/code-error</code>.</p>

Hmm.. I wonder if these file references should point to actual filenames so
people not yet familiar with template directory structures will be able to find
them more easily?

>+=head1 DESCRIPTION
>+
> FlagType.pm provides an interface to flag types as stored in Bugzilla.
> See below for more information.

Command =over is missing from this example. It's needed before any =item
commands.

>+  <li>If a feature isn't ANSI-standard, but is supported by all databases
>+      we support, you can go ahead and use it.</li>

What databases are supported should be documented somewhere..

>+  <li>Regular expression searches done by the database (with 
>+      $dbh-&gt;sql_regexp) use a subset of the regexp language of Perl,
>+      and not all features that work in Perl will work with the database
>+      regexps. When in doubt, keep it simple.</li>

Wasn't there a change to move away from DB regexps and use perl regexps only?

> <p>The current recommended method for creating a query is as follows:</p>
> 
> <pre>
>   use Bugzilla;
>+  my $dbh = Bugzilla-&gt;dbh; # Connects if not already connected.
>+                              # Also handles db, user, password...
>+  my $sth = $dbh-&gt;prepare(&quot;SELECT foo, bar FROM bath WHERE log = ?&quot;);
>+  my $sth-&gt;execute(&quot;foobar&quot;);
> 
>   # Gathering rows of data
>+  while ($result=$sth-&gt;fetchrow_array()) {
>         # do whatever with $result
>   }
> </pre>

Really? I would much rather use the selectall_arrayref types of functions for
simple selects and only rarely this complete prepare/execute/fetch construct.

>+    <p><code>my $template = Bugzilla-&quot;template;<br />

Probably you didn't want to use &quot; here..

>+      4.01</a> Strict.  You can test the output of a Bugzilla page by using 

Hmm.. It's Strict now? Is there a bug open to change DOCTYPE accordingly? I
wonder what HTML constructs become illegal after this change. Maybe this just
means to use CSS everywhere but I keep remembering that something else is not
allowed after this change.

>-<p>Bugzilla has various facilities to restrict products and bugs to  users in
>-   certain groups. When users can bypass this mechanism, it is a security
>-   hole.</p>
>+<p>Bugzilla has various facilities to restrict products and bugs to users in
>+   When users can bypass this mechanism, it is a security hole.</p>

Oops, "certain groups." seems to be missing from here..

>-  <li>Passing untrusted strings to SQL. Use <code class="perl">SqlQuote</code> to escape and detaint the string.</li>
>+  <li>Passing untrusted strings to SQL. Use placeholders instead of passing
>+      strings directly into SQL.</li>

Shouldn't this mention to use trick_taint or otherwise detaint the variable?
Otherwise this will crash and burn.

Comment 4

12 years ago
Comment on attachment 196305 [details] [diff] [review]
Developer's Guide: Modernized

Max - Great job on the updates.  I wouldn't have whacked as much as you did
(placeholders for example), but I see you added a lot more than you cut.

I'm not sure, but does IE support &quot; correctly?  Last time I tried it, IE
displayed it literally rather than as a quote.

I noticed that you suggest using SqlQuote to escape and detaint strings,
however, I also noticed earlier that you said use the DBI methods.  I think it
would be wise to clarify what you mean there so it doesn't appear to set up a
conflict in how to write SQL-based code.

Comment 5

12 years ago
Sorry all - I retract my earlier comments - I was looking at the wrong pane as
"updated" while looking at the diffs.  More comments to follow...
(Assignee)

Comment 6

12 years ago
Created attachment 196379 [details] [diff] [review]
v2

Oops. :-) The one I posted was *slightly* out-of-date I noticed, from some
above comments. Here's one with a few fixes.
Attachment #196305 - Attachment is obsolete: true
Attachment #196379 - Flags: review?(justdave)
(Assignee)

Updated

12 years ago
Attachment #196305 - Flags: review?(justdave)
IE doesn't support &quot; :-( Use the numeric version (&#39;, I think). 

This looks great - nice work, Max.

Gerv
> IE doesn't support &quot; :-( Use the numeric version (&#39;, I think). 

there's a lot of entities that ie doesn't support, but ie 6 definately supports
&quot; and i'm pretty certian ie 5 also has support for it.

either way, &#34; is "

Comment 9

12 years ago
Just reminder: Don't forget to add vimrc autocommand for template editing :)
See bug 297891, comment #4
Comment on attachment 196379 [details] [diff] [review]
v2

@@ -294,250 +439,545 @@
	+ <p>Function descriptions should generally follow the format of:<</p>

in <h2><a name="sql-general"></a>General</h2> section (and some other place)
some period(.) are inside of quote.


<h2><a name="sql-style"></a>Style</h2>
inside of 'pre' element, 4 '(' and 5 ')'


@@ -593,18 +1033,13 @@
...
<br />


+<h2><a name="security-confidential"></a>Confidential Information Leakage</h2>
+<p>Bugzilla has various facilities to restrict products and bugs to users in
+   When users can bypass this mechanism, it is a security hole.</p>

some words mis-deleted?

Comment 11

12 years ago
Comment on attachment 196379 [details] [diff] [review]
v2

Please remove all occurences of SqlQuote (and SendSQL if there are some). Also,
avoid writing SELECT * FROM ... as we don't allow to select columns in a random
order. You should maybe write SELECT foo1, foo2 FROM ... .
(Assignee)

Comment 12

12 years ago
(In reply to comment #11)
> (From update of attachment 196379 [details] [diff] [review] [edit])
> Please remove all occurences of SqlQuote (and SendSQL if there are some). Also,
> avoid writing SELECT * FROM ... as we don't allow to select columns in a random
> order. You should maybe write SELECT foo1, foo2 FROM ... .

  I'm nearly certain I did remove all instances of SqlQuote. If you're looking
at the current landfill version, it's not the patched version.

  I only use SELECT * in the examples where I'm explaining JOIN statements, and
there's an explicit statement later that SELECT * should not be used in Bugzilla
code.
Comment on attachment 196379 [details] [diff] [review]
v2

So close...  lots of little nits :)  Content is mostly good, I'm basically
picking on formatting and typos.

>-	title = "Developers' Guide" 
>+	title = "Developer's Guide" 

You got the title...

> <h1>Developers' Guide</h1>

But you didn't get the H1.

I've never known which is the right way to do this.  Is this a guide that
belongs to all the developers or for an individual developer?  Guess it works
either way, I'm not picky ;)  We just need to be consistent and do it the same
way both places.


>+    <p>If your variable is actually <em>supposed</em> to contain a regular
>+       expression, then you don't have to escape it. It is a good idea to 
>+       include &quot;regexp&quot; in the variable name, for readability purposes.</p>

Could probably make this a little more clear that the variable name should only
contain "regexp" in the name if the variable is supposed to contain a regexp. 
Although it's the same paragraph, the sentence starting "It is a good idea" if
taken by itself might not connect.  It should, but we all know how well some
people pay attention. ;)   Maybe "In this case, it is a good idea..." then when
they read it they instantly know to read the rest of the paragraph to find out
which case that is.

>+    <p><strong>Errors</strong>

There's been a lot of question on this in the past, and we argue about it from
time to time, so we probably need to really clarify this.

There's a couple general rules of thumb that come into play here: 

1) Don't blame the user if there's doubt whether it's the user's fault.
2) Don't tell the user to email the admin if the error doesn't mean Bugzilla is
broken.

#1 tells us to use ThrowCodeError when in doubt, by the symantics of the Throw*
routine names, however, ThrowCodeError tells the user to email the admin.  What
we really need to do is ThrowCodeError if Bugzilla isn't broken even if we
don't know if it's the user's fault, but make sure the error message put in the
red box doesn't blame the user in that case.  The difference between the two
routines isn't really about whose fault it is, but whether the error is worthy
of emailing the admin about.

>@@ -136,50 +273,30 @@
>       <li><code class="perl">trick_taint</code> will detaint a piece of data

There's a few additional detaint_foo() routines now besides detaint_natural and
trick_taint(), aren't there?

>   # Gathering rows of data
>-  while ($result=$sth->fetchrow_array()) {
>+  while ($result=$sth-&gt;fetchrow_array()) {
>         # do whatever with $result
>   }

fetchrow_array() returns an array, right?  Should probably be 

  while (my @result = $sth->fetchrow_array()) {

or something like that.

>@@ -593,18 +1033,13 @@
>+    <p><code>my $template = Bugzilla-&quot;template;<br />

s/&quot;/&gt;/

>-<h2>Confidential Information Leakage</h2>
>-<p>Bugzilla has various facilities to restrict products and bugs to  users in
>-   certain groups. When users can bypass this mechanism, it is a security
>-   hole.</p>
>+<h2><a name="security-confidential"></a>Confidential Information Leakage</h2>
>+<p>Bugzilla has various facilities to restrict products and bugs to users in
>+   When users can bypass this mechanism, it is a security hole.</p>

We lost part of the sentence here in the reformatting...  missing "certain
groups"
Attachment #196379 - Flags: review?(justdave) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 200147 [details] [diff] [review]
v3

OK, I've integrated wicked's, LpSolit's, and justdave's comments into this new
version. All very good comments. :-)
Attachment #196379 - Attachment is obsolete: true
Attachment #200147 - Flags: review?(justdave)
(Assignee)

Comment 15

12 years ago
Oh, and it's actually live at the URL in the "URL" field, now, so you can see
what it really looks like if you'd care to. (It was down for a while while I was
doing the 2.20 release updates.)

Comment 16

12 years ago
Comment on attachment 200147 [details] [diff] [review]
v3

>+SELECT DISTINCT groups.id, groups.name, groups.description
>+           FROM groups 
>+                CROSS JOIN user_group_map
>+                CROSS JOIN group_group_map AS ggm

Nit: This example under Style will not work with Oracle. Oracle does not allow
the 'AS' keyword before the table (bug 312349)
Comment on attachment 200147 [details] [diff] [review]
v3

>+<p>Note that in the below examples we use <code>SELECT *</code> for
>+   simplicity, but <a href="#sql-general"><code>SELECT *</code>should
>+   never be used in Bugzilla code</a>.</p>

Missing a space after </code>

Comment 18

12 years ago
Comment on attachment 200147 [details] [diff] [review]
v3

>+<h1><a name="perl"></a>Perl Code</h1>

>+  <li>CGIs should call <code class="perl"><a href="http://www.bugzilla.org/docs/tip/pod/Bugzilla.pm#methods">Bugzilla-&gt;login</a></code>
>+      before determining user identity, as this will perform the 
>+      proper verifications against impersonation.</li>

1) You should also mention that you should use Bugzilla->dbh to access the DB instead of calling Bugzilla::DB yourself. Ditto for Bugzilla->cgi instead of calling Bugzilla::CGI yourself. In other words, you should never have to write 'use Bugzilla::CGI;' or 'use Bugzilla::DB;' in your code. I think you can include all Bugzilla::Auth::* modules as well.

You could give the following example:

my $user = Bugzilla->login(LOGIN_FOO); # when the current user is called for the first time;
                                       # use it instead of calling Bugzilla::Auth yourself.
my $user = Bugzilla->user;             # for all subsequent calls to the current user.

my $cgi = Bugzilla->cgi;               # instead of calling Bugzilla::CGI yourself.
my $dbh = Bugzilla->dbh;               # instead of calling Bugzilla::DB yourself.


2) You should mention that all 'my $foo = Bugzilla->foo' should be redeclared in each subroutine which use them; for instance to be compatible with mod_perl and in case we move the subroutine in another file which doesn't define them globally.



>+<h2><a name="perl-errors">Throwing Errors</a></h2>

3) You should mention that Throw*Error() automatically unlock tables and stop the execution of the script. There are still a few guys who write:

$dbh->unlock_tables();
ThrowUserError('foo');
exit;



>+<h2><a name="perl-style"></a>Style</h2>

4) When the test is splitted on several lines, you should mention that the bracket should be on its own line:

if (($my_very_long_variable_name ne 'what is the wheather today')
    || defined($my_another_very_long_variable_name))
{
    ...
}

Maybe also a comment about where to write && or ||. Some developers want them at the end of the lines, some others at the beginning of the next lines.



>+<h3>CROSS JOIN</h3>

5) From what has been discussed on IRC very recently, we should ban CROSS JOIN in favor of INNER JOIN.



>+<p><code>SELECT * FROM a, b</code> or <code>SELECT * FROM a CROSS JOIN b</code></p>

6) could you move 'or' on its own line? Maybe is it due to the font I use in my browser, but I first thought that the whole line was a single SQL query, instead of two distinct ones.



>+<h2><a name="sql-style"></a>Style</h2>

>+SELECT DISTINCT groups.id, groups.name, groups.description
>+           FROM groups 
>+                CROSS JOIN user_group_map
>+                CROSS JOIN group_group_map AS ggm
>+          WHERE user_group_map.user_id = ?
>+                AND ((user_group_map.isbless = 1
>+                      AND groups.id = user_group_map.group_id)
>+                     OR (groups.id = ggm.grantor_id
>+                         AND ggm.grant_type = ?
>+                         AND ggm.member_id IN(1, 2, 3))))
>+       ORDER BY groups.id

7) I disagree with this indendation. This is not the usual way we do it. We do instead something like:

SELECT DISTINCT groups.id, groups.name, groups.description
           FROM groups 
     CROSS JOIN user_group_map
     CROSS JOIN group_group_map AS ggm
          WHERE user_group_map.user_id = ?
            AND ((user_group_map.isbless = 1
                  AND groups.id = user_group_map.group_id)
                 OR (groups.id = ggm.grantor_id
                     AND ggm.grant_type = ?
                     AND ggm.member_id IN(1, 2, 3))))
       ORDER BY groups.id


8) Maybe should you mention to use objects whenever possible instead of doing your own calls to the DB. For instance, build a user object using Bugzilla::User->new_from_login() instead of writing your own (and maybe incorrect) SQL query. Almost all .pm modules have a 'new' constructor now, and we should really start to use them. This also makes the code much cleaner, see e.g. editproducts.cgi, editcomponents.cgi & co.
Comment on attachment 200147 [details] [diff] [review]
v3

(Filenames and Paths)

> Template "fragments" (files which are used by other files are not intended to
> be used by themselves) and full single format templates should be named
> stubname.contentshortname.tmpl, eg bar.html.tmpl.

This would sound better as "other files *and* are not intended"

That's the only thing I see.  Feel free to check it in with that fixed.
Attachment #200147 - Flags: review?(justdave) → review+
(Assignee)

Comment 20

12 years ago
Great! Thanks, justdave. :-)

LpSolit and others: Feel free to open another bug and assign it to me, with any suggested changes that didn't make it in. A lot of them are really good suggestions, it would be good to update the Dev Guide with them.


Checking in docs/developer.html.tmpl;
/cvsroot/mozilla-org/html/projects/bugzilla/docs/developer.html.tmpl,v  <--  developer.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 291867
You need to log in before you can comment on or make changes to this bug.