Coding Guidelines

From Koha Wiki

Koha coding guidelines

Basics

General rule: if you submit code that fixes existing code that violates those guidelines, 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 do 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 a 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.

Commit messages

When you submit code to the project (using Git) please write useful commit messages. Detailed guidelines on writing commit messages can be found at the wiki page on Commit messages.

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
As of version 3.12 Koha uses an HTML5 doctype, and templates should validate accordingly. Templates in previous versions are expected to be valid XHTML (Transitional). For these earlier versions the only exception is some attributes which exist in HTML5 (autocomplete="off").
HTML5 does not strictly require closing tags, but Koha templates should continue to use XHTML-style syntax tag closing, including self-closing tags.
Web Developer Toolbar, a Firefox addon, is recommended for testing templates in the staff client. It includes the option to "Validate local HTML" using the W3C's validator.

HTML1: Template Toolkit markup inside HTML

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. The most common example of an incorrect construction:

<option value="1" [% IF (selected) %] selected="selected" [% END %]>Yes</option>

This must be written this way:

[% IF (selected) %]
   <option value="1" selected="selected">Yes</option>
[% ELSE %]
   <option value="1">Yes</option>
[% END %]

HTML2: HTML inside Perl scripts

Language strings inside .pl or .pm files cannot be translated. Define variables in your script to pass to the templates for output. For example:

[% IF ( variable) %] Message to display [% END %]

HTML3: Displaying dates

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.

HTML4: Upper and lower cases in strings

We have decided to use upper cases only for the first 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. There is an open bug (Bug 2780, Capitalize strings consistently) for submitting patches with corrections.

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

JS1: Embedding JavaScript blocks in templates

Whenever possible JavaScript should be placed in a separate file. When this is not possible (for instance, when template processing is required) JavaScript may be embedded directly in the template. Follow this structure:

<script type="text/javascript">
   //<![CDATA[

   ...JavaScript here

 //]]>
 </script>

The commented lines mark the JavaScript as code which should not be validated by the HTML validator. JavaScript blocks should be placed within <head> when possible but may be embedded elsewhere in the template if necessary (for instance when using JS in an include file).

JS2: Enabling translation of language strings in embedded JavaScript

If your JavaScript includes strings which will be output to the user, it must be possible for them to be parsed by Koha's translation tool. Each language string must be wrapped in a special function, _(). Wherever would quote a string, "Hello world!", you must enclose it in this function: _("Hello world!"). For example:

   if (f.company.value == "") {
    alert(_("You must specify a name for this vendor."));
    return false;
   }

A more complex 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>"

This must be done any time you want linked JavaScript to output language strings to the user. Failing to do so will result in untranslated interface elements.

JS3: Avoid strings which mix text and markup

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\">";

JS4: Avoid joining multiple language strings with other variables

Avoid building sentences by concatenating multiple translatable strings. Your language may not use the same sentence structure. The best options keep strings at the beginning or end of a construction. This is incorrect:

$('#cartDetails').html(_("Your cart contains ")+updated_value+_(" items"));

...where "updated_value" is a number. A better way to do this might be:

$('#cartDetails').html(_("Items in your cart: ")+updated_value);

The translation tool will only parse JavaScript which is embedded in templates. Text in linked JavaScript files cannot be translated.

JS5: Enabling translation of language strings in linked JavaScript

The translation tool cannot parse language strings in linked JavaScript files (Bug 4503, Javascript files in js directory are not translated). In order to allow linked JavaScript files to output strings to the user, placeholders must be used in the script and definitions provided in the template.

For example, when the staff client's basket.js wants to pop up an alert warning the user that they have not selected any items:

           alert(MSG_NO_RECORD_SELECTED);

In order for this to work the variable, "MSG_NO_RECORD_SELECTED" must have been already defined. In this case it is defined in doc-head-close.inc before basket.js is included:

         var MSG_NO_RECORD_SELECTED = _("No item was selected");

JS6: Trailing comma in a reference list

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

JS7: Don't use <body onload=...>

