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.

General Coding

Follow defined coding standards

New code is encapsulated in Classes

In ME, PHP 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

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 doc comment blocks, e.g. PHP Doc blocks, for your code (not necessary for getters and setters). Explain what the code does: what, how and why. Give examples of usage where it is helpful.

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 separate functions

Function names reflect their purpose and usage

A well-chosen function name can avoid the necessity of commenting what the function is doing, e.g. // Sum up the ages of members $age_sum = processMembers($part);

could be

$age_sum = sumMemberAges($part);

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.

Remove commented-out code

If code is not needed, remove it. Other developers will not know why it is there. Deleted code can always be retrieved using git.

Remove all @todo statements

Resolve any @todo items, or remove them. Work that needs to be done should be described in a Jira ticket.

Remove all 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 the language doesn't do under-the-hood type conversions that may not work as expected.

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

In the core product, do not reference files via CDN or other external sources.

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 ADODB

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 are looking for something more robust, consider using mustache.js (bundled in ME), or even better, write your feature in EJS2

PHP-specific Issues


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

As of PHP 8, calling a non-static method statically will throw a fatal error.

Use type hinting for function parameters and return values

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

HTML code should be outside of PHP tags

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

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

Check the PHP version in composer.json, and set that version in your IDE to be warned when using code for the wrong version.

Issues related to PHP 8

When comparing strings to numbers, the numbers are converted to strings before the comparison; previously strings were converted to numbers and then they were compared. The word "match" is now a keyword, so it can not be used as a constant or a class name. String concatenation in PHP 8 has a lower priority than mathematical operations. Nested ternary statements are not allowed without parentheses. If a "Magic" method has a type-hinted return value, PHP 8 will throw an error if the return value has the wrong type.

Elentra-specific Issues


New API endpoints should be created in Elentra API

We are 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

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.

All text strings exposed to users are translated

Use translate functions such as $translate→_() in ME code

Migrations must be named correctly

Use the ticket number, so that the filename follows the format:

[yyyy]_[mm]_[dd]_[time]_[ticket] Example: 2019_03_03_114947_ME1074.php

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

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.

Follow conventions for settings in the "settings" table

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.

Database-specific Issues


SQL queries should use parameters to avoid SQL injection attacks

$query = 'Select * from table_name where param1=? and param2=?'; $db->GetAll($query, [$v1, $v2]);

Do not use database version-specific features

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.

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.

Versioning-specific Issues


Fix version should match target branch

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

Hot-fixes 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. e.g. 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 Pull Request. Before submitting a Pull Request, you should merge the latest version of the branch you are merging with, to detect merge conflicts as early as possible.

Last updated