Koha Test Wiki MW Canasta on Koha Portainer

Test major Koha Wiki changes or bug fixes here without fear of breaking the production wiki.

For the current Koha Wiki, visit https://wiki.koha-community.org .

Coding Guidelines

From Koha Test Wiki MW Canasta on Koha Portainer
Jump to navigation Jump to search

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 welcome 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.

Patches submitted before the introduction of a new rule may pass QA even if they do not meet the current coding guideline requirements of the discretion of the QA team member (Grandfather clause).

Handling disagreements

If a community member has concerns with a patch that:

  • Are specifically about coding style (not stated functionality or regressions in other code)
  • Are not covered by an existing coding guideline

then the patch and concern should be brought up on the development mailing list and/or next development IRC meeting so a new coding guideline can be added.

Licence

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

# This file is part of Koha.
#
# Copyright (C) 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 3 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, see <http://www.gnu.org/licenses>.

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

Indentation

Indentation of text-based files (.pl, .pm, .tt, .css, .js, etc.) must be 4 spaces (not tabs, not 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).

Editors

Koha's source tree includes an EditorConfig file (.editorconfig). If your editor supports EditorConfig, this file will be picked and your editor will be configured (for Koha's source) so it follows the guidelines.

Most editors support EditorConfig, either natively or by using a plugin. Check your favourite editor's support here.

Specific editor tricks

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

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.
  • 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

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"

HTML6: A hash key with the same name as a TT virtual method

In TT, when calling a hash value, you use its key, like "borrower.name". But TT also proposes virtual methods with the same syntax, like "results.size".

The problem appears when a hash key has the same name as a TT virtual method. If hash "data" contains a key named "size":

  • "data.size" will return the number of keys in the hash
  • use "data.item('size')" to get the value of the key named size

The list of TT Virtual Methods.

This solution was found to correct Bug 11357. Solution came from here.

HTML7: Use system preference TT plugin

When adding a system preference check, use the Koha.Preference() method, provided by the Koha TT plugin, in the TT template. This should be used instead of passing a variable to a TT template from a Perl script using the C4::Context->preference() method.

Usage example: 

[% USE Koha %]
...
[%- IF Koha.Preference( 'OpacAuthorities' ) -%]
   code here
[%- END -%]

HTML8: Use Asset TT plugin for linking JavaScript and CSS files

Usage example: 

[% USE Asset %]

# create <link> tag 
[% Asset.css("css/buttons.dataTables.min.css") | $raw %]

# create <script> tag
[% Asset.js("lib/jquery/plugins/dataTables.buttons.min.js") | $raw %]

HTML9: Filter all template variables

In order to prevent XSS vulnerabilities all the variables displayed in the templates must be escaped using an appropriate filter. See Bug 13618 for more information.

Not all of them are concerned by the vulnerability but to be on the safe side we are going to escape them all.

Usually you will want to use the filters provided by TT: html, uri, url, etc. And sometimes we will want to display the variable as it is, i.e. you will want to make the browser interpret the content (HTML or JavaScript), in that case you will explicitly use the raw filter, that does nothing.

Examples:

  • Display a variable coming from the controller, you do not really know if it is safe or not:
[% your_variable | html %]

if your_variable contains HTML tags, they will be displayed, escaped, in the browser. So if it is contains 

"<script>alert('hola');</script>"

, the html escaped version will be 

"&amp;lt;script&amp;gt;alert('hola');&amp;lt;script&amp;gt;"

, which will be displayed 

"<script>alert('hola');</script>"
  • Display a variable that does contain HTML tags, but you want them to be interpreted by the browser. A typical use case is the system preferences (eg. StaffLoginInstructions, IntranetCirculationHomeHTML, OPACMySummaryNote, etc.)
[% Koha.Preference('OPACMySummaryNote') | $raw %]

if your variable contains html tags, they will be interpreted. So if it is contains 

"<strong>something important</strong>"

, the raw version will be 

"<strong>something important</strong>"

