Coding Guidelines

From Koha Wiki

Jump to: navigation, search
Home > Development

Koha coding guidelines


Contents

Basics

General rule: if you submit code that fixes an existing code that violates those guidelines, the QA can still be passed. You don't have to fix everything. If you want to fix more than just your fix, to respect current guidelines, you're welcomed of course. But in this case, please to it in a 2nd patch, to have reviewers being able to distinguish easily what's related to your bugfix and what is related to your guidelines-violation fixing.

Example: you submit a patch that does not respect perlcritic after applying the patch. The QA team will check for perlcritic after your patch, and if the patch does not add new perlcritic violation, it's OK.

Licence

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

# This file is part of Koha.
#
# Copyright YEAR YOURNAME-OR-YOUREMPLOYER
#
# 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 this program; if not, write to the Free Software Foundation, Inc.,
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

Committing Code

When you submit code to the project (using Git) please write useful commit messages. Reference a bug number in the first line of the commit message.

Good commit message

A good commit message has a few parts:

- 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-community.org (bug 99999: for example), any particular branch limitation ([3.2.x] for example) and a summary of this particular change. - The next line should be blank - Then, please include a description of the changes you made to the code. - 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.


Bad commit message 

*** empty log message ***


Refactoring Code

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 yours! If you must refactor, make sure that the commit for the refactoring is completely separate from a bugfix.

Bug numbers

Contributors to Koha must 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-community.org that addresses your contribution, please open one and describe the bug or enhancement. Then, include Bug + the bug number at the beginning of your commit message and set the "Status" field to "Needs Signoff". This helps us keep track of patches that have been contributed but not yet applied.

It is also requested that you attach the patch file to the bug report. This allows others not on the patches list to pull in your code, test, and sign-off. You can use git bz for that

User Interface

HTML Templates

  • Since version 3.4, Koha uses Template::Toolkit
  • There is a way to check syntax. prove xt/author/valid-templates.t will catch unclosed [% IF %] and [% FOREACH %] and nesting problems.
  • To run it on one file, xt/author/test_template.pl file_name path_to_approprate_template_incudes_dir

(HTML1) Avoid [% IF %] inside tags because the translation tools try to read the templates as straight HTML and most TT comments inside tags probably confuse/break them. (HTML2) Never include any string that will be displayed in a .pl or .pm, it won't be translatable. Use a variable in .pl that has a [% IF variable %]Message to display [% END %] in the template.

Displaying dates

(HTML3) If you have a date to display in the interface, just: add 

[% USE KohaDates %]

at the beginning of your template and, for each date to be displayed, add | $KohaDates For example: [% MyDate | $KohaDates %]

The MyDate must be provided in iso format (that's what mySQL provides), and will be displayed as defined by the DateFormat syspref/

This coding guideline has been added in january 2012, you may find code not using it, but for new code, you must use it.

Upper and Lower cases in strings

(HTML4) We have decided to use Upper cases only for the 1st letter of a given sentence/label/tab description/page title/...

So you must write:

Pay fines

and not

Pay Fines

At the time of writing of this rule (2012-02-07), there are a lot of places where this rule is broken. You're welcomed to fix them !

Also note this rule will ease a lot translation process, as, for instance, translators will have to translate each of "Pay Fines" and "Pay fines" and "pay fines"

JavaScript

Trailing comma in a reference list

(JS1) 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
    }
 };

Translatable text

(TRANS1) 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\">";

Database

Database Design

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

becomes 

default 0
  • (SQL6) don't use ` in table or column names (this is a mySQLism)

SQL

(SQL7) There muse be no SQL in .pl script. For historical reason there is SQL in admin/script, that's considered as a mistake. SQL is also tolerated for reports. Other than those 2 cases, all SQL must be in the C4/xxxx.pm (xxxx being the module your SQL refers to)

SELECT

(SQL8) 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)"?

(SQL9) 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);

Perl 

use strict;
use warnings;

Another option is to use: 

use Modern::Perl;

that has the same result

  • (PERL3) Error and warning messages produced by strict and warnings should be quelled by fixing the code, not by tacking on a no warnings directive.
  • (SQL7) 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/
  • (PERL4) all Perl submitted must validate perlcritic (level 5, the lowest one)
  • (PERL5) In case of a disagreement on coding style, the perlstyle man page will decide for us 
 * For hash refs, use $myhash->{mykey}, not $$myhash{mykey}
  • (PERL6) indentation must be 4 spaces (no tab, no 2 or 6 or 8 spaces)
  • (PERL7)define vocabulary (& update file & tables names accordingly) 
 * We have decided to use [ODLIS].
  * Should we use supplier [], vendor[+++++] or bookseller [] in acquisition ? 
 //(again, they mean different things: acq has standardized on vendor, bookseller is a no-no, not all our suppliers/vendors sell books)//
  • (PERL8) if you call a sub and don't use some arguments at the end of the sub, it's OK not to pass undef ( ie: blabla($var1,$var2,undef,undef) can be written blabla($var1,$var2) )

Command-line argument conventions

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

(PERL9) SUBs NAMING conventions 

All sub names must contain 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.

(PERL10) sub Parameters conventions

  • 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

(PERL11) No CVS

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

(PERL12) VERSION

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


(PERL13) POD

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;
}

Editor Helpers

Naturally, what editor is used is up to you. Some helpful things for the various ones can be listed here.

Retrieved from "http://wiki.koha-community.org/wiki/Coding_Guidelines"