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