Koha coding guidelines

This page aims to precise/improve/replace current Coding guidelines from Kohadocs.

Each file (scripts & modules) must include the GPL licence statement, typically at the top.

# This file is part of Koha.
#
# Koha is free software; you can redistribute it and/or modify it under the
# terms of the GNU General Public License as published by the Free Software
# Foundation; either version 2 of the License, or (at your option) any later
# version.
#
# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License along with
# Koha; if not, write to the Free Software Foundation, Inc., 59 Temple Place,
# Suite 330, Boston, MA  02111-1307 USA

• All scripts and modules should enable the strict and warnings pragmas, i.e.

use strict;
use warnings;

• Error and warning messages produced by strict and warnings should be quelled by fixing the code, not by tacking on a no warnings directive.
• SQL code should never be embedded in the .pl scripts, it should almost always be in the .pm C4 modules. One exception to this is the perl scripts in admin/

• There is a way to check syntax. prove xt/author/valid-templates.t will catch unclosed tmpl_if and tmpl_loops and nesting problems.
• To run it on one file, xt/author/test_template.pl file_name path_to_approprate_template_incudes_dir

• Installer SQL should not cause FOREIGN KEY violations
• Don't use SQL92 keywords as field names
• Use SET SQL_MODE=’ANSI_QUOTES’
• Design for date field with NULL rather than ‘0000-00-00’
• Don't use quotes around integer values:

    default '0'

    becomes

    default 0

When you commit code to the project (whether using CVS or git) please write useful commit messages.

Don't needlessly refactor code, if it ain't broke, don't fix it! Don't waste time on changing style from someone else's style to your's! If you must refactor, make sure that the commit for the refactoring is completely separate from a bugfix, etc.

A good commit message has a few parts:

1. git uses the first line of your commit message is used as an email subject line and as a summary to describe your commit. Take the opportunity to make that line short, concise, and useful. Include the bug number from bugs.koha.org (bug 99999: for example), any particular branch limitation ([3.0.x] for example) and a summary of this particular change.
2. The next line should be blank
3. Then, please include a description of the changes you made to the code.
4. Last, please describe any functionality changes that should be included in the user documentation. This will help the documentation team keep caught up with the constantly changing application

bug 1234: adding limit to number of items shown on reading history page [9.3.x]

Added max_issues_history to borrowers table. Max_issues_history defines
how many items will show up in the OPAC Reading History page. So, if
max_issues_history is set to 5, the page will only list the last 5 items
the borrower has returned.
If max_issues_history is set to 0, then history is unlimited,
i.e. The same as before max_issues_history was added.

The manual should be updated to show how users can set the max_issues_history variable.

*** empty log message ***

Contributors to Koha are encouraged to reference a bug number with every commit. This helps us determine the purpose of the commit and establishes a more searchable and understandable history. If there is no bug open at bugs.koha.org that addresses your contribution, please feel free to open one and describe the bug or enhancement. Then, include the bug number in your commit message and set the “priority” field to “patch-sent”. This helps us keep track of patches that have been contributed but not yet applied.

FF will tolerate a trailing comma in an array literal, but Internet Explorer does not.

This is wrong:

KOHA.MyLib = {
    GetUgc: function(id) {
        // foo
    },
    ugcCallBack: function(booksInfo) {
        // foo
    },
};

Replacing }, line with } this is correct:

KOHA.MyLib = {
    GetUgc: function(id) {
        // foo
    },
    ugcCallBack: function(booksInfo) {
        // foo
    }
};

To be translatable by Koha translator system, text must be surrounded by _( ).

For example, this code is not correct:

param1 +="<\/optgroup><option value=\"newlist\">[ New List ]<\/option>"

This code is correct:

param1 +="<\/optgroup><option value=\"newlist\">[ "+_("New List")+" ]<\/option>"

And text mustn't contain html tags. For example, this code is not correct:

var displaytext = _("The Numbering Pattern will look like this: <br \/><ul class=\"numpattern_preview\">");

The correct form is:

var displaytext = _("The Numbering Pattern will look like this: ") + "<ul class=\"numpattern_preview\">";

Instead of writing:

$sth=$dbh->prepare("Select borrowers.*,categories.category_type from borrowers left join categories on borrowers.categorycode=categories.categorycode where cardnumber=?");

write:

my $query = '
SELECT borrowers.*, categories.category_type
  FROM borrowers
    LEFT JOIN categories ON borrowers.categorycode = categories.categorycode
  WHERE cardnumber = ?
';
$sth = $dbh->prepare($query);

What are the rules from this example:

