Coding Conventions

Braces and indentation

Braces and indentation

If you’re working on an existing file with an existing indentation style, continue using whatever is already in use. Definitely don’t mix indentation styles in the same file. If you are adding a new file to an existing project, try to keep the indentation style consistent with whatever the rest of it uses, or fix the entire file (in a separate commit) if that’s more appropriate.

C-like syntax (PHP, C, Java, etc)

  • Use 4-space tabs, and ensure the tabs are expanded to spaces.
  • For new files, use the Zend Coding Standard or the K&R style:
    int main(char a)
    {
        int i;
    
        if (a == 'g') {
            return 6;
        } else {
            for (i = 0; i < 10; i++) {
               ...
            }
    
            return 8;
        }
    }
  • Use type declarations in method signatures wherever possible. If you’re only expecting products, the relevant parameter should look like this: Mage_Catalog_Model_Product $product (for example).
  • There should always be a blank line between any license headers at the top of a file, and the first line of actual code. This follows the convention set out by core Magento, and ensures that IDEs will fold up the license header by default when loading a file.

CSS/SCSS

  • Use 4-space tabs, and ensure the tabs are expanded to spaces.
  • For new files, use the following style:
    /* Some divs */
    .class-selector div {
        padding: 0 17px 0 25px;
        line-height: 40px;
    }
    
    .another-class-selector div {
        border: 1px solid #f19900;
        background: #f9f3e3;
        float: left;
    }
    
    .test div {
        padding: 0 3px 0 2px;
    }
    
    /* Images */
    .product-image,
    .small-image,
    thumbnail {
        border-radius: 2px;
        padding-bottom: 5px;
    }
    
    /* Headings */
    h1 {
        font-size: 2em;
    }
    
    h2 {
        font-size: 1.2em;
    }

    All blocks should span over multiple lines, even if there is only one declaration in the block. There should only be one selector per line. Using comments to indicate what groups of rules relate to can make the file easier to understand.

    For existing files, use the same indentation style as the current content.

  • Avoid !important unless absolutely necessary. Always include a comment if you do need to use !importantstylelint will raise an error about any usage of !important – you can disable the lint on a per-usage basis, but disabling it must also include a comment.
  • When using the url() function, always put quotes around the path, like so:
    background: url('../images/button.png');
  • For SCSS, don’t let nested selectors get too long. For example, break up this:
    .product {
        .info {
            ...
        }
    
        .images {
            ...
        }
    
        ...100+ lines...
    
        .add-to-cart {
            ...
        }
    }

    into this:

    .product .info {
        ...
    }
    
    .product .images {
        ...
    }
    
    ...
    
    .product .add-to-cart {
        ...
    }

    Each nested selector should normally fit on one screen (i.e. 50-60 lines max).

Import statements

In general, the list of import statements must be sorted alphabetically.

PHP

Avoid any fully-qualified classnames (FQCNs) in your code. Always add import statements for any classes that are not in the same namespace as the current file, like so:

namespace MyNamespace\MyNestedNamespace;

use MyNamespace\Class1;
use MyNamespace\MyNestedNamespace\Class2;
use OtherNamespace\Class3;

Classnames in use statements must never have a leading namespace separator. They are completely redundant in this context.

Python

It is perfectly okay to import multiple objects from a module in a single import statement. The only guideline is to separate out imports for code inside and outside your application. Additionally, any relative imports must come last. Using dev-env as an example:

import click
import executor
from pathlib import Path

from dev_env.util import myfunc
from ..abstract import AbstractController

PHP

Array Syntax

PHP 5.4 introduced a new array syntax that matches similar functionality and syntax in other languages. Previously, this was your only option:

$myArray = array(1, 2, 3);

Now you can do this too:

$myArray = [1, 2, 3];

Please do not use the new style ([]) in any code that relates to Magento 1. Magento 1 codebases pre-date PHP 5.4, and exclusively use the old syntax.

For any code that isn’t related to Magento 1, feel free to use either syntax in any situation. We do not currently care about mixing the two styles, or using one style in particular.

Ternary Operators

  • In PHP, only use the ternary operator (... ? ... : ...) if it’s going to make things more readable, not less. This generally means only using it for very simple comparisons, and even then you should consider whether it’d be more appropriate to use if ... else.
  • PHP’s ternary operator has potential performance issues: it copies non-object values it’s comparing. Definitely don’t use it to switch on values from a large array. See these links for more information:
  • Nesting ternary operators is always a bad idea; don’t do it. (Especially in PHP, where they’re left-associative and so will do really counter-intuitive stuff unless you use parentheses everywhere.)

Logical operators andor

  • These operators are not aliases of && and ||.
  • They have different precedences to && and ||. For example:
    $bool1 = true && false;
    var_dump($bool1); // false
    $bool2 = true and false;
    var_dump($bool2); // true

    This is because ‘and/or’ have lower priority than ‘=’, whereas ‘&&/||’ have a higher priority.

References to primitive type names

This section refers to booleans and integers. When writing these type names for the purposes of casting, PHPDoc in docblocks, and type hints, it is possible to use two different names to represent them: the long form, and the short form:

  • bool vs boolean
  • int vs integer

In all cases, the shorter forms (int and bool) should be used. This is to ensure uniformity with type declarations in method signatures (introduced in PHP7) which only support the short form for primitive types.

Yoda expressions

Yoda expressions are a programming style where the two operands in an expression are reversed, such that the constant operand is on the left side of the operator, and the variable operand on the right. You can find more information about it at the following URL:

https://en.wikipedia.org/wiki/Yoda_conditions

