Skip to content

Commit

Permalink
Streamline developer documentation (aces#6107)
Browse files Browse the repository at this point in the history
Modifies some of our developer documentation to be up-to-date and less repetitive. Moves some sections around so that they are in a more appropriate place.

Resolves aces#4390
  • Loading branch information
driusan authored and laemtl committed Jun 16, 2020
1 parent 4b1f359 commit e185d10
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 45 deletions.
14 changes: 9 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,8 @@ issues and/or Redmine tickets (if applicable).
* Include a test for any new module in the `modules/MODULENAME/test/`
directory. You can look at other modules for examples of how to
write tests.
* Add your new automated tests to TravisCI in the `.travis.yml`.
* Before sending any pull request, make sure you run our static analysis tools
using the command `make checkstatic` and fix any resulting errors. Otherwise,
your pull request will fail our automatic testing and we will not be able
to merge it. If you run the command `git config core.hooksPath .githooks`, git will automatically
run `make checkstatic` when you do a git push.
locally. More information can be found in the [testing guide](./docs/wiki/99%20-%20Developers/Automated%20Testing.md).
* Try and make all changes backwards-compatible with existing installations.
* If you must change something in a non-backwards-compatible way - or if it
would affect the data or custom code of a study - document this in your pull
Expand All @@ -84,6 +80,14 @@ you've contributed contains language that is friendly and accessible.
If you're unsure about any of the above, feel free to ask us for
clarification via the mailing list.

## Git Best Practices
- Any changes should be done on a branch based on the current development branch and contain only the changes which are applicable for that branch. (ie don't merge master back into your branch, and don't include commits that are unrelated) so that if someone merges the branch into their repository, they only get that branch's changes.
- Commits should be atomic (self contained) and contain only the changes described by the commit message. The commit message should be a sentence that describes the goal of the change as a whole.
- Avoid changing unrelated code in the same commit, even if it is an improvement.
We prefer that you create two separate pull requests A and B rather than add a
'quick bonus fix' to pull request A.
- ALWAYS do a diff before committing (after doing "git add file1 file2" when you're planning on doing `git commit`, you can use `git diff --staged` to see a diff of what will be committed). Ensure that nothing unexpected is included.

## Getting Started

If you're looking for ideas for a way to contribute but don't know where
Expand Down
76 changes: 36 additions & 40 deletions docs/CodingStandards.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
# General Formatting
- Indentation should be 4 spaces instead of tabs
- Each embedded block should be indented 1 more indentation level
# Coding Standards

In vim, you can use the following in your .vimrc to set this automatically:
```vim
set tabstop=4
set shiftwidth=4
set expandtab
set autoindent
set smartindent
```
## General

while working in vi, this command will automatically indent:
```vim
gg=G
```
### Style and Formatting
We use a variety of static analysis tools to create a consistent style across
the codebase. For more details about the tools used and what they do,
please review the [Automated Testing Guide](./wiki/99%20-%20Developers/Automated%20Testing.md)

### Separation of languages
HTML, PHP, CSS, SQL, and JavaScript code should not be mixed. Instead, they
should be separated into their own directories and files.

# PHP
PHP code must pass our static analysis test suite. For details on the process,
please the [Automated Testing Guide](./wiki/99 - Developers/Automated Testing.md)
An exception to this is PHP code that builds SQL queries to interact with the
database. Where possible, this should be limited to specific classes that
handle these transactions rather than mixed in with more general code.

## PHP
PHP code should be compliant with PHP Standards Recommendations](https://www.php-fig.org/psr/) PSR-7 and PSR-15.

All new functions should use type hinting and return type declarations.

Expand All @@ -28,31 +26,35 @@ the top of the file:
<?php declare(strict_types=1);
```

DateTime and related classes should be used instead of strings for handling dates.

Prepared statements MUST be used for any database interactions that use user input.

LORIS has many classes that use a Singleton design pattern. To facilitate with
unit testing, it is best to use these singletons via the NDB_Factory class.
For example, you should use the Database class like this:

```php
$database = \NDB_Factory::singleton()->database();
```

instead of

```php
$database = \Database::singleton();
```

# HTML
- HTML should never be mixed with code.
- HTML should go into a template and be rendered using a templating library (smarty for PHP).
- General formatting rules about indentation applies for each tag embedded inside
another tag of HTML:
- Embedded tags should be on a new line and indented one level deeper than its
parent, e.g.:
```html
<div>
<span>foo</span>
</div>
```

# JavaScript
- Javascript should never be mixed with HTML or PHP code.
- Javascript should go into `modules/js/`
- Any newly written JavaScript should pass ESLint with default options.
- Compiled JavaScript should not be tracked by Git so make use of .gitignore
if you are adding new files or converting existing code to use React.

# SQL
* Prepared statements MUST be used for any statements which involve user input.
* You must never use string concatenation (such as the example below) to create an SQL statement as this is a serious security risk. i.e.
**Don't do the following:**:
```mysql
"SELECT abc FROM table WHERE field1='" + $_REQUEST['val'] + '"';
```
ANSI join syntax:
```mysql
"table1 t1 JOIN table2 t2 ON(conditions)"
Expand All @@ -65,9 +67,3 @@ ANSI join syntax:

- In any query involving more than one table, each table should be given an alias (`t1` and `t2` above)
- SQL keywords should be ALL CAPS

# Git:
- Any changes should be done on a branch based on the current development branch and contain only the changes which are applicable for that branch. (ie don't merge master back into your branch, and don't include commits that are unrelated) so that if someone merges the branch into their repository, they only get that branch's changes. In particular, so pull requests merge the proper code.
- Commits should be atomic (self contained) and contain the changes and only the changes described by the commit message. The commit message should be a sentence that describes the goal of the change as a whole, for seeing the details of what code changed we have diff.
- Don't try to correct unrelated code in the same commit, even if it violates these coding standards, that should be done in a separate branch/commit with a message such as "Fixed coding standard violations". In particular, don't try to fix whitespace since that is likely to cause conflicts even if you don't have any real (code) changes to those lines.
- ALWAYS do a diff before committing (after doing "git add file1 file2" when you're planning on doing git commit, you can use git diff --staged to see a diff of what will be committed). Ensure that nothing unexpected is included (such as whitespace changes. If using an external diff tool such as kdiff3, ensure your tool is whitespace sensitive)
3 changes: 3 additions & 0 deletions docs/wiki/99_Developers/Automated_Testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ Generally there are two categories of testing, **static** and **dynamic**. The f

Static tests can be executed by running `make checkstatic` in the LORIS root diretory. This command will also use PHP's default linter (`php -l`) to check for basic syntax errors.

If you run the command `git config core.hooksPath .githooks`, git will automatically
run `make checkstatic` when you do a git push.

We use the following tools for static analysis.

### PHP
Expand Down

0 comments on commit e185d10

Please sign in to comment.