• SQL reserved words must be in capital letters
• the query string must start by a “go to line”, thus the query starts at the beginning of an editor line
• the query string always ends with a line containing only ”';”
• 2 spaces indent for “FROM”, “WHERE”, “ORDER BY”, “GROUP BY”, “LIMIT” blocks.
• 2 more spaces for “JOINS” (in the “FROM” block) and “AND” (in the “WHERE” block)

How to write “WHERE xxx IN (23, 45, 67)”?

In order to prevent SQL injection attacks, you must use placeholders (question marks) and pass the variables into the “execute” method for WHERE clauses and all other points in the SQL that pass user input directly to database queries.

my $query = '
SELECT itemnumber,
       biblionumber
  FROM items
  WHERE biblionumber IN
    (' . join(',', ('?') x @biblionumbers) . ')';
my $sth = $dbh->prepare($query);
$sth->execute(@biblionumbers);

We should decide on a convention for command-line argument processing and stick to it:

The problem is… Some people may have already created scripts (for crontab jobs, like you said) already relying on “rebuild_nozebra.pl” running right away. For ”–help” I think we should add “usage” instructions (I don't think that breaks any thing).

Yeah, adding ”–help” to jobs that don't have it shouldn't break crontabs.

As proposal of convention rules, the GNU coding standards, http://www.gnu.org/prep/standards/ . In particular the option table and two standard options http://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html#Command_002dLine-Interfaces

All sub names must contains a verb & a name. The verb describing what the sub does and the name the object it works on. Use capital for verb initial & name initial.

For example, AddBiblio will add a biblio in the database. common verbs :

• Add to add (NOT “New” or “Create”)
• Mod to modify (NOT “Update”)
• Del to delete (NOT “Remove”)
• Get to read something (NOT “Read” or “Find”)

if the sub works on a single object, it's singular. If it works on more than 1, it's plural. For example :

• sub GetBranch ⇒ will return a single branch
• sub GetBranches ⇒ will return many branches informations.

Internal functions should begin with an underscore and contain only lower-case characters with fully-spelled out names.

For example, _koha_delete_biblio will delete a biblio record from the koha biblio table.

• some subs have a $dbh handler as 1st parameter : remove it & use C4::Context→dbh instead
• some subs have $env as 1st parameters : remove it & use C4::Context→userenv→{variable} instead

• Development has moved from CVS to git. Therefore the use of CVS keywords $Id$ and $Revision$ should be discontinued.

• Each module should have a $VERSION variable to preserve progressive levels of compatibility with dependent code.

In addition to use POD coding guidelines on this page : http://www.kohadocs.org/codingguidelines.html#d0e442 each function has to be “poded” like the example below :

 
=head2 SearchSuggestion
 
=over 4
 
my @array = SearchSuggestion($user, $author, $title, $publishercode, $status, $suggestedbyme)
 
searches for a suggestion
 
return :
C<\@array> : the suggestions found. Array of hash.
Note the status is stored twice :
* in the status field
* as parameter ( for example ASKED => 1, or REJECTED => 1) . This is for template & translation purposes.
 
=back
 
=cut
 
sub SearchSuggestion  {
  my ($user,$author,$title,$publishercode,$status,$suggestedbyme) = @_;
  ...
  return \@array;
}

For 3.4, we plan a large rewrite & strengthening of the code. Here are some questions we should ask ourselves on coding guidelines. If you prefer an option, just put a + in the []

• in case of a doubt or disagreement : PBP (Perl Best Practices) will decide for us : Yes [++] or No [] ?
• Spelling: should we use Upper Case On Every Word [] or just use UC the 1st letter [+] or use LC and separate words with underscore [+](in labels or table headers)
• Indentation: 4 spaces [++] or tabs [] or 2 spaces []
• For hash refs, use $myhash→{mykey} [++] or $$myhash{mykey} [+] The $$ syntax is deprecated and all perl documentation including perlreftut tell you to use the arrow → Changing to use $$ is something I would fight hard against
• define vocabulary (& update file & tables names accordingly)
  • Should we use patron [+], member [], borrowers [] ?
  • Should we use supplier [], vendor[++] or bookseller [] in acquisition ? (acq has standardized on vendor, bookseller is a no-no, not all our suppliers/vendors sell books)
• about SQL table & row naming
  • table in singular [+] or plural [++] form ? Tables in the plural make for some odd sounding code once you start using an ORM. It sounds more logical to say $borr = Borrower→new(); than $borr = Borrowers→new(); We should be thinking about this in terms of moving toward a more Class based concept (DBIx::Class) than in the handcrafted SQL of old. The main thing is to be consistent.
  • row of tables in singular [++] or plural [] form ?