, which will be displayed "

something important

"

  • Display a variable in a URL query string, using the uri filter
<a href="/a/link/to/detail.pl?param=[% param | uri %]">click me</a>

You may need to use the url filter in cases where the variable contains the URL:

  • Display a variable in a URL using the url filter
<a href="[% homepage_url | url %]">Return to home</a>


  • Display a safe variable
[% safe_variable | $raw %]

If and only if you are sure that the variable is safe (for instance they are integers from the DB), you can use the raw filter. It can be done later to speed up the processing, in case we are displaying hundreds/thousands of integers, or already escaped variables, in a loop.

This is not recommend if you are not certain of what you are doing!

(Note that you will need to add [% USE raw %] to the start of your template if you do want to use the raw filter plugin.)

If it is still not clear you can try and apply this patch: [1]

Click on the different link it generates and understand which ones are the correct ones.

NOTICE1: Default Notices should now be writing using template toolkit

The notices system has supported the use of template toolkit for some time, all developments that add new notices or update existing notices should now use Template Toolkit.

Accessibility

ACC1: All OPAC pages require a single 'block' with the '.maincontent' class

Every OPAC page should contain a single .maincontent block signifying to accessibility helpers where the primary content of the page is located.. This allows for the 'Skip to main content' link to work. 

  <div class="maincontent">

  </div>

ACC2: Input type "number" should be avoided

Using `<input type="number">` has a number of accessibility and localisation issues. Decimal delimiters and number incrementing in browsers is inconsistent and screen readers treat the input type with various levels of inconsistency and incompatability.

The current recommendation is to use: 

    <input type="text" inputmode="numeric">

This allows for a degree of separation between how the user enters data (“input mode”), what the browser expects the user input to contain (type equals number), and potentially how it tries to validate it.

More information can be found at: gov.uk

  • Note:* A `pattern` attribute can optionally be added to limit the numeric input to just integers `pattern="\d*"` or `pattern="[0-9]*"`.
    <input type="text" inputmode="numeric" pattern="\d*">

ACC3: Title elements should contain unique information information first

https://www.w3.org/WAI/tips/writing/ states that title elements should contain unique information first. This aids accessibility for all as browser titles become much more relevant and useful for navigation.

Example: 

    <title>Advanced editor shortcuts &rsaquo; Administration &rsaquo; Koha</title>

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>

       ...JavaScript here

    </script>

Note that "//<![CDATA[" comments are no longer required now that templates use an HTML5 doctype. Existing comments should be removed as templates are updated. As of Bug 20053 the "type" attribute is no longer added to the <script> tag.

JavaScript blocks should be placed at the bottom of the template. See Coding Guidelines#JS12:_Include_javascript_at_the_end_of_template.

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. Exception: #JS5:_Enabling_translation_of_language_strings_in_linked_JavaScript

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: Use the format() method to join variables with translatable strings

Koha's JavaScript allows you to join variables with translatable strings using a format() method: 

$('#merge_status').text(_("Closed on %s").format(invoice.closedate));

During the translation process translators can restructure the sentence and move the "%s" wherever it belongs in their language. More than one placeholder can be passed: 

var alert_message = _("You have chosen to move all unreceived orders from '%s' to '%s'.").format(budget_from, budget_to);

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

Since Bug 21156 - Internationalization: plural forms, context, and more for JS files you can have translated strings directly defined in the .js files. Example: 

 __("your string to translate")
 __nx("There is one item", "There are {count} items", 3, {count: 3}));

The list of the available function are in koha-tmpl/intranet-tmpl/js/i18n.js, they are the twins of the method from Koha/I18n.pm.

More examples can be found in the POD of Koha/Template/Plugin/I18N.pm.

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 ESLint

ESLint offers an online tool for checking JavaScript, as well as several options for incorporating into your workflow via many tools. 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 23833 - JavaScript should conform to coding guidelines recommended by ESLint.

JS9: Avoid the use of event attributes like "onclick" to attach events