<body onload> is not the best way to add load events to a page. Generally the event should be added to the jQuery $document(ready)() function.

   $(document).ready(function(){
     // onload function here
   });

Doing so lets jQuery handle all the onload events and helps prevent conflicts. jQuery can intelligently parse and process multiple document.ready() blocks, so a linked JavaScript file can contain one and an embedded <script> block on the same page can include one too.

JS8: Follow guidelines set by JSHint

JSHint offers an online tool for checking JavaScript, as well as several options for incorporating the tool into your workflow on several platforms. Developers are encouraged to use this tool to catch errors in their JavaScript. Errors found in existing JavaScript in Koha should be corrected as part of Bug 9284 - JavaScript should conform to coding guidelines recommended by JSHint.

Using jQueryUI widgets

The jQueryUI JavaScript file included with Koha contains several modules providing different features. The jQueryUI file was created using their Build Your Download tool. Only the modules required for Koha are included.

jQueryUI modules currently in use in master:

UI Core
- Core
- Widget
- Mouse
- Position
Interactions
- All
Widgets
- Autocomplete (staff client only)
- Tabs
- Datepicker
- Slider (staff client only)

New jQueryUI downloads should be built using the smoothness theme.

As new features are added to Koha which require additional jQueryUI modules, the jQueryUI JavaScript file should be regenerated by selected the existing modules and only the new required module. Unused modules should not be included.

jQueryUI CSS Customizations

jQuery widgets in Koha are styled to match Koha's other interface elements. CSS customizations to jQueryUI widgets should be put in the global CSS file for the affected interface: opac.css for the OPAC, staff-global.css for the staff client. The CSS file downloaded with jQueryUI should not be modified.

jQueryUI tabs

The jQueryUI tabs widget enables JavaScript-driven tabs in many places in the OPAC and staff client. No page-specific include is required. The jQueryUI library is globally included.

The HTML structure:

<div id="some_cutom_id" class="toptabs">
  <ul>
    <li><a href="#tab1">Tab 1</a></li>
    <li><a href="#tab2">Tab 2</a></li>
  </ul>
  <div id="tab1"> Contents of tab 1 </div>
  <div id="tab2"> Contents of tab 2 </div>
</div>

Note that each anchor in the unordered list corresponds to the id of the

to which it refers. The link must contain only the in-page reference ("#tab1"). If you use a URL ("/cgi-bin/koha/catalogue/detail.pl#holdingst") jQueryUI will try to load that page into the tab via AJAX.

The JavaScript initialization of the tabs:

$(document).ready(function(){
   $("#some_custom_id").tabs();
});

For more information see http://jqueryui.com/demos/tabs/

jQueryUI Datepicker

The jQueryUI datepicker widget replaces the DynArch calendar library which getting old and the new version of which is no longer open source. The jQueryUI datepicker widget is part of the globally-included jQueryUI library but it requires some special initialization on each template where it is to be included. calendar.inc should be included immediately after doc-head-close.inc:

 [% INCLUDE 'doc-head-close.inc' %]
 [% INCLUDE 'calendar.inc' %]

calendar.inc includes some reusable configuration settings and translatable strings to be used by the calendar.

When you want a form field to trigger a basic calendar popup simple add a class to the input tag:

 <input type="text" size="10" id="suspend_until" name="suspend_until" class="datepicker" />

If a form field requires custom configuration the datepicker instance can be initialized in a script block:

 $(document).ready(function(){
$("#newduedate").datepicker({ minDate: 1 }); // require that renewal date is after today
 });

In this case you would not add the datepicker class to the input tag.

Part of the global configuration available via calendar.inc is some default behavior for linking two inputs so that the date in the first input doesn't fall after the date in second. If your template contains only one pair of linked fields you can add a special class to each one: datepickerfrom and datepickerto.

  <ol>
    <li>
     <label for="from">From:</label>
     <input type="text" id="from" name="dateduefrom" size="10" value="[% dateduefrom %]" class="datepickerfrom" />
    </li>
    <li>
     <label for="to">To:</label>
     <input type="text" id="to" name="datedueto" size="10" value="[% datedueto %]" class="datepickerto" />
    </li>
  </ol>