Our policy is to not use yoda expressions at all in any language.

The only exception to this rule is when working on existing code where the convention is to use Yoda expressions (typically created by third parties). As always, you should always examine the existing code before writing your own to ensure you conform to the style already in use.

Use of deprecated functionality in future versions of languages

Just because we happen to be working with (for example) PHP 5.6, it does not mean that we should ignore deprecations and other changes that occur in future versions of the language. Over time, we will update to newer versions of all languages that we work with (sass, PHP, etc). By avoiding deprecated functionality when developing code for older versions, it makes upgrading easier. Therefore, it is important to pay attention to deprecations and other potentially breaking changes in newer versions as much as possible.

For example, in PHP 7.2, various pieces of functionality were deprecated (examples include create_function()__autoload()each(), and calling parse_str() without the second argument). This functionality should therefore be avoided no matter what, unless there is quite simply no other reasonable alternative available to you at the time.

Unchecked exceptions in PHP

The concept of “checked” and “unchecked” exceptions is one that comes from Java:

https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html

The idea is that there are various exception types that are not reasonable for you to handle in most of your code, and therefore can generally be ignored. They do not need to be specified in your docblocks, for example. Some common examples from Magento include:

  • Zend_Db_Exception: If this is thrown, it’s either because you’re still developing some new functionality and there’s an issue with your query, or because the connection to the database is literally broken. If it’s the latter, the site probably won’t be functioning at all, nevermind your tiny piece of non-critical functionality.
  • Mage_Core_Model_Store_Exception: You aren’t supposed to catch this. Magento catches it as one of its “catch everything” fallbacks, and has specific handling for it. If you were to worry about it, you’d be worrying about every single call to Mage::app()->getStore(), which would be overkill.
  • Varien_Exception: This is thrown in Varien_Object::__call() when you attempt to call a method on any instance of Varien_Object that isn’t concrete and doesn’t start with “get”, “set”, “has” or “uns”. However, PhpStorm isn’t able to determine that logic. Instead, it will now notice that you’re calling a non-concrete method on a class that has a magic __call() method, realise that __call() throws Varien_Exception, and warn you about it. This would mean acknowledging Varien_Exception in every single place that you use a magic getter or setter.
  • Fontis\MagentoMocker\MagentoMockerException: This is thrown by the Fontis Magento Mocker. It’s not something that you ever need to worry about in normal usage of Magento.
  • ReflectionException: If this is thrown, it’s probably because you’ve referenced a class/method/property that doesn’t exist. In this case, the solution is to fix the code, not attempt to catch the exception and then do something mysterious.

Fortunately, PhpStorm now has a mechanism for dealing with unchecked exceptions in PHP.

https://blog.jetbrains.com/phpstorm/2018/04/configurable-unchecked-exceptions/

It’s probably also worth adding RuntimeException to this list, as it’s the main inspiration behind the original concept of unchecked exceptions in Java.

By adding all of the above exception types to your unchecked exceptions list, your life will become a lot easier. Unfortunately, this list is per-project, so you’ll need to make sure that you a) configure this manually for all existing projects, and b) add it to your default settings as well.

Most frameworks, including Magento, will have various exception types that are

SEO keywords

  • code style
  • coding style
  • style guide
Advertisements

Fontis

Oliver De Guzman: Anyone know if we ever enabled facebook and paypal login for binglee? Or anyone know of a reason why they are disabled?

Lloyd Hazlett: they previously had google and my guess is they disabled it at some point recently after they’d reported it wasn’t working anymore: https://rt.fontis.com.au/Ticket/Display.html?id=19003 . Nigel probably forgot.

Oliver De Guzman: I see. He must have also forgotten that Facebook login was never enabled in the first place.

Oliver De Guzman: I don’t see any config changes disabling facebook login and there are no credentials configured for facebook login in the config

Oliver De Guzman: paypal login was disabled in 2016

Oliver De Guzman: they have an old version of fontis/sociallogin extension, even if I enabled facebook login on staging, I’m not able to login because facebook is not sending the email address as expected

Lloyd Hazlett: i’d say we just address google as per the original ticket

code review

Context:
Symptomp:
I tried:
I think:
Question:

the closing brace and the elseif must be on the same line.

I’m not sure there’s any benefit to translating this text.

Peter Spiller: are there any logs we could import into elasticsearch? that way we could use kibana to browse them, look for patterns, etc.

Lloyd Hazlett: the logs are useless though, they are impossibly verbose

Lloyd Hazlett: we have to do something though, what we have just does not stack up

Lloyd Hazlett: it isn’t really production-grade

Peter Spiller: are there any rules we could apply to filter the existing logs?

Peter Spiller: or could we redirect what currently goes into emails to a log?

Lloyd Hazlett: we’ll have to wait for Ron and Oliver to update us

Ronilo Carr: those errors emails at the moment are the output from the systemd services which is basically a stack trace of a fatal error. we can’t really make those show a friendly error message can only turn down the log level which will make it even less useful.

Peter Spiller: are they useful for debugging?

Ronilo Carr: it’s only useful for letting us know when something has happened as we usually have to look up the entire log in the database

Ronilo Carr: if we turn off the errors emails completely from the systemd services we basically won’t know when jobs are constantly running into fatal errors

Ronilo Carr: all other errors are already being logged in cartwheel

Oliver De Guzman: For the pentaho order export, we can ignore the pentaho errors since we check for the output of the job — eg. we have an alert when recent orders are not being exported

Ronilo Carr: those errors from hype stg do need our intervention, looking into it