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	https://docs.elentra.org/technical/developers/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

Rule	Details
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

Rule	Details
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	Read the naming convention 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.

Database-specific Issues

Rule	Details
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

Rule	Details
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.