In order for this to work via the default configuration settings the id of the first input must be "from."

If you have more than one pair of inputs which need to be linked in this way you must explicitly include the configuration for each in the JavaScript on that page. Here is an example from reports/acquisition_stats.tt:

        $(document).ready(function() {
	        // http://jqueryui.com/demos/datepicker/#date-range
	        var dates = $( "#from, #to" ).datepicker({
	            changeMonth: true,
	            numberOfMonths: 1,
	            onSelect: function( selectedDate ) {
	                var option = this.id == "from" ? "minDate" : "maxDate",
	                    instance = $( this ).data( "datepicker" );
	                    date = $.datepicker.parseDate(
	                        instance.settings.dateFormat ||
	                        $.datepicker._defaults.dateFormat,
	                        selectedDate, instance.settings );
	                dates.not( this ).datepicker( "option", option, date );
	            }
	        });
	        var datesRO = $( "#fromRO, #toRO" ).datepicker({
	            changeMonth: true,
	            numberOfMonths: 1,
	            onSelect: function( selectedDate ) {
	                var option = this.id == "fromRO" ? "minDate" : "maxDate",
	                    instance = $( this ).data( "datepicker" );
	                    date = $.datepicker.parseDate(
	                        instance.settings.dateFormat ||
	                        $.datepicker._defaults.dateFormat,
	                        selectedDate, instance.settings );
	                datesRO.not( this ).datepicker( "option", option, date );
	            }
	        });
        });

See http://jqueryui.com/demos/datepicker/ for more information.

Database

Database Design

SQL1: FK violations in the installer

Installer SQL should not cause FOREIGN KEY violations

SQL2: SQL92 keywords

Don't use SQL92 keywords as field names

SQL3: ANSI quotes

Use SET SQL_MODE=’ANSI_QUOTES’

SQL4: Date defaults

Design for date field with NULL rather than ‘0000-00-00’

SQL5: Quoting integers

Don't use quotes around integer values:

default '0'

becomes

default 0

SQL6: Backticks

Don't use ` in table or column names (this is a mySQLism)

SQL7: Primary keys

If you add a table to Koha, the table must have a primary key, and the primary key name must be tablename_id (color_id for example)

SQL

SQL8: SQL code in .pl scripts

There must be no SQL in CGI .pl scripts (command-line scripts may include special-purpose SQL). For historical reasons 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 or Koha/xxxx.pm (xxxx being the module your SQL refers to)

SQL9: SELECT

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

SQL10: Placeholders

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

Adding tables and fields

SQL11: All fields added to the database must be documented in kohastructure.sql

This is so that Schema Spy will provide a description of the fields. It is very easy to do. Simply add a comment at the end of each CREATE TABLE/field definition line:

