Code Review

This list includes things that we look for when reviewing pull requests into the Core product. Please read through and become familiar with this list when coding your changes.

Rule

Details

Follow defined coding standards

https://docs.elentra.org/technical/developers/coding-standards

New code is encapsulated in Classes

Classes that are potentially reusable can be placed in core/library/Entrada

Code must be human-readable

  • Use meaningful variable and function names

  • Code must be properly indented

  • Code must be cleaned and organized

Massive files of code must be logically broken down

  • Form submission handling should never be in the same file as the form itself

  • Giant if/switch statements with all sorts of cases within are not allowed

New API endpoints should be created in Elentra API

We're moving away from the ME API. We will make exceptions for this, however if you're building a new feature, or rebuilding an existing one, we highly encourage you to build the new endpoints in the Elentra API project

Any non-trivial methods should have unit tests

Any method that contains significant logic should have a test for that logic. A getter or setter, for example, does not need a test. A method that manipulates data from a database may require a mock, but can you separate the database functionality from the manipulation?

Functions and classes require a comment block

Add PHPDoc comment blocks for your code (getters and setters not necessary). Explain what the code does: what, how and why.

No "magic" numbers or strings

Do not use non explained numeric or string values in your logic. Use define() for constants or declare them as class constants

Code does not have duplicated blocks

Extract code into a function and call it where needed

Functions should have a single responsibility

Any function you write should do one thing, and one thing only. Break down your code to multiple functions, and for medium/large features, create classes to encapsulate a process or a pattern

Lines should be entirely visible on a typical editor screen

Wrap your code appropriately

Avoid more than 3 levels of nested control structures

Move code to seperate functions

Function names reflect their purpose and usage

Static functions are called statically, and non-static functions are called non-statically

Use type hinting for function parameters and return values

public function getFooBar(int $number, string $text) { ...

Functions should not have more than 5 parameters

Pass values in an associative array or object

Variables should have the tightest scope possible

Declare variables inside the block where they are used.

No commented-out code

Remove all commented out code

No @todo

Resolve any @todo items, or remove

No debug code

remove all console.log var_dump dd etc..

Functions must be as reusable as possible

Try not to rely on globals. Pass in (inject) the data that is needed.

Use strict comparisons where possible

Use === to test type equality as well as values, so that PHP doesn't do under-the-hood type conversions that may not work as expected.

No queries in views

There should be no SQL in a PHP file that outputs to the browser. It should be moved into a Model, where appropriate, or into a class in core/library/Entrada/[module name]/ If you see raw SQL or $db→ in the rendering PHP, that is a sign something is wrong.

HTML code should be outside of PHP tags

Avoid assigning/concatnating large blocks of HTML to PHP variables. Leverage PHP open/close tags to output HTML as is, to improve readability and maintainability

All resources must exist in the Elentra source tree. Do not call external JS or CSS files.

Do not reference files via CDN in the core product

All text strings exposed to users are translated

Use $translate→_()

Migrations must be named correctly

Use the ticket number ME9999

Audit is required in any Migration, and must return proper value

  • 0 if the migration has not yet been run

  • 1 if the migration has been run

  • -1 if the migration state is unknown

PHP functions should be available in the target version for the PR

Check the PHP version in composer.js, and set that version in PHPStorm

Fix version should match target branch

Within the Pull Request, ensure that the target matches the fix version

Hotfix should target lowest version where bug occurs

When fixing a bug, the developer should target the lowest version of Elentra where the bug occurs. We support last 3 versions. Ex: when 1.18 is the latest version, bugs should be fixed in 1.15.x (the latest version of 1.15)

Fix merge conflicts

If there are merge conflicts, the developer needs to merge the target branch locally, address all merge conflicts and update the PR

Database version-specific features used

Do not use IF NOT EXISTS for columns (when creating tables it is allowed) or CREATE OR REPLACE TABLE, if MySQL 5.7 is the target database. Check for the use of other version-specific syntax.

Migrations should be thoroughly tested

When creating a migration, implement and test the audit() method first, then the up() and down() methods. You should always run the "up" migration, then "down", and then "up" again for testing.

Variable validation checks for daisy-chained method calls

Ensure that method calls are from the valid object before calling them. Daisy Chaining might cause a show stopper error when the object is really a boolean from dbo SQL

Follow conventions for settings in the "settings" table

Read the naming convension for settings here: https://docs.elentra.org/technical/developers/elentra-settings#naming-convention

Ensure that the target branch is pointing to a supported version of Elentra

Elentra has a list of supported versions, ensure that the target branch is to a supported version of Elentra

To avoid confusion, database field names should NOT be the same as MySQL keywords

Do not use names that are the same as MySQL keywords, like "select", "order", "key", and so on. These will work, but must always be quoted with back-ticks; better to use "selection", "display_order", "item_key", or other names that will still work if not quoted.

SQL queries should use parameters to avoid SQL injection attacks

$query = 'Select * from table_name where param1=? and param2=?';

$db->GetAll($query, [$val1, $val2]);

Do not use document.createElement to create form elements in ME

All form elements must be created in the PHP template, and then manipulated via JS/jQuery.

If you're looking for something more robust, consider using mustache.js (bundled in ME), or even better, write your feature in EJS2