In order to separate behavior from presentation, the use of "onclick" and other event attributes should be avoided.

From the Mozilla Developer Network:

"Warning: These attributes should be avoided. This makes the markup bigger and less readable. Concerns of content/structure and behavior are not well-separated, making a bug harder to find. Furthermore, usage of event attributes almost always causes scripts to expose global functions on the Window object, polluting the global namespace."

https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Event_attributes

Instead, event handlers should be created in scripts linked to or embedded in the page. For example, instead of using "onclick:" 

<input type="button" onclick="location.href = '/page/redirect/link'" value="Redirect" />

Define an event handler in a <script> block:

<script>
    $(document).ready(function(){
        $("#redirect").on("click",function(e){
        e.preventDefault();
        location.href = '/page/redirect/link';
    });
})
</script>

And add an ID to the element, or a class if you want to apply it to several elements: 

<input type="button" id="redirect" value="Redirect" />

JS10: Form Validation

JavaScript-based form validation should be done using the jQuery form validation plugin unless the form validation will not work with its limitations. We are not limited to rules built into the plugin. Custom validation rules can also be defined.

Enabling form validation by form class name

Validation of required fields can be enabled in a form by adding the class "validated" to the form tag. This enables automatic validation of required fields based on markup in the form.

Required fields should have a "required" class to trigger form validation and a "required" attribute as a non-JS fallback: 

  <label for="firstname" class="required">First name:</label>
  <input type="text" name="firstname" id="firstname" class="required" required="required" />
  <span class="required">Required</span>

This will appear like this:

First-name-required.png

If you try to submit the form without filling in the field the validation plugin will add a default error:

First-name-validated.png

Enabling form validation via JavaScript

If you require more complex validation rules, these can be added via JavaScript. A good example is in the validation rules for adding or editing a patron category: 


    $("#category_form").validate({
        rules: {
            categorycode: {
                required: true,
                letters_numbers: true
            },
            description: "required",
            enrolmentperiod: {
                required: function(element){
                    return $("#enrolmentperioddate").val() === "";
                },
                digits: true,
                enrollment_period: true,
                min: 1
            },
            enrolmentperioddate: {
                required: function(element){
                    return $("#enrolmentperiod").val() === "";
                },
                enrollment_period: true
            },
            dateofbirthrequired: {
                digits: true
            },
            enrolmentfee: {
                number: true
            },
            category_type: {
                required: true
            }
        },
        messages: {
            enrolmentperiod: {
                required: __("Please choose an enrollment period in months OR by date.")
            },
            enrolmentperioddate: {
                required: __("Please choose an enrollment period in months OR by date.")
            }
        }
    });

In this example category_form is the ID of the <form> tag. In the list of validation rules, names like categorycode description refer to the ID of form fields.

The line, 

 description: "required",

...shows a shortcut syntax for defining a required field. The extended syntax allows more options: 

categorycode: {
  required: true,
  letters_numbers: true
},

JS11: 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 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.

JS12: Include javascript at the end of template

All scripts should be at the end of template, just before: 

[% INCLUDE 'intranet-bottom.inc' %]

At the begin of template set the 'footerjs' variable like this: 

[% SET footerjs = 1 %]

This is important for handling system-wide included javascript correctly in header and footer.

The included javascript should look like this (example taken from tools/overduerules.tt): 

[% MACRO jsinclude BLOCK %]
    [% Asset.js("js/tools-menu.js") %]
    <script>
        var tab_map = { "1" : _("First"), "2" : _("Second"), "3" : _("Third")};
        $(document).ready(function() {
            $('#selectlibrary').find("input:submit").hide();
            $('#branch').change(function() {
                $('#selectlibrary').submit();
            });
            $("li>a.tab").each( function(){
                var id = $(this).attr("data-number");
                $(this).html(tab_map[id]);
            });
            $('#rulestabs').tabs();
        });
    </script>
[% END %]

So the page related javascript is defined as macro and after that correctly handled by js_includes.inc, for more information see bug 17418 and bug 17858

Terminology

TERM1: The agreed set of library terms must be used

See the dedicated page: Terminology.

TERM2: Gender-Neutral Pronouns

When referring to a person who could be of any gender, you should use the words they/them/their or avoid using pronouns altogether. This goes for code comments, text in templates, and strings in tests. For example, here's a string from a patrons test updated to be gender neutral.

Before: 

is( $total, $enrolmentfee_K + $enrolmentfee_J, "A child growing and becoming a juvenile,
he should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) );

After: 

is( $total, $enrolmentfee_K + $enrolmentfee_J, "A child growing and becoming a juvenile,
they should pay " . ( $enrolmentfee_K + $enrolmentfee_J ) );

Gender neutral terms are preferable for a few reasons. They're more welcoming, showing that Koha expects users and contributors to be of any gender. They're also more accurate. Inappropriately using a particular gender can cause confusion, leading someone to believe that code operates differently based on the value of borrowers.sex, for instance.

TERM3: Inclusive Language

Use inclusive language that promotes a community where everyone feels they are represented. Avoid terms that express or imply negative ideas that are racist, colonialist, or otherwise biased towards groups of people. Refer to the terminology list for terms to use/avoid.

Example: Instead of using the term blacklist, use deny list which more accurately describes its function.

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, and dbicdump, 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 (
  module varchar(20) NOT NULL default '' COMMENT 'Koha module that triggers this notice or slip',
  code varchar(20) NOT NULL default '' COMMENT 'unique identifier for this notice or slip',
  branchcode varchar(10) default NULL COMMENT 'the branch this notice or slip is used at (branches.branchcode)', 
  name varchar(100) NOT NULL default '' COMMENT 'plain text name for this notice or slip',
  is_html tinyint(1) default 0 COMMENT 'does this notice or slip use HTML (1 for yes, 0 for no)',
  title varchar(200) NOT NULL default '' COMMENT 'subject line of the notice',
  content text COMMMENT 'body text for the notice or slip',
  PRIMARY KEY  (module, code, branchcode)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT="table for all notice templates in Koha";

SQL12: Booleans

In order to standardize the way we handle boolean columns, and to provide a good path for serializing boolean values into JSON objects, this rules must be followed.

DB structure

Boolean fields have to be defined in kohastructure.sql as TINYINT(1) for consistency. For example: 

CREATE TABLE `letter` (
  ...
  is_html TINYINT(1) default 0 NOT NULL, -- does this notice or slip use HTML (1 for yes, 0 for no)
  ...
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
DBIC schema files

Booleans have to be annotated as booleans in the schema files. This annotation will be picked by Koha::Object::TO_JSON to properly handle booleans in the serialization process.

Following the above example, this is how it would be done for the letter table, in the Koha/Schema/Result/Letter.pm file: 

# Created by DBIx::Class::Schema::Loader v0.07042 @ 2017-05-09 21:01:19
# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:WZxxDvP7M/Ns/e9cFseeNA

__PACKAGE__->add_columns(
    '+is_html' => { is_boolean => 1 },
    ...
);

Perl

Here is a guide to using Koha Objects

Here is a guide to using the Koha Logger

PERL1: Perltidy

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

This rule has been superseded by Coding Guidelines#Indentation

PERL7: Definitions

The same vocabulary should be used both internally in source code ( Module names, variable names, etc ) and externally ( javascript, templates, etc ).

Canonical Koha terminology is listed on the Terminology page.

If you are developing a new feature that requires new terminology, please locate and use the matching term defined by ODLIS. If you cannot find an appropriate term via ODLIS, please bring the question to the Koha community via the Koha Developers Mailing List or the next Koha Developers IRC Meeting.

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

Koha namespace [current]

  • subroutines in the Koha namespace should be snake case.
  • module names should be camel case.


C4 namespace [outdated]

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

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

    my @array = SearchSuggestion($user, $author, $title, $publishercode, $status, $suggestedbyme)

Searches for a suggestion that matches the supplied parameters. If you want a parameter to be ignored, set it to C<undef>.

Returns:
 
=over 4
 
=item C<@array>

the suggestions found, as an array of hashes. The hashes contain the fields from the suggestions table.

=back 

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.

=cut

sub SearchSuggestion  {
  my ($user,$author,$title,$publishercode,$status,$suggestedbyme) = @_;
  ...
  return \@array;
}

When updating a module you are required to add POD for added and changed subroutines or ensure existing POD is accurate, it is encouraged to add POD to all subroutines as possible

PERL14: Exports

Exports from routines in C4:: and Koha:: should be kept to an absolute minimum, and Good-Practices should be followed. 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. Modules in the Koha:: namespace should not reference the C4:: namespace, with the exception of C4::Context.

Koha::Object(s)

Tables can be turned into Objects trivially with Koha Objects

Table relationship accessors should be written to allow 'prefetch'

For example one should use: 

sub guarantor {
  my $self = shift;
  my $guarantor_rs = $self->_result->guarantorid;
  return unless $guarantor_rs;
  return Koha::Patron->_new_from_dbic($gaurantor_rs);
}

One should not use: 

sub guarantor {
  my $self = shift;
  return Koha::Patrons->find( $self->guarantorid() );
}

Koha::Objects can be extended with new methods. Koha::Object(s) is the preferred way to create object-oriented ("OO") modules in Koha at this time.

Another examples:

There is a foreign key (and ON DELETE CASCADE constraint), and there is no need to test the existence: 

sub biblio {
    my ( $self ) = @_;
    my $biblio_rs = $self->_result->biblio;
    return Koha::Biblio->_new_from_dbic( $biblio_rs );
}

See also Koha::Item->current_holds.

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) = @_;
    $args = {} unless defined $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 (updated Apr 26, 2017)

Unit tests must be provided for *ALL* new routines in modules, as well as for changes to existing routines (perhaps excluding very trivial edits). The scripts in t are database independent and should also pass if the database is completely empty or the SQL service is stopped. The scripts in t/db_dependent obviously need a Koha database, but should not rely on [optional] sample data as branches, currencies, patron categories, etc. As demonstrated in many existing scripts, TestBuilder can help you to create your own data.

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.

PERL20: Koha namespace

Modules in the Koha namespace sould be object oriented when possible, using Koha::Object(s) as a preferred base.

PERL20.1 Use Koha::Object methods if exists

If an Koha::Object already exists, use it instead of other methods of table CRUD.

PERL21: CRUD operations

CGI scripts that handle CRUD (Create, Read, Update, Delete) operations should all be handled in the same script when possible. Example: admin/cities.pl

PERL22: Plack friendly coding

Global state variables in modules and CGI scripts are forbidden, their use elsewhere is discouraged. (see https://perl.apache.org/docs/general/perl_reference/perl_reference.html#my____Scoped_Variable_in_Nested_Subroutines for context)

PERL23: Single argument for methods and functions

Methods and functions should take a single argument: either a database ID, arrayref, or hashref with named arguments.

PERL24: Performance and cache related patches should include benchmarks of some sort to prove their effects

Simply put, if you submit a patch that is intended to improve performance, include so pre and post patch benchmarks to show the effect.

PERL25: Read only variables

Use the constant pragma. In the past Readonly was used, but it was phased out with bug 14066 and bug 16588.

PERL26: Koha::Exceptions (Approved on Apr 26, 2017)

Instead of die or croak when meeting unfavorable conditions in our code, we should raise exceptions via Koha::Exceptions.

Do not hastily add new submodules in Koha/Exceptions. First try to use existing or more generalized ones, like MissingParameter, WrongParameter, etc. When raising the exception in your code, you can pass a more detailed description to throw() or even pass additional fields (see Exception::Class).

Examples of its use can be found in e.g. Koha/Patron/Modification.pm or git grep "Koha::Exceptions". Also look at the try catch blocks (with Try::Tiny) in the subtest Exceptions of t/db_dependent/Koha/Objects.t. Here is another example: 

 # From Koha::Exceptions
use Exception::Class (
[...]
  'Koha::Exceptions::WrongParameter' => {
    isa => 'Koha::Exceptions::Exception',
    description => 'One or more parameters are wrong',
  },
[...]
);

 # From Koha::DateUtils and sub output_pref
use Koha::Exceptions;
[...]
Koha::Exceptions::WrongParameter->throw( 'output_pref should not be called with both dt and str parameter' ) if $dt and $str;

 # From t/DateUtils.t
use Try::Tiny;
[...]
try {
  output_pref({ 'str' => $testdate_iso, dt => $dt, dateformat => 'iso', dateonly => 1 });
  ok( 0, 'output_pref should carp if str and dt parameters are passed together' );
} catch {
  is( ref($_), 'Koha::Exceptions::WrongParameter', 'output_pref should throw an exception if str and dt parameters are passed together' );
};

PERL27: Rule about returning undef from Tomas Cohen Arazi [NEW PROPOSAL dev meeting 2/2020]

PERL28: General object-oriented code guidelines [PROPOSAL FOR CHANGE dev meeting 2/2020; to be moved from PERL15]

PERL29: Direct Object Notation

Modern best practice is to always use direct object notation.

Use: 

 my $cgi = CGI->new;

Don't use: 

 my $cgi = new CGI;

Reasoning: https://www.effectiveperlprogramming.com/2020/06/turn-off-indirect-object-notation/

PERL30: Pass references when adding a new plugin hook

When implementing a new hook for Koha plugins using Koha::Plugin->call( 'method_name', $arguments ) one must ensure that $arguments are passed as a reference to ensure each plugin that implements the hook method is fed the most up to date data (which may have been altered by a previous plugin called in the execution chain).

Use: 

 my $new_barcode = $newdata{'cardnumber'};                                                                                                                                                                                                                                                                            
 Koha::Plugins->call( 'patron_barcode_transform', \$new_barcode );

Not: 

 my $new_barcode = Koha::Plugins->call( 'patron_barcode_transform', $barcode );

Reasoning: If we pass a simple value, each plugin will receive just that original value and not the updated return value from the previous plugin

Scripts

CMD1

All new scripts that are intended to be run at the command line (cronjobs, maintenance) should use the Koha::Script base class. 

   use Koha::Script
   use Koha::Script -cron;

This sets up some basic environment for your script behind the scenes so logging actions that may be called behind the scenes are recorded consistently.

Documentation

DOC1: Cookies

Documenting the use of cookies in Koha for the purpose of abiding to European GDPR and other data privacy regulations is required. If a new cookie is added, an old cookie is removed or updated, the documentation on Use of Cookies needs to be updated accordingly.

Deprecations

DEPR1: non-XSLT search results and bib display templates deprecated

As of 2014-07-02, use of the XSLT search results and bib details display templates for the staff interface and public catalog is now the preferred option. Consequently, the non-XSLT option is now deprecated and it will not be maintained.

Template files used by the non-XSLT bib and results displays need not be touched in future staff interface and public catalog enhancements; it is necessary only to update XSLT.

DEPR2: The GRS-1 Zebra indexing mode is deprecated

As of 23 July 2014, the DOM Zebra indexing mode is now the standard supported method for Koha. The GRS-1 indexing mode is deprecated for all record types and MARC flavours.

New Koha patches that update index definitions must change the DOM indexing definitions. If a patch is intended to be backported to a maintenance release, changes to GRS-1 index definitions should be kept in a separate patch.

DEPR3: C4 deprecated for new modules

New modules should be added to the Koha namespace as C4 is deprecated.


Developer handbook

Retrieved from "https://wiki.test.koha-community.org/w/index.php?title=Coding_Guidelines&oldid=30565"