DROP TABLE IF EXISTS letter;
CREATE TABLE letter ( -- table for all notice templates in Koha
  module varchar(20) NOT NULL default '', -- Koha module that triggers this notice or slip
  code varchar(20) NOT NULL default '', -- unique identifier for this notice or slip
  branchcode varchar(10) default NULL, -- the branch this notice or slip is used at (branches.branchcode)
  name varchar(100) NOT NULL default '', -- plain text name for this notice or slip
  is_html tinyint(1) default 0, -- does this notice or slip use HTML (1 for yes, 0 for no)
  title varchar(200) NOT NULL default '', -- subject line of the notice
  content text, -- body text for the notice or slip
  PRIMARY KEY  (module, code, branchcode)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

Perl

PERL1: Perltidy

Use perltidy with its default man perlstyle settings (agreed at http://meetings.koha-community.org/2011/koha.2011-10-05-10.00.log.html#l-862)

PERL2: Modern::Perl

All scripts and modules should use Modern::Perl to enable the strict and warnings pragmas, i.e.

use Modern::Perl;

PERL3: Fix 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.

PERL4: Perlcritic

All Perl submitted must validate perlcritic (level 5, the lowest one)

PERL5: Disagreements

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

Indentation must be 4 spaces (no tab, no 2 or 6 or 8 spaces) You can fix tabs by using

 git config –global core.whitespace trailing-space,space-before-tab
 git config --global apply.whitespace fix

(from http://blog.bigballofwax.co.nz/2011/12/15/fixing-whitespace-when-apply-patches-with-git/)

If you want to reindent large amounts of existing code in connection with a bugfix or enhancement, you must do so in a separate patch from the one with code changes (unless the reindent is required due to the addition or removal of an indentation level (for example, an additional if added around a block).

PERL7: Definitions

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: Undefined arguments at the end of function calls

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) )

PERL9: Subroutine 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 information about more than 1 branch

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: Verboten subroutine parameters

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

[Deprecated as of start of 3.12 release cycle] Each module should have a $VERSION variable to preserve progressive levels of compatibility with dependent code.

PERL13: POD

POD comments should include as least the following headings:

NAME
    The name of the module and a very brief description of the type of functions contained in the module.

SYNOPSIS
    This may give an example of how to access the module from a script, but it may also contain any general discussion of the purpose or behavior of the module's functions, and discussion of changes made to the module.

DESCRIPTION
    If not included in the SYNOPSIS, a general description of the module's purpose should appear here.

FUNCTIONS
    Descriptions of each function in the module, beginning with an example of how the function is called and continuing with a description of the behavior of the function.

AUTHOR
    This will generally be the "Koha Development Team," but may also be an individual developer who has made significant contributions to the module. Give a contact e-mail address.

For two examples, see the POD comments in the Search.pm module (perldoc Search.pm) and the Biblio.pm module.

In addition, every subroutine must include POD as in the following:

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

PERL14: Exports

Exports from routines in C4:: and Koha:: should be kept to an absolute minimum. Ideally there should be no routines at all exported from Koha:: though this ideal may not be attainable.

PERL15: Object-oriented code and the Koha:: namespace

Whenever it makes sense, code added to the Koha:: namespace should be object-oriented. However, code that is naturally procedural should not be shoehorned into the OO style.

Class::Accessor

Class::Accessor is a great way to provide access to member variables:

 use base qw(Class::Accessor);
 __PACKAGE__->mk_accessors(qw( member1 member2 ));

Those two lines at the top of the package are sufficient to create read/write accessors to the member variables $self->{'member1'} and $self->{'member2'} that can be accessed as follows:

 warn $object->member1;
 $object->member2('newvalue');

A useful idiom for ->new()

A useful idiom for the ->new() routine in object-oriented classes that do not need to process the arguments passed in as a hashref but merely need to save them for future processing:

 sub new {
     my ($class, $args) = @_;
     return bless ($args, $class);
 }

An object with that initialization routine can be created with:

  my $obj = Koha::Object->new({ 'member1' => 'value1', 'member2' => 'value2' });

PERL16: Hashrefs should be used as arguments

Rather than passing hashes, which is inefficient, or using lots of positional parameters, subroutines that require multiple arguments should use hashrefs for passing arguments, like so:

 sub myroutine {
     my ($args) = @_;
     return $args->{'index'} . ' => ' . $args->{'value'};
 }
 
 print myroutine({ 'index' => 'kw', 'value' => 'smith' });

An example of BAD practice would be either of the following:

 sub myroutine {
     my (%args) = @_;
     return $args{'index'} . ' => ' . $args{'value'};
 }
 
 print myroutine('index' => 'kw', 'value' => 'smith');

or

 sub myroutine {
     my ($index, $value) = @_;
     return $index . ' => ' . $value;
 }

 print myroutine('kw', 'smith')

PERL17: Unit tests are required

Unit tests must be provided for *ALL* new routines added to the C4:: and Koha:: namespaces. Statement coverage should be as close to 100% as possible. In order to check test coverage, you will need to install Devel::Cover and run the following commands:

 perl -MDevel::Cover ${MYTEST}
 cover

This will generate a report in cover_db/coverage.html which you can review for coverage issues.

For more about unit tests and continuous integration, see Unit_Tests and Continuous_Integration.

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

Editor Helpers

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

Vim