Coding conventions

PSR-2 reference: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md

PSR-2 does not set a guideline for the properties names such as $StudlyCaps, $camelCase, or $under_score.
We are going to gradually use $camelCase from v1.10, in order to have a better code consistancy, because of the interactions we have with other libraries such as PEAR, Silex, Symfony2 components, Doctrine (to name a few) that use this convention.

Code documentation

For many reasons, it is important to write good documentation. All page or block-level code documentation should be generated respecting PHPDoc (see http://phpdoc.org/docs/latest/getting-started/your-first-set-of-documentation.html). Adding comments with ======= or ///////// separators is a deprecated syntax (although present in most of the Chamilo code). Please remove it on sight.
All files in the Chamilo code should always show the following second line (after the PHP opening tag):

/* For licensing terms, see /license.txt */

All code in Chamilo must be written and shared under the GNU/GPLv3+ license or a compatible license (in the exclusive case of including external libraries which already have their own license).

Language

The code, comments and developer instructions you write should be in English, no matter what your native language is. It's great if manuals are available in many languages, but code with comments and variables in English, French and Spanish is hard to understand. This also applies to bug reports and features requests.

Language terms

Note: We will be moving to gettext before the release of Chamilo 1.10, which should happen in 2014. We will migrate all current translations to it at this point, so don't worry about loosing efforts translating: nothing will be lost.

In general terms, language terms have to follow these few rules:
  • always start with an uppercase letter [A-Z]: e.g. $Alert;
  • split your words with uppercasing (not underscores): e.g. $AlertIncomingAttack;
  • when requiring a complex, variable string, use "%s" to enable a substitution, and mention it in the variable title with an X, Y or Z: $HelloXWelcomeToPortalYWhereYouHaveZTrial = 'Hello %s, Welcome to the %s portal, where you have a %s days trial available to you';

A language term should always be as large as possible.
Don't try to join several strings into one.
Something like: get_lang('Dear ').$username.get_lang('WelcomeToPortal').get_lang('WeHopeYouHaveFun').get_lang('BestRegards').$teamname; will never work out in languages with different grammatical rules like Japanese, Arabic, Quechua and many more.

Instead, use a context-full string like this "Dear %s, welcome to this portal. We hope you'll have fun learning through the courses made available to you. Best regards, %s".

The %s will then be replaced in the call to get_lang(): sprintf(get_lang('GeneralWelcomeToPortal'),$username, $teamname);

This is because, in other languages, the order of the sentences may not be exactly the same. You might make an introduction as "Yannick San E" (as compared with "Dear Mr Yannick"), instead of what could be possible with a single get_lang('Dear'), which would force you to use "E San Yannick", which doesn't mean anything in Japanese (it would be the equivalent of "Yannick Mr Dear" in English)

Twig

Language terms

When using Twig, instead of preparing the translated variable inside PHP and for Twig, you should use the Twig filter "get_lang" and give the appropriate variable (without the $ sign), like this:

  <div class="salutation">{{ "Hello" | get_lang }}</div>

This will automatically translate the message to the current language.

If, as suggested above, you use a composed language variable (using %s signs), then you can call the get_lang filter as well, but you need an extra step (and you need the replacement of "%s" to have been assigned to a Twig variable:

  <div class="salutation">{{ "HelloX" | get_lang | format(show_user_info.user_info.complete_name) }}</div>
Follow Twig code conventions

Use lower cased and underscored variable names:

{% set foo = 'foo' %}
{% set foo_bar = 'foo' %}

http://twig.sensiolabs.org/doc/coding_standards.html

Default conventions

As of 15/02/2013, the coding conventions of Chamilo LMS are the standard PSR-2: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md
The following rules define elements not defined in PSR-2 (which includes PSR-1, which includes PSR-0).

require vs include

'''include''' and '''require''' are language constructs, not functions. Although they do work with parenthesis, the fact of adding the parenthesis actually makes PHP believe it is a function, and as such go through the additional process of trying to understand the function.

Please use:

require 'main/inc/global.inc.php';

'''don't use''':

require('main/inc/global.inc.php');

When you get a chance to safely fix these in some script you are modifying, please do so.

An '''include''' in PHP is the same as a '''require''', except that '''require''' is more strict, meaning that if you try to include a file that does not exist, '''include''' will generate a warning message, while require will generate a fatal error.

Unless you want to try out inclusion of some files, and you actually catch the warning messages so you can deal with them, there's no reason at all to be using '''include'''. '''include''' obfuscate problems in your code, preventing you from realizing that some of the libraries you are trying to use are not actually there. Sometime later, side-effects might appear because the file you are supposed to be including is not actually included but you don't know it.

Don't use '''include''', use '''require''' instead!

Short php tags

Use the correct php tags: start with <?php and end with ?> . Do '''not''' use short tags <?, <?= or ASP-style tags <%, <%= as these rely on php.ini configuration settings ([http://www.php.net/manual/en/ini.core.php#ini.short-open-tag short_open_tag] and [http://www.php.net/manual/en/ini.core.php#ini.asp-tags asp_tags]). These settings can cause errors when they are off and Chamilo code assumes that they are set to on.

The php.ini comments advise not to use short tags or ASP-style tags:
NOTE: Using short tags should be avoided when developing applications or
libraries that are meant for redistribution, or deployment on PHP
servers which are not under your control, because short tags may not
be supported on the target server. For portable, redistributable code,
be sure not to use short tags.

Opening and closing php tags

As per PSR-2, we ask not to use the closing ?> tag at the end of a script. This is due to the fact that, if there are too many blank lines at the end of a script (more than one), after the closing tag, then your script will trigger the sending of HTTP headers to the user, preventing the system to send the appropriate redirect headers or other headers bound to the session (sending of cookies).

For example,

<?php
// Comment.
$var = 123;
?>

This code will send headers to the user although we are not at the end of the PHP process (there are still more scripts to be loaded in this chain), and consequently trigger the apparition of strange warning message, and the loss of the user's session.
If you intentionally omit the closing tag (which in turn could trigger problems for parsing the PHP code through XML, but who does that?), then you ensure nothing will send blank lines to the user's browser, because the blank lines are considered as part of the PHP code and, as such, are ignored.

<?php
// Comment.
$var = 123; 



Note that PHP itself will automatically ignore one blank line after the closing PHP tag. Just one.

Whitespace

Adding a blank line between lines of code creates paragraphs of code and helps to visualize it. You don't want to read books that have a thousand pages filled with line after line and no breaks; nobody wants to read code like that either.

Also, using spaces around operators and after comma's can help to make the code more readable. However, most people write their function brackets without a space between the function name and the brackets:

   function something($param1, $param2) 
   {
       if ($param1 and $param2) {
           // Code
       }
   }


When configuring your PHP editor, make sure it replaces all tabulations by 4 blank spaces. In PHPEclipse, this can be done by following the sequence: Window -> Preferences -> PHPEclipse -> PHP -> Appearance -> Display Tab Width: 4 and PHPEclipse -> PHP -> Typing -> Insert spaces for tab.
This ensures files are seen the same way under all editors (including terminal-based editors).

Submitting code style changes

When sending changes in coding style (for example increasing the indentation for a whole function, a whole class or a whole file, please make sure you send a commit with a "Minor: code style" message separately, so comparing the code between an initial file and another version with the modified code is not too difficult. As an example, a mixed code commit can be seen here http://code.google.com/p/chamilo/source/detail?repo=classic&r=fc7c01f47d27d5b663d2f31d0f975a7c98fefd55 (very difficult to distinguish the actual code changes).

Inline JavaScript and CSS

Syntax

All CSS classes and IDs should be expressed in lowercase letters separated by hyphen (-).

echo '<div class="generic-class other-class-less-generic">'.get_lang('Term').'</div>';

When using a CSS style specific to a tool, use the name of the tool as a prefix.

echo '<div class="tool-name-something tool-name-something-else generic-modifier">'.get_lang('Term').'</div>';

Inclusion

JavaScript and CSS must be added in the header. To do this, prepare your CSS or JS as strings, then inject them into the $httpHeadXtra variable:

global $httpHeadXtra;
$css = '<style type="text/css" media="screen">/*<![CDATA[*/
.mybutton {
  border: 1px solid black;
}
/*]]>*/
</style>';
$httpHeadXtra[] = $css;

(same thing for JavaScript, preferably using the JQuery way of loading a function: $().load(function(){...}))

Although inline CSS is not welcome, it is authorized under certain circumnstances

Debugging messages

All debugging messages should be kept locally, unless they represent a long-term debugging session (spanning several days). The error_log('debug message'); method is preferred to the echo 'debug message'; method because it doesn't break the user display, even if forgotten in the code.
To check the results of error_log(), check your Apache's error log. Under Ubuntu, you can have a live view of the debug console using the following command pattern:

sudo tail -f /var/log/apache2/yoursite-error.log

Use of brackets { ... }

For classes and methods, put your opening brackets and closing brackets on an empty line, and at the same indentation

function something($a)
{
    for ($i=0; $i < $a; $i++) {
        echo "$a <br />";
    }
}

This gives a very clear view of where a loop starts and where it finishes. Some code editors even improve this visibility by tracing a vertical line between the opening and the closing bracket.

Please note that control structures should have their opening brackets on the same line.

Use brackets on single-line conditions

Using a condition on a single line is not a good idea, and [[https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md PSR-2]] kind of implicitely forbids it: "Opening braces for control structures MUST go on the same line, and closing braces MUST go on the next line after the body.", so the logic in Chamilo is to always define the body of a condition on a separate line.

To remain flexible and more quickly readable, the code should always include the brackets.

This helps delimiting clearly the boundaries of the condition even if the indentation is broken, without requiring the 1-second brain loop to figure out if that's really the single statement included and whether it should be more indented or not.

Incorrect example:

if ($hello) echo 'goodbye';

Correct example:

if ($hello) {
    echo 'goodbye';
}

Use of backticks

Backticks ( ` ) are a very bad MySQL-only habit. '''Don't use backticks at all'''. This will improve your respect of the SQL standard and make you find meaningful names for your tables and your fields, that will avoid them being misunderstood for a MySQL reserved keyword.
When copy-pasting some code from phpMyAdmin, make sure you remove all backticks.

Use of internal functions

Try to use Chamilo's internal functions (API) as much as possible. There are three main libraries that you will need to review before starting to code with Chamilo: main_api.lib.php, database.lib.php and security.lib.php

The database library provides a complete abstraction layer that allows you to use all the necessary MySQL functions from Database:: prefixed static methods, to the exception of mysql_query(), that is replaced by api_sql_query(), defined in main_api.lib.php.

Don't use '''mysql_fetch_array()''', but use '''Database::fetch_array()''' (as of Chamilo 1.8.*)

Due to an uncontrolled past history of coding, a very large number of calls to mysql_query() are passed through the api_sql_query(). Although this is not really a problem in itself, we are making efforts to see this function passed to Database::query() instead, to make sure everything related to the database layer will be handled by the Database class. If you are updating some code in Chamilo, please make sure you change all api_sql_query() calls to Database::query(), to ease the move to an abstraction layer in the future.

Variable naming

New variables must be expressed in $camelCase (starting with lower case and using a capital letter for each new word).

Although you might find a lot of $lower-case-variables, these are remains of an older coding convention. Wherever you are completely sure about the use of a specific variable expressed in lower-case, you are invited to change it to camel case in a separate commit prefixed with the "Minor - Coding conventions" message.

'''Language variables''' should all use camel-case and start with an upper-case character. However, these are used literally through the get_lang() function, so there should be no confusion about their usage.

'''CONSTANTS''' should always be in all-upper-case letters

time(), not NOW

'''time()''' is a PHP function that generates a UNIX timestamp (the number of seconds from 1/1/1970 00:00:00 until now).
'''NOW''' is a MySQL function that generates a timestamp to the required format to fill the field it is put in.

Combining the two functions can lead to time matching problems, because time() depends on Apache and NOW depends on MySQL server. These two programs can have different times (they can be on different servers or simply have a different configuration).

Because the process in Chamilo is based on PHP interpretation, time() must be used instead of NOW, even when simply filling database logs fields. This will ensure that time-based values are all based on the same time source, and will make reliable statistical queries possible.

Date and time management

These coding conventions have to be applied only for new code (e. g. code written after Feb 17, 2010) that uses dates and times:
  • Any date and time stored in the database must use the SQL DATETIME format
  • Before storing any DATETIME in the database, you MUST call the function '''api_get_utc_datetime($time)''' with the date and time you want to store in the database. This function will return you a string under the format Y-m-d H:i:s that you can store directly in the database.
  • Before displaying any datetime, you must call the function '''api_get_local_time($time, $to_timezone = null, $from_timezone = null)''' with the datetime you want to convert.

To know more about date and time management in Chamilo, click on Date and time management

Database: INT, not INT (11)

INT (11) is a markup created by phpMyAdmin in the automated table creation query generation, but is actually useless in the sense of PHP + MySQL. To be truly portable, declare INT fields as INT, not INT (11).

This is a quote from http://dev.mysql.com/doc/refman/5.0/en/numeric-types.html:

The display width does not constrain the range of values that can be stored in the column, 
nor the number of digits that are displayed for values having a width exceeding that
specified for the column. For example, a column specified as SMALLINT(3) has the usual
SMALLINT range of -32768 to 32767, and values outside the range allowed by three characters
are displayed using more than three characters.

Database: INT UNSIGNED for primary and foreign keys

When declaring a primary or foreign key, please use the "UNSIGNED" qualifier. Although few of the current fields have been declared as unsigned, there is no use for the negative side of integers.

Unsigned integers, in MySQL, go from 0 to 4,294,967,295 (which is a considerably safe primary key allowance, given that MySQL starts to have problems dealing with new records over the 1,000,000,000 records without optimization)

Database: Foreign keys implicit declaration

When declaring foreign keys in the database, the relation is considered implicit, and should be using the name "id" in the table where it acts as a primary key, and "[origintable]_id" in the table where it is used as a foreign key. The element type must be the same.

Database: SQL reserved keywords

In the different implementations of SQL, some words happen to be reserved for specific use, like uid for internal IDs in Oracle or "time" in PostgreSQL. Although Chamilo 1 doesn't support anything else than MySQL, we are definitely working on having support for other database management systems. This means that we have to avoid using reserved keywords, even if they are reserved only in another database system. If you have doubts, you can check the words on this page: http://www.petefreitag.com/tools/sql_reserved_words_checker/?word=time

An easy trick to avoid using reserved keywords by mistake is to simply put the name of the table (or any short version of it) as a prefix to the field.

An essential part of not using reserved keywords is to avoid using the special MySQL character "backtick" (`) which doesn't work in any other database management system, and is specially made to allow you to use reserved keywords in MySQL. We don't want that, so we don't want to see any ` in a SQL query. Backticks are generally added because you made a copy-paste from phpMyAdmin. Avoid that. Care about your queries!

Error obfuscation

When using the '''@''' symbol to obfuscate errors, make sure you check $php_errormsg (only available if php.ini '''track_errors''' setting is on) to deal with the error. In this specific case, adding a call to error_log() is an excellent idea, so that the error message is not "lost".

See [http://www.php.net/manual/en/reserved.variables.phperrormsg.php the php documentation page] for more info about this variable.

For example:

$res = @rmdir('/var/www/chamilo/archive/ABC');
if ($res === false) {
error_log(FILE.' line '.__LINE__.': '.(ini_get('track_errors')!=false?$php_errormsg:'error not recorded because track_errors is off in your php.ini'));
}

Fatal, Warning and Notice -level error messages

We assume that any code you add to Chamilo will not generate Fatal nor Warning errors. You can try that by putting your portal in "test mode" in the configuration page.
Notice-level error messages are accepted when submitting new code, but they are intended to disappear when the code is properly reviewed.

To show notice-level warnings, it is necessary to change the code of main/inc/global.inc.php to substitute '''E_ALL & ~E_NOTICE''' by '''E_ALL'''.

api_get_setting('X') === 'true'

Due to a certain level of freedom added to the platform settings in the database, true and false values are written as 'true' and 'false' strings in the database. As a result, api_get_setting('X'), where X has a value of either true or false, will return 'true' or 'false', not TRUE or FALSE.

For this reason, these coding conventions include the fact that, when calling api_get_setting() on a boolean value, you compare it directly to the strings 'true' or 'false', and you do NOT use the if(api_get_setting('X')) form. Explicit, not implicit.

Security

Please also check our Secure_development_policy for high-level requirements in terms of security.

Security: Filter close to the database

Although the new object-oriented architecture of Chamilo 2.0 might improve this, there is no way to always be 100% sure that the variables entering one of your functions have been pre-filtered. It is thus mandatory to always make sure the data is filtered properly before being put in the database.

To do this, always filter your data inside the function which is calling the database query. This way, you ensure your database is safe, no matter where the variables came from.

For strings, use

$filteredString = Database::escape_string($unfilteredString);

For integer values (or other numerical values) use the corresponding converter:

$filteredInt = intval($unfilteredInt);
$filteredFloat = floatval($unfilteredFloat);

This is very important, as using the string filter on integers might open the way for an SQL injection (refs #7440)

Security: Filter user input that has a chance to get on screen

XSS attacks are always based on using JavaScript to the user. What is less known is that a user can be directed to your site with a URL that includes JavaScript. In this case, the JavaScript can be executed with increased access due to the user's session. Although the effects are often damaging the user's experience rather than the website's safety, it is best to avoid the problem as much as possible.

A Security class is provided inside Chamilo 1.8.x which allows for filtering of several kinds of attacks. We ask you to make use of Security::remove_XSS() on any content that might contain JavaScript to show to the user. This includes messages generated from third party sources.

Generally speaking, displayable text has to be filtered just before displaying it.

Security: CSRF

Cross-Site Request Forgery is also a mechanism by which JavaScript is abusing the user's browser. It is generally triggered from another website to benefit from the "open-session" effect that a user generates by staying "connected" to several sites at any one time. For example, a hackers site might use the user's session to connect to Chamilo and post spam content inside a course's description.

To avoid this, always make sure your forms are protected by a mechanism of controlled submission. The Security class can help you do that, particularly Security::get_token() and Security::check_token().

Git

Git and inclusion of external libs: commit original first

When integrating an external library, plugin or any piece of code maintained by a third party, make sure the first integration commit is the integration of the original version of the external code. Only commit the adaptation to the Chamilo code after that first "original" commit. This way, it will be much easier, later on, to detect the customization that has been made to integrate the external library.

A natural consequence of that (but that might not be obvious for everybody) is that you should always send one separate commit to include a new library. It should not be sent together with a Chamilo code change in one group commit.

Please avoid sending the code examples or unit tests that generally come with these libraries. They will take space and possibly introduce security flaws in the Chamilo code.

Starting in 1.10.0, Chamilo LMS uses Composer and Bower for the inclusion of PHP libraries and JS libraries, respectively. If the library you are including is available through either Bower or Composer, you are required to use it (and update composer.json) as a source rather than inserting it into Git directly.

Other Git concerns

A commit should be something "atomic", that you can send as one piece and that you can remove in one piece without affecting the rest too much.
If fixing a small bug, a commit could be a few lines of changes in one or several files.

If, during the bugfix, you find code that is not developed in respect with these coding conventions, your code style changes must go into a separate commit, preferrably starting with the keyword "Minor - " so reviewers know this commit doesn't affect the code logic (and cannot affect the code logic).