Paysera PHP style guide extends PSR-1 and PSR-12, so code must also follow these standards to be compatible with this style guide.
Guide is divided into several parts:
- PHP Basics. This defines additional style rules for the code, some restrictions for basic PHP usage;
- Main patterns. This describes patterns and architecture decisions that must be followed;
- Symfony related conventions. As Paysera use Symfony as the main framework, this chapter describes rules to be used when developing for this framework;
- Composer conventions. Describes usage rules related with composer: releasing libraries or requiring ones.
- PHP Basics
- Basics
- Code style
- Usage of PHP features
- Condition results
- Logical operators
- Strict comparison operators
- Converting to boolean
- Comparing to boolean
- Comparing to
null
- Assignments in conditions
- Unnecessary variables
- Reusing variables
- Unnecessary structures
- Static methods
- Converting to string
- Double quotes
- Visibility
- Functions
str_replace
- Return and argument types
- Type-hinting classes and interfaces
- Dates
- Exceptions
- Checking things explicitly
- Calling parent constructor
- Traits
- Arrays
- Default property values
- Scalar typehints
- Void typehints
- Comments
- IDE Warnings
- Main patterns
- Symfony related conventions
- Composer Conventions
For our libraries we use lowest PHP version that is used in any of currently actively maintained projects.
We do not use global variables, constants and functions.
We put every class to it’s own file.
If we modify legacy code where some other conventions are used, we can use the same style as it is used there.
If we split array items into separate lines, each item comes in it’s own line. Comma must be after each (including last) element.
<?php
['a', 'b', 'c'];
[
'a',
'b',
'c', // notice the comma here
];
Why?
- each element in it's own line allows to easily scan all the elements - more understanding, less bugs;
- comma after last element allows to easily reorder or add new elements to the end of the array.
If we split statement into separate lines, we use these rules:
-
&&
,||
etc. comes in the beginning of the line, not the end; -
if we split some condition into separate lines, every part comes in it’s separate line (including first - on the new line, and last - new line after). All of the parts are indented.
Some examples:
<?php
return ($a && $b || $c && $d);
return (
$a && $b
|| $c && $d
);
return ((
$a
&& $b
) || (
$c
&& $d
));
return (
(
$a
&& $b
)
|| (
$c
&& $d
)
);
return ($a && in_array($b, [1, 2, 3]));
return (
$a
&& in_array($b, [1, 2, 3])
);
return ($a && in_array(
$b,
[1, 2, 3]
));
return ($a && in_array($b, [
1,
2,
3,
]));
return ($a && in_array(
$b,
[
1,
2,
3,
]
));
return (
$a
&& in_array(
$b,
[
1,
2,
3,
]
)
);
This is wrong:
<?php
return [
'a', // use 4 spaces, not 8 here
'b',
];
When making chain method calls, we put semicolon on it’s own separate line, each of chained method calls is indented and comes in it’s own line.
Example:
<?php
return $this->createQueryBuilder('a')
->join('a.items', 'i')
->andWhere('i.param = :param')
->setParameter('param', $param)
->getQuery()
->getResult()
; // semicolon here
Why? Same as with arrays: allows to easily scan all the calls, reorder or add new ones when needed.
We always add ()
when constructing class, even if constructor takes no arguments:
<?php
$value = new ValueClass();
We use full names, not abbreviations: $entityManager
instead of $em
, $exception
instead of $e
.
We use nouns for class names.
For services we use some suffix to represent the job of that service, usually *er:
-
manager
-
normalizer
-
provider
-
updater
-
controller
-
registry
-
resolver
We do not use service
as a suffix, as this does not represent anything (for example PageService
).
We use object names only for entities (value objects), not for services (for example Page
).
We always add suffix Interface
to interfaces, even if interface name would be adjective.
Why? If we have base class witch implements the interface, we would have name clash. For example,
ContainerAware
andContainerAwareInterface
.
We use nouns or adjectives for property names, not verbs or questions.
<?php
class Entity
{
private $valid; // NOT $isValid
private $checkNeeded; // NOT $check
}
We use verbs for methods that perform action and/or return something, questions only for methods which return boolean.
Questions start with has
, is
, can
- these cannot make any side-effect and always return boolean
.
For entities we use is*
or are*
for boolean getters, get*
for other getters, set*
for setters,
add*
for adders, remove*
for removers.
We always make correct English phrase from method names, this is more important that naming method to
'is' + propertyName
.
<?php
interface EntityInterface
{
public function isValid();
public function isCheckNeeded();
public function getSomeValue();
public function canBeChecked();
public function areTransactionsIncluded();
// WRONG:
public function isCheck();
public function isNeedsChecking();
public function isTransactionsIncluded();
public function getIsValid();
}
<?php
interface ControllerInterface
{
/**
* @return boolean
*/
public function canAccess($groupId);
/**
* @throws AccessDeniedException
*/
public function checkPermissions($groupId);
}
We provide methods and fields in the following order:
- constants
- static fields
- fields
- constructor
- constructing class methods
- static methods
- class methods
Constructing class methods are those used in service construction phase, usually by dependency injection container.
Why no ordering by visibility?
protected
andprivate
methods usually are used from single place in the code, so ordering by functionality (versus by visibility) makes understanding the class easier. If we just want to see class public interface, we can use IDE features (method names ordered by visibility or/and name, including or excluding inherited methods etc.)
We use singular for namespaces: Service
, Bundle
, Entity
, Controller
etc.
Exception: if English word does not have singular form.
We do not make directories just for interfaces, we put them together with services by related functionality
(no ServiceInterface
namespace).
We do not use service names for namespaces, instead we use abstractions.
For example, namespace should be UserMerge
or UserMerging
, not UserMergeManager
.
If we compare to static value (true
, false
, null
, hard-coded integer or string),
we put static value in the right of comparison operator.
Wrong: null === $something
Right: $something === null
Why? Speak clearly to be understood correctly, you should. Yeesssssss.
If class has a namespace, we use use
statements instead of providing full namespace.
This applies to PhpDoc comments, too.
Wrong:
<?php
namespace Some\Namespace;
// ...
/**
* @var \Vendor\Namespace\Entity\Value $value
*/
public function setSomething(\Vendor\Namespace\Entity\Value $value);
Right:
<?php
namespace Some\Namespace;
use Vendor\Namespace\Entity\Value;
/**
* @var Value $value
*/
public function setSomething(Value $value);
If condition result is boolean, we do not use condition at all.
Do not do this:
<?php
$a = $d && $e ? false : true;
if ($a && $b) {
return true;
}
return false;
Do this:
<?php
$a = !($d && $e);
return $a && $b;
We use &&
and ||
instead of and
and or
.
Why?
and
andor
have different priorities and commonly leads to unintended consequences
We use ===
(!==
) instead of ==
(!=
) everywhere. If we want to compare without checking the type, we should
cast both of the arguments (for example to a string) and compare with ===
.
Same applies for in_array
- we always pass third argument as true
for strict checking.
We convert or validate types when data is entering the system - on normalizers, forms or controllers.
Why? Due to security reasons and to avoid bugs
php > var_dump(md5('240610708') == md5('QNKCDZO'));
bool(true)
php > file_put_contents('/tmp/a', '<?xml version="1.0" encoding="UTF-8"?><B4B xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://swedbankgateway.net/valid/hgw-response.xsd"><Alert><AccountInfo><IBAN>abc</IBAN></AccountInfo></Alert></B4B>');
php > $a = simplexml_load_file('/tmp/a');
php> var_dump($a == false);
bool(true)
php > var_dump(in_array(1, ['1a2b']));
bool(true)
php > var_dump(in_array(true, ['a']));
bool(true)
We use type casting operator to convert between types:
<?php
return (bool)$something;
This should not be used at all. If variable is not
boolean
already, check it’s available values explicitly.
We do not use true
/false
keywords when checking variable which is already boolean
.
Wrong:
<?php
return $valid === true;
return $valid === false;
return false !== $valid;
Correct:
<?php
return $valid;
return !$valid;
Exception: When variable can be not only boolean, but also int
or null
.
<?php
return strpos('needle', 'haystack') === false;
When comparing to null
, we always compare explicitly.
<?php
function func(ValueClass $value = null)
{
return $value !== null ? $value->getSomething() : null;
}
We also do not use is_null
function for comparing.
We do not use assignments inside conditional statements.
Exception: in a while
loop condition.
Wrong:
<?php
if (($b = $a->get()) !== null && ($c = $b->get()) !== null) {
$c->do();
}
if ($project = $this->findProject()) {
}
Correct:
<?php
$b = $a->get();
if ($b !== null) {
$c = $b->get();
if ($c !== null) {
$c->do();
}
}
$project = $this->findProject();
if ($project !== null) {
}
Why? We save a few lines of code but code is less understandable - we make several actions at once. Furthermore, as we explicitly compare to
null
, conditional-assignment statements become complicated.
We avoid unnecessary variables.
Wrong:
<?php
function find($needle, $haystack)
{
$found = false;
foreach ($haystack as $item) {
if ($needle === $item) {
$found = true;
break;
}
}
return $found;
}
function getSomething()
{
$a = get();
return $a;
}
Correct:
<?php
function find($needle, $haystack)
{
foreach ($haystack as $item) {
if ($needle === $item) {
return true;
}
}
return false;
}
function getSomething()
{
return get();
}
This does not mean that we cannot use them to make code more understandable. For example, this way of variable usage is fine:
<?php
function canModify($object)
{
$rightsGranted = isAdmin() || isOwner($object);
$objectEditable = isNew($object) && !isLocked($object);
return $rightsGranted && $objectEditable;
}
We do not set value to variable passed as an argument.
We do not change the type of any variable.
Instead, in both cases, we create new variable with different name.
<?php
public function thisIsWrong($number, $text, Request $request)
{
$number = (int)$number; // Illegal: we 1) change argument value 2) change it's type
$text .= ' '; // Illegal: we change argument value
$document = $request->get('documentId');
$document = $this->repository->find($document); // Illegal: we change variable's type
// ...
}
public function thisIsCorrect($numberText, $text, Request $request)
{
$number = (int)$numberText;
$modifiedText = $text . ' ';
$documentId = $request->get('documentId');
$document = $this->repository->find($documentId);
// ...
}
Why? To understand code better (quicker, easier) and thus to avoid bugs. This also integrated with IDE better so it can give better static analysis support
We avoid unnecessary structures.
Wrong:
<?php
if ($first) {
if ($second) {
do();
}
}
Correct:
<?php
if ($first && $second) {
do();
}
We do use static methods only in these cases:
-
to create an entity for fluent interface, if PHP version in the project is lower thant 5.4. We use
(new Entity())->set('a')
in 5.4 or above -
to give available values for some field of an entity, used in validation
We do not use __toString
method for main functionality, only for debugging purposes.
Why? We cannot throw exceptions in this method, we cannot put it in interface, we cannot change return type or give any arguments - refactoring is impossible (only by changing it to simple method). Also if debugging uses __toString method, we cannot change it to get any additional information.
We do not use double quotes in simple strings.
We use double quotes only under these conditions:
- Single quote is used repeatedly inside the string
- Some special symbols are used, like
"\n"
If we use double quotes, we do not use auto variable includes ("Hello $name!"
or "Hello {$object->name}!"
).
We don’t use public properties. We avoid magic methods and dynamic properties - all used properties must be defined.
Example:
<?php
class A {
private $a; // this must be defined as a property
public $b; // this is illegal
public static $availableValues = ['a', 'b']; // this is illegal
public function __construct($a)
{
$this->a = $a;
}
}
We prefer private
over protected
over public
as it constraints the scope - it's easier to refactor, find usages,
plan possible changes in code. Also IDE can warn about unused methods or properties.
We use protected
when we intend some property or method to be overwritten if necessary in extending classes.
We use public
visibility only when method is called from outside of class.
We use count
instead of sizeof
.
We use compare to null
using ===
instead of is_null
function. For example:
if ($result === null) {
// ...
We do not use str_replace
if we need to remove prefix or suffix - this can lead to replacing more
content unintentionally.
<?php
function removePrefix($text, $prefix)
{
if (substr($text, 0, strlen($prefix)) === $prefix) { // strpos === 0 is also possible
$text = substr($text, strlen($prefix));
}
return $text;
}
assertSame('Some asd text', removePrefix('asdSome asd text', 'asd'));
We can also use preg_replace
for this.
We always return value of one type. Optionally, we can return null
when using any other return type, too.
For example, we cannot return boolean|string
or SuccessResult|FailureResult
(if SuccessResult
and FailureResult
has no common class or interface; if they do, we document to return that
interface instead).
We can return SomeClass|null
or string|null
.
Note: when we are always returning the same class but it's iterable of other classes, we can (and should)
provide Collection|SomeClass[]|null
as return value.
Same applies for any argument.
Exception: When making query from repository, both int|Entity
can be taken (object or it’s ID).
We give priority to object in this case if it’s available.
Usually this should not be done as it would probably violate some other convention.
If argument is optional, we provide default value for it.
If optional argument is object, we type-hint it with required class and add default to null
.
If argument is not optional, but just nullable, we can typehint it with default value null
, but when using,
we pass null
explicitly.
Example:
<?php
class Service
{
public function __construct(SomeService $s, LoggerInterface $logger = null)
{
}
}
<?php
class Entity
{
/**
* @param ValueClass|null $value
*/
public function setValue(ValueClass $value = null)
{
}
}
$entity->setValue($someValue);
$entity->setValue(null);
// but we do not use $entity->setValue();
For PHP 7.1 and above, we must use nullable type hints instead:
<?php
class Entity
{
/**
* @param ValueClass|null $value
*/
public function setValue(?ValueClass $value)
{
}
}
We always return something or return nothing. If method does not return anything ("returns" void
),
we do not return null
, false
or any other value in that case.
If method must return some value, we always specify what to return, even when returning null
.
Wrong:
<?php
function makeSomething()
{
if (success()) {
return true;
}
}
function getValue(MyObject $object)
{
if (!$object->has()) {
return;
}
return $object->get();
}
/**
* @throws PaybackUnavailableException
*/
function payback($requestId)
{
makePayback($requestId);
return true;
}
Correct:
<?php
function makeSomething()
{
if (success()) {
return true;
}
return null;
}
function getValue(MyObject $object)
{
if (!$object->has()) {
return null;
}
return $object->get();
}
/**
* @throws PaybackUnavailableException
*/
function payback($requestId)
{
makePayback($requestId);
}
Why? If method result is not used and you return something anyway, other programmer may assert that return value is indeed taken into account. For example, return
false
on failure, even if this failure is not handled anyhow by the functionality that's calling your method.
If we declare some class property type as not nullable, after that class object construction it should never be or become null.
To rephrase from other perspective – if property is not initialized in the constructor, it's type must be nullable.
This also applies for typehints in PhpDoc of properties.
Examples:
<?php
declare(strict_types=1);
class SomeClass
{
/**
* @var int|null –> this must include `|null`, as `$id` can be uninitialized
*/
private $id;
public function getId(): ?int // return value here must be nullable
{
return $this->id;
}
/**
* @return int|null
*/
public function getIdBefore71()
{
return $this->id;
}
public function setId(int $id): self // we can use non-nullable type for setter, though
{
$this->id = $id;
return $this;
}
}
<?php
declare(strict_types=1);
class SomeClass
{
/**
* @var Child|null
*/
private $child;
public function getChild(): Child
{
if ($this->child === null) {
throw new RuntimeException('child is not initialized yet');
}
return $this->child;
}
public function hasChild(): bool
{
return $this->child !== null;
}
}
We always type-hint narrowest possible interface which we use inside the function or class.
Why? This allows us to refactor easier. We can just provide another class, which implements the same interface. We can also find real usages quicker if we want to change the interface itself (for example
RouterInterface
vsUrlGeneratorInterface
when we change declaration ofRouterInterface::matches
).
If we have dependency on service with several responsibilities (which implements several interfaces), we should inject it twice. For example:
class ResultNormalizer implements NormalizerInterface, DenormalizerInterface
{
// ...
}
class MyService
{
public function __construct(
NormalizerInterface $normalizer,
DenormalizerInterface $denormalizer
) {
// ...
}
// ...
}
$resultNormalizer = new ResultNormalizer();
$myService = new MyService($resultNormalizer, $resultNormalizer);
We use \DateTimeImmutabble
objects to represent date or date and time inside system.
We use \DateTimeInterface
where we get the date to be more compatible with functionality that still operates
\DateTime
objects.
We never throw base \Exception
class except if we don’t intend for it to be caught.
We never catch base \Exception
class except where it’s thrown from vendor code.
In any case, we never catch exception if there are few throwing places possible and we only expect one of them.
Wrong:
<?php
try {
// code
// some more code
$this->service->someDeepMethod();
// more code here
} catch (\Exception $exception) {
return null; // not found
}
Why? Because usually in these situations tens or hundreds different situations could have gone wrong but we make assumption that it was that single one. This is how we not only make the system behave incorrect on unexpected failures, but also ignore the real problems by not logging them.
We use only functions or conditions that are designed for specific task we are trying to accomplish. We don’t use unrelated features, even if they give required result with less code.
We avoid side-effects even if in current situation they are practically impossible.
Some examples:
-
we use
isset
versusempty
if we only want to check if array element is defined; -
we use
$x !== ''
instead ofstrlen($x) > 0
- length of$x
has nothing to do with what we are trying to check here, even if it gives us needed result; -
we use
count($array) > 0
to check if array is not empty and not!empty($array)
, as we do not want to check whether$array
is0
,false
,''
or even not defined at all (in which case IDE would possibly hide some warnings that could help noticing possible bugs).
Why? We avoid side-effects if some related code changes or is refactored. Code is much easier to understand if we see specific checks or function calls instead of something unrelated that happens to give the needed result.
If we need to call parent constructor, we do it as first statement in constructor. For example:
public function __construct($arg1, $arg2)
{
parent::__construct($arg1);
$this->setArg2($arg2);
}
Why? Parent class can have some mandatory stuff to do before we can use it's functionality. See following example. This is the only way to call parent constructor in some (probably most) of other languages (etc. JAVA)
Example of parent class, with which calling constructor later would fail:
protected $params;
public function __construct($arg1)
{
$this->params = [$arg1];
}
public function setArg2($arg2)
{
$this->params[] = $arg2;
}
The only valid case for traits is in unit test classes. We avoid using traits in our base code.
Why? We use (classical) OOP to refactor functionality to avoid duplicated code.
We always use short array syntax ([1, 2]
instead of array(1, 2)
) where PHP code-level allows it.
If we need to define some default value for class property, we do this in constructor, not in property declaration.
Why? Some default values cannot be set when declaring properties, so this way everything is in one place. It especially makes sense when entity has lots of properties.
class MyClass
{
const TYPE_TWO = 'two';
/**
* @var ArrayCollection
*/
private $one;
/*
* @var string
*/
private $two;
/*
* @var int
*/
private $three;
/*
* @var bool
*/
private $four;
/*
* @var SomeObject
*/
private $five;
/*
* @var \DateTime
*/
private $createdAt;
public function __construct()
{
$this->one = new ArrayCollection();
$this->two = self::TYPE_TWO;
$this->three = 3;
$this->four = false;
$this->createdAt = new \DateTime();
}
}
For newly written files we always add declare(strict_types=1);
at the top of the file.
We add it for already existing files if we can guarantee that no negative side-effects would be caused by it. It's safer to type-cast variables in the file to the correct scalar types to keep the consistent behavior.
We always use scalar typehints both for arguments and return types, unless our code with them would not work as intended so we are forced not to use them.
For already existing methods we can add scalar typehints but we're responsible to guarantee that any place in the project that has strict types declaration and calls this method always passes the correct type as an argument. We could add typecast in the places where we're not guaranteed to keep the consistent behavior.
If we add scalar typehint for any argument in a public method in our library, this means that major release is needed. Scalar typehints can be always safely added to return values if needed, though.
<?php
declare(strict_types=1); // this is required for new files
class Something
{
/**
* @var int|null
*/
private $b;
public function increment(int $a): int //if we can declare it, we declare types both for arguments and return type
{
return $a + 1;
}
public function setBPhp70Style(int $b = null): self //PHP 7.0 only
{
$this->b = $b;
return $this;
}
public function setBPhp71AndUpStyle(?int $b): self //PHP 7.1 and up
{
$this->b = $b;
return $this;
}
/**
* @return int|null
*/
public function getBPhp70Style() //We can not provide nullable return type hint in PHP 7.0
{
return $this->b;
}
public function getBPhp71AndUpStyle(): ?int //PHP 7.1 and up supports nullable types so we use them
{
return $this->b;
}
}
// some old interface from some old library we have to implement
interface SomeOldInterface
{
/**
* @param string $withSomething
* @return Result[]
*/
public function doOldStuff($withSomething);
}
class OurShinyNewClass implements SomeOldInterface
{
/**
* @param string $withSomething
* @return Result[]
*/
public function doOldStuff($withSomething): array // we can not declare argument type, but we can narrow return type
{
return [];
}
/**
* @param Result[] $results
* @return int[]
*/
public function getSomeIntProperty(array $results): array
{
return array_map(
function (Result $result): int {
return $result->getSomeIntProperty();
},
$results
);
}
}
For functions and methods that do not return value, we should typehint return value as : void
.
<?php
declare(strict_types=1);
class SomeClass
{
public function doSomething(): void // we must typehint return value as `void` here as method does not return anything
{
echo "Hi!";
}
public function returnMixed() // no typehint will be used only for cases where we do not guarantee some exact return type
{
return $_GET['something'];
}
}
We put a PhpDoc comment on methods where the return and argument types are not type-hinted in the method signature. For example, prior to PHP 7.0, we would need a PhpDoc to type-hint scalar types. In PHP 7, the scalar types would be strictly typed and PhpDoc is not needed.
If PhpDoc does not add any other information that's already provided with typehints, we don't use PhpDoc at all.
Wrong:
/**
* @param Client $client
* @param string $type
*
* @return Money
*/
public function getLimit(Client $client, string $type): Money;
public function setNumber($number);
Correct:
public function getLimit(Client $client, string $type): Money;
/**
* @param int $number
*/
public function setNumber($number);
Why? Any comment needs to be maintained – if we add parameter, change parameter or return type, we must also update the PhpDoc. If PhpDoc does not add any additional information, it just duplicates information that's already provided.
If method description, parameter descriptions or any other PhpDoc tags other than @param
and @return
are used
we will add a full PhpDoc like the following:
/**
* Gets a day limit starting from midnight.
*
* @see http://example.com/
*
* @param Client $client
*
* @return Money
*/
public function getDayLimit(Client $client): Money;
When we take as an argument or return an array of strictly typed elements, we should add a PhpDoc to describe the type of elements in the array.
/**
* @param Client[]|array $clients
*
* @return Money
*/
public function getDayLimitForClients(array $clients): Money;
A full PhpDoc is required when at least one of the parameters or the return type is not strictly typed:
/**
* @param Client $client
*
* @return Money|null
*/
public function getDayLimit(Client $client);
Starting with PHP 7.1, nullable return types can be type-hinted in the code, so PhpDoc is not necessary:
public function getDayLimit(Client $client): ?Money;
If we use PhpDoc comment, it must contain all information about parameters, return type and exceptions that the method
throws. If method does not return anything, we skip @return
tag.
sometimes scalar types are not autocompleted by IDE, but must be provided by this requirement. Example:
@param string $param1
We use PhpDoc on properties that are not injected via constructor.
We do not put PhpDoc on services, that are type-casted and injected via constructor, as they are automatically recognised by IDE and desynchronization between typecast and PhpDoc can cause warnings to be silenced.
If property value is not set in constructor, we must always add |null
to the PhpDoc of that property.
If method returns $this
, we use @return $this
that IDE could guess correct type if we use this method
for objects of sub-classes.
If we return or take as an argument value that has few types or can be one of few types,
we provide all of them separated by |
.
<?php
/**
* Description of the method - optional
*
* @param string $param1 description of param1. Description is optional, type is required (string)
*
* @return SomeObject[]|Collection|null Optional description of return type
*/
public function getSomething($param1)
{
// ...
return $collection;
}
Why? This way we
- know if
null
value can be returned or passed as an argument;- can use methods with auto-completion from both of specified types. This is very convenient for collections - we can use
$collection->count()
andforeach ($collection as $a) {$a->someMethod();}
If method is deprecated, we mark this in PhpDoc as @deprecated
with description or additional
@see
with new method to use. For example:
@deprecated use DI to create and inject this service
@deprecated
@see \Acme\Component\SomeComponent::someMethod
We do not add file comments.
Why? We use class comments - they are used if PhpDoc is auto-generated from PHP code, also can be used by IDE.
We do not use @package
, @namespace
, @date
or @author
annotations anywhere.
Why? Namespace is provided by PHP keyword. Both date and author can be seen via annotations and becomes stale really quickly. After refactoring, the only name in the file can be totally irrelevant.
We do not use @inheritdoc
.
Why? It does not contain information for programmer. For IDE, no PhpDoc at all has the same results.
We use multi-line /** */
comments for method, property and class annotations.
We use single-line /** @var Class $object */
annotation for local variables. We always avoid this if possible -
usually PhpDoc is enough (sometimes it's missing, for example in vendor code).
We can use //
single line comments in the code.
We do not use /* */
or #
comments at all.
Why no multi-line comment? We try to keep functions small and comment them in PhpDoc. If some things are to be explained in function body, single line comments should be enough. If we want to comment-out something, we just delete it (and use VCS to revert if needed) or disable functionality in configuration.
Some examples:
<?php
class NewsletterSender
{
/**
* @var MailFactory this must be multiline - 3 lines, not 1
*/
private $mailFactory;
public function sendNewsletter(Newsletter $newsletter)
{
/** @var Mail $mail this must be single line - 1 line, not 3 */
$mail = $this->mailFactory->createNamedMail($newsletter->getName());
// we can add single line comments to explain behavior
}
}
We always prefer refactoring code to be understandable over adding some comments to explain it's behavior. Most of the cases, code should be clear enough to not need any comments at all.
Why should we avoid comments? They are technical debt - if you cannot keep it updated, it gets stale real quick. When you cannot trust some of the comments, you don't know which ones you can really trust.
We always avoid IDE warnings if possible (it's not possible only when there is an IDE bug or some similar case).
We provide @var
PhpDoc comment if IDE cannot guess the type of variable and we cannot fix method declaration for
type to be correctly guessed (vendor code etc.)
We use Plain Value Objects (we name them Entities) for moving information/data around functionality.
We do not put any logic into those objects - all logic is handled outside, in services.
This allows to change and configure behaviour in different context.
In general we try to make services without run-time state. That is, once configured, it should not change it’s behaviour in run-time.
This does not apply to specific services, which have, for example, to collect some events and redispatch them in the end of request etc. - in those cases where collecting the state in run-time is their primary objective.
Instead of:
<?php
class BadExample
{
protected $name;
public function setManagerName($name)
{
$this->name = $name;
}
public function search()
{
// do something with $this->name
}
}
we use:
<?php
class GoodExample
{
public function search($managerName)
{
// ...
}
}
Why? When services become context-aware unintentionally, this makes unpredicted side-effects. More bugs, which can be very hard to debug and/or test. Much more test scenarios. Much harder to refactor.
This is similar to using globals - if we store run-time context, we become unaware of all the things that can influence the behavior.
We always try to use composition instead of inheritance.
If we want to make some abstract method - we inject interface into constructor with that method.
This helps to follow single responsibility principle. Also this often allows to configure object by injecting different functionality without explosion of classes. If we have 2 abstract methods and 2 possible logic for each of them, we would need 4 classes with 8 methods total, also having some duplicated code. If we inject services, we only need 4 classes with 1 method each with no duplicated code.
Also, this allows to test code much easier, as we can test each functionality separately.
This is related to composition over inheritance - we try to configure each service instead of hard-coding any of parameters in the code itself.
For example, instead of:
class ServiceA
{
public function doSomething()
{
$a = Manager::getInstance();
$a->doSomething('a');
}
}
class ServiceB
{
public function doSomething()
{
$a = Manager::getInstance();
$a->doSomething('b');
}
}
$a = new ServiceA();
$b = new ServiceB();
we use:
class Service
{
private $manager;
public function __construct(Manager $manager)
{
$this->manager = $manager;
}
public function doSomething()
{
$this->manager->doSomething();
}
}
$a = new Service(new Manager('a')); // we use Dependency Injection Container for creating services, this is just an example
$b = new Service(new Manager('b'));
This allows to reuse already tested code in different scenarios just by reconfiguring it.
Of course, we still need to test the configuration itself (functional/integration testing), as it gets more complicated.
We do not store configuration in constants.
Why? As configuration can change, it is not constant. Possible situation:
const SOFT = 'soft'; const HARD = 'soft';
We try to write code that is self-explanatory. This implies writing small methods - we should understand what the method does by looking at it's code. We also try to name methods by their meaning, which also helps understanding the code.
We always take into account component or bundle dependencies.
No unnecessary dependencies between components or bundles must be created.
If there is some abstract functionality, it must not depend upon it’s implementations.
No circular dependencies between components or bundles must be created.
Services can be created only by factory classes or dependency injection container. We do not create services in other services not dedicated to do that (for example, in constructor).
We make changes to entity state in managers or some similar designated services.
Manager can make the following actions:
- check initial entity state before changing it, validate it;
- change the state;
- dispatch event;
- make some additional actions.
If state is changed in a controller or some other place, duplicated code can occur when functionality is required in another place. With duplicated code, different behavior can occur, which eventually leads to bugs.
We prefer concrete methods instead of one abstract method to change state.
This allows us to see available actions and make our code clearer:
no switch
statements, event maps, complex validation rules, nested `if`s or similar magic.
<?php
class Manager
{
// simple short methods with clear logic:
public function accept(Request $request)
{
$this->assertPending($request);
$request->setStatus(Request::STATUS_ACCEPTED);
$this->eventDispatcher->dispatch(RequestEvents::ACCEPTED, new RequestEvent($request));
}
public function deny(Request $request)
{
$this->assertPending($request);
$request->setStatus(Request::STATUS_DENY);
$this->eventDispatcher->dispatch(RequestEvents::DENIED, new RequestEvent($request));
}
// more complicated and totally unclear from interface
public function changeStatus(Request $request, $status)
{
$this->assertPending($request);
switch ($status) {
case Request::STATUS_ACCEPTED:
$eventName = RequestEvents::ACCEPTED;
break;
case Request::STATUS_DENY:
$eventName = RequestEvents::DENIED;
break;
default:
throw new \InvalidArgumentException('Invalid status provided ' . $status);
}
$request->setStatus($status);
$this->eventDispatcher->dispatch($eventName, new RequestEvent($request));
}
}
To make example more complicated, we can imagine that different validation groups must be provided for validator
for each of the given status and some other event class should be provided if request is denied.
Furthermore, denial could take one more argument - DenialReason $reason
.
We use objects to represent data when possible. For example, if data can be represented by both scalar value and an object (like date), we use object. When several variables belong to single item, we use an object representing them and do not pass or operate on these separately (like money object versus amount and currency as separate variables).
We always try to use single representation of that data item - we avoid having multiple classes which represent the same object in the system (for example Transfer as DTO in common library and as an Entity - these could both co-exist, but we try to have an Entity as soon as possible in the system).
We normalize data as soon as possible in the system (input level) and denormalize it as later as possible (output level). This means that in service level, where our business logic resides, we always operate on these objects and not their concrete representations.
We operate with objects inside services, not their identifiers.
Objects must be resolved by their identifiers in controller, normalizer or similar input layer.
In business objects (entities or simple DTOs, like Filter
) we already have other objects, taken from database.
If we do not find object by identifier, we throw exception and return 404
as a response
(or specific error code with 400
).
In some cases we use integer UNIX timestamp to represent date with time.
When creating \DateTime
, we must not use constructor with @
as it ignores the time zone of the server.
For example:
<?php
$createdAt = new \DateTime();
$createdAt->setTimestamp($data['created_at']);
$entity->setCreatedAt($createdAt);
Always amount and currency, never only amount.
If there is functionality that can be implemented in different ways and should be selected at runtime, we create Interface for it and use Manager (or separate Registry class) to collect all those services that implement it.
From code, we do not call those services directly, we call Manager which chooses the correct service and redirects call to it.
This way if Interface changes, we only have to make changes in the Manager. Also, if we need to do some action every time some method is called, we can add it to manager, no need to put it to all those services.
Example:
class Manager
{
protected $providers = [];
public function addProvider(ProviderInterface $provider, $providerKey)
{
$this->providers[$providerKey] = $provider;
}
public function doSomething(Data $data)
{
// todo: check if provider with such key exists
return $this->providers[$data->getProviderKey()]->doSomething($data);
}
}
We use dependency injection container tags to add all those services to the manager.
We use events to notify about some result or to optionally change behaviour before making some action.
We do not use events to actually make some action happen, which is mandatory for the place which dispatches the event.
We should not make assumptions about listeners of some event. If we require a result from event listeners, this indicates that we should refactor functionality and use interfaces with tags or some similar solution.
Sensitive values are those that are secret and should not be known or read in plain text. For example passwords, tokens, secret generated codes etc. Any values that can affect security of the system.
We never log content where sensitive values can be found. For example, request content for all requests, as these include call to authentication. We must either
- do not log at all;
- make whitelist what to log;
- make blacklist when to avoid logging (this is not recommended as can be easily forgotten).
When we configure loggers, we always use normalizers that does not extract private fields from any given objects by default.
As we log exception stack trace, any scalar values passed as arguments can be used in stack trace.
To avoid that, we put sensitive values to SensitiveValue
object as soon as they enter our system.
We pass them as this object. We get the value itself only in lowest level possible.
If we cannot pass SensitiveValue
(for example, to vendor library), we catch \Exception
and re-throw a new one,
without providing last exception from before (we can copy the message itself, if it does not contain sensitive values).
If we really need to store sensitive value in an entity, we make that property private.
We also make that setter and getter would operate with SensitiveValue
and not scalar type itself.
Better approach is to always use hash of sensitive values, if we just need to check if provided value matches it. If we need to resend generated code or use original value later, only in that case we save generated value (like code) in plain-text.
This chapter describes conventions related directly with Symfony framework and related components (Doctrine, Twig).
It also includes some closely related conventions that can be used independently (for example in libraries).
We use XML configuration inside bundles.
Why not YAML? We use XML, because it is explicit and has schema definition - many mistakes can be avoided.
Why not annotations? We don’t use annotations, as we want to leave configuration and logic separate. Moreover, class and object/service are not the same, this makes it difficult to configure several services for a single class.
We use YAML configuration inside app
directory for semantic configuration files.
Why? Configuration is semantic here and thus not very complex. Most of the examples use only this format, it’s default for Symfony standard distribution.
We still use XML for service definitions inside app
directory.
We use {origName}.dist.{origExt}
name for distribution versions of parameters.
Why? Original extension is left the same so we can use all IDE features. It is configurable (or not important) so we can do that.
We do not put bundle prefix to parameters that we only use outside the bundle, for example to configure
config.yml
file. Treat them as private to that bundle.
Incorrect:
config.yml
:
acme_cool_feature:
parameter: %acme_cool_feature.parameter%
option: %acme_cool_feature.option%
parameters.dist.yml
:
acme_cool_feature.parameter: A
acme_cool_feature.option: B
Correct:
config.yml
:
acme_cool_feature:
parameter: %cool_feature_parameter%
option: %cool_feature_option%
parameters.dist.yml
:
cool_feature_parameter: A
cool_feature_option: B
We can use some other naming, as long as it has no bundle prefix.
Why?
Because when using bundle prefix, we make assumptions about their usage inside the bundle itself. It would brake or have unintended effects in a case like this:
// ...
$definition->setArguments(array($config['parameter'], $config['option']));
// here we have a problem - our parameter from parameters.yml unintentionally overwrites this parameter in the bundle:
$container->setParameter('evp_cool_feature.parameter', 'ABC');
// ...
In other words, we have prefixes to avoid parameter clashing, by using prefix in config.yml
(or other bundles - that's the same), we can break things.
Default parameters (inside parameters.dist.yml
) must be configured for development environment.
Why? We cannot use production as this would be security issue. All developers can add default values and change them only if needed.
We use value pass
for default passwords in development environment.
We use default domains in parameters - if everywhere configured the same, no changes in parameters should be needed.
Basic rule: using default parameters must always work for any developer out-of-the-box with default environment set-up.
When configuring services, we split definitions into separate files and import them in main
services.xml
and routing.xml
files. For example:
config/services/controllers.xml
config/services/forms.xml
config/services/repositories.xml
config/services/normalizers.xml
We put routing prefix into <import/>
directive.
We put configuration files in subdirectories - services/
, routing/
etc.
Service ID always begins with bundle identifier, like acme_newsletter
, followed by dot.
If type of service is repository, controller, listener or normalizer, these are following parts:
- type of service, like
repository
,controller
,normalizer
- name of the service, excluding the type.
If service is a manager, registry or some other unique service for that bundle, we use it’s identifier directly without the type.
Valid service names: acme_page.page_manager
, acme_page.repository.page
We explicitly state class names for our services, we do not put them in separate parameters.
Instead of this:
<parameter key="bundle.service.class">Namespace\ClassName</parameter>
<service id="..." class="%bundle.service.class%"/>
we use this:
<service id="..." class="Namespace\ClassName"/>
Why? This makes unnecessary difficulty when understanding available services and their structure. If we need to overwrite service, we overwrite it by it's ID, so we could change not only the class name, but also constructor arguments or make additional method calls. If functionality is to be changed by circumstances, we should make ability to change this service using bundle semantic configuration.
We use new syntax for defining factory services:
<service id="newsletter_manager" class="NewsletterManager">
<factory class="NewsletterManagerFactory" method="createNewsletterManager"/>
</service>
We do not use service IDs as class names by default.
Why? Even if this allows easier configuration in some cases and avoids naming concerns for our services, it could be hard to refactor code when another service for the same class is added to our project. This would make one of the services "default" one, which would make debugging harder and understanding of internal working more difficult.
For example, if we have
BalanceProvider
and we would add another service for providing balance with debts included (using same class with different configuration), this would require to go over every usage of the first service and changing it's ID.
We can use class name as service ID (or alias it) when we are providing open-source bundle. This allows others to use autowiring features if needed, even if we don't use them. We should only use this for public services - ones that are to be used outside of our bundle.
We don't use autowiring and autoconfiguration features of Dependency Injection component.
Why? First of all, we don't use FQCN as service IDs, so this would not work. Secondly, it might be quicker at the beginning to use it, but to understand the code, we would need to guess where the service really comes from. If we would need to refactor something, it might require to write DI configuration for already (automatically) defined services, too.
We always use bundle name as route prefix, just like in services. We leave out vendor prefix.
Exception: we drop out -common
suffix, if one exists. Routes should not clash with corresponding
bundle without -common
.
We try to use names to identify the action taken by route, not methods to take it.
For example, create_page
instead of page_post
.
We do not put specific configuration in bundles - we provide semantic configuration for those and define them inside
app/config.yml
.
We do not put secret parts of production configuration anywhere in repository - no private key files, common secret
strings, passwords etc. They must be ignored by git and provided in parameters.yml
, shared directory or in
some other way.
We never dynamically build parameter names or service IDs. We use tags if we need abstraction. We define map or state all things explicitly if needed.
Do not do this:
$container->get('my_prefix.' . $type . '.provider');
Why?
- It's hard to find all possible services which are registered (and thus hard to refactor - many potential pitfalls);
- we do not control if service is even registered in container;
- we cannot add some other configuration;
- we do not test if service implements some specific interface;
- we cannot get all possible types available or all possible services.
Instead do this:
public function addProvider(SomeInterface $provider, $type)
{
$this->providers[$type] = $provider;
}
public function getProvider($type)
{
// todo: throw if missing or return null
return $this->providers[$type];
}
// ...
public function build(ContainerBuilder $container)
{
$container->addCompilerPass(new AddTaggedCompilerPass(
'my_prefix.my_registry',
'my_prefix.tag_name',
'addProvider',
['type']
));
}
// ...
$someRegistry->getProvider($type);
And tag services:
<service class="..." id="...">
<tag name="my_prefix.tag_name" type="my_awesome_provider"/>
<!-- arguments etc. -->
</service>
We inject repository objects inside the services via constructor.
Thus, we do not use EntityManager::getRepository
in controllers or services.
We configure repository classes with lazy=true
.
Why? Creating repository class requires loading Entity metadata, which requires cache to be loaded at service construction
For finding entity by ID we always use find
method, not findOneById
etc.
Exception: If we want to preload some related entities.
Another exception: If we override findOneById
to contain call to find
.
This could be used to provide PhpDoc for IDE to guess the type returned.
Why?
find
uses internal Doctrine cache, while simple queries to database (even only by ID) by default does not
We use query builder and queries inside repositories only - we don’t build queries inside controllers or other services.
Repository methods starting with find
returns instance(-s) of the class, that this repository is related to,
or scalar values.
Let's take an example:
- we have
UserBundle
, which does not have any dependencies; - we have
AvatarBundle
. It does depend onUserBundle
, but reverse is not true (and shouldn't).
We need some repository to return User
entities, that does not have any avatars.
We cannot put this to AvatarRepository
, as it should return only Avatar
entities.
We cannot put it anywhere in the UserBundle
as it would make a hard (and unwanted) dependency on AvatarBundle
.
In this case we create a separate UserRepository
service inside AvatarBundle
, inject EntityManager
manually
and make needed queries in there.
We use findOneBy*
to find one object and findBy*
to find list of objects.
We use getQueryBuilderFor*
to get query builders (for example for pagination),
findCountBy*
etc. to get scalar results.
We do not use findOneBy
or findBy
methods from outside of repository.
We create method inside repository for that concrete use-case. In that method, we can call $this->findBy(...)
with given parameters.
Why? This allows to add more constraints in one single place. For example, after adding
deleted
column we can update all queries inside repository to filter those records out by default. Also we can see all possible queries in one place - this allows us to easily review which indexes should be defined and which are unused.
We avoid direct usage of repositories from other bundles than the repository belongs to.
If we need to get information from another bundle, we tend to use services (which, in that bundle, can inject that repository).
Why? This creates proxy service for getting needed information. Even if at first it would only pass method call to repository itself, when something changes we can refactor it easier - we can inject other services, handle filtering of arguments or results etc. This helps a lot if method in repository is used from many many places inside whole project and we need to change some logic with other injected service.
We do not select related entities if we filter by them. For example, do not do this:
<?php
return $this->createQueryBuilder('user')
->select('user, email')
->leftJoin('user.emails', 'email')
->where('email.status = :activeStatus')
->setParameters([
'activeStatus' => User::STATUS_ACTIVE,
])
->getQuery()
->getResult()
;
If we iterate over user entities that were returned, and call getEmails()
on them, we will not get all emails
for that user. We will get only those that matched the original query (active emails in this case).
Furthermore, it breaks code such as this later in the process:
$user = $userRepository->find(123);
$user->getEmails(); // this will also return *only* active emails, as the user is cached in Doctrine's identity map
What are the options?
- change
->select('c, a')
to->select('c')
, so we do not pre-fetch accounts; - pre-fetch them with a separate join:
Keep in mind that pre-fetching is OK and recommended in most of the cases where we will use those relations. It's just not good if we also filter by that relation.
Further work and filtering should still be done within the code as results will contain users with at least one active email, but won't have users which all emails are not active - in other words we filter out users without active emails.
<?php
return $this->createQueryBuilder('user')
->select('user, email')
->leftJoin('user.emails', 'email')
->leftJoin('user.emails', 'activeEmail')
->where('activeEmail.status = :activeStatus')
->setParameters([
'activeStatus' => User::STATUS_ACTIVE,
])
->getQuery()
->getResult()
;
// ---
foreach ($users as $user) {
foreach ($user->getEmails() as $email) {
if ($email->getStatus() === User::STATUS_ACTIVE) {
// do something
}
}
}
- select only those fields which are relevant for you within that specific case:
Entities won't be resolved, plain array would be returned, hence Doctrine's identity map is not built.
$result = $this->createQueryBuilder('user')
->select('user.name AS userName, email.email AS userEmail, email.status AS emailStatus')
->leftJoin('user.emails', 'email')
->where('email.status = :activeStatus')
->setParameters([
'activeStatus' => User::STATUS_ACTIVE,
])
->getQuery()
->getResult()
;
$return = [];
foreach ($result as $item) {
$return[] = (new NormalizedReturnObject())
->setUserName($item['userName'])
->setUserEmail($item['userEmail'])
->setEmailStatus($item['emailStatus'])
;
}
return $return;
When searching by date period, we take beginning inclusively and ending exclusively.
Why? We do not need to add or remove one second from time periods, code gets much more readable. If both would be inclusive or exclusive, when iterating through periods, some results would be provided in both periods and some results on no periods.
On the other hand, in the user interface, if user wants to set dates inclusively, frontend logic has to map between UI and backend/API convention (usually by adding a day to the end date).
All setters in entity returns $this
for fluent interface.
Setters always resets the previous value. Adders leaves previous value and only adds provided to already existing collection.
In many-to-one relation, many
side (the owning one) always fixes relations.
This is done in the add*
method, adding reverse relation to $this
.
In this case set*
contains code to reset collection and add*
every item in provided collection, so that
- every item in collection would have relation fixed;
- collection object would be the same as before
set*
.
<?php
class Tree
{
private $leaves;
public function __construct()
{
$this->leaves = new ArrayCollection();
}
/**
* @param Leaf[] $leaves we do not typecast to array or Collection as this can be any iterable
* @return $this
*/
public function setLeaves($leaves)
{
$this->leaves->clear();
foreach ($leaves as $leaf) {
$this->addLeaf($leaf);
}
return $this;
}
public function addLeaf(Leaf $leaf)
{
$this->leaves[] = $leaf;
$leaf->setTree($this);
return $this;
}
}
We save states, types and other choice-type information as strings in database.
We provide constants with possible property values in Entity.
Constant name starts with property name, followed by the value itself.
<?php
class Entity
{
const TYPE_SIMPLE = 'simple';
const TYPE_COMPLICATED = 'complicated';
private $type;
public function __construct()
{
$this->type = self::TYPE_SIMPLE;
}
}
We do not provide setId
method as we shouldn't use it - we either persist new entity
or take it from repository and update it.
We set createdAt
property in constructor if we need creation date.
We can also set createdAt
in manager if entity is always created in that one place (it should be).
Why not Doctrine extension? This works faster and more reliable. Furthermore, even new not-yet-persisted entities has
createdAt
value, so we can assert that it’s always available. Also this makes unit testing easier as we do not need to mock extensions.
We do not put updatedAt
to entities unless needed. Valid use-case is if we need to see when any of the fields
was changed, for example if synchronizing relational database via cron job into some other storage etc.
In any other use-case updatedAt
does not give needed information.
Either it is not used at all, either it is used where it shouldn't.
For example, if we need to see when some state has changed, we need to put extra column or even one-to-many relation
to be sure that the date saved really represents that state change, not some other change in any other of entity
fields. Even if there is one (besides id
and updatedAt
) field in entity, extra one can be added later and this
would break the functionality (or naming).
We always avoid more than one flush
in one web request.
We use flush
only in controllers, commands and workers.
These are the top input layer to the system, and is used usually only once per request life-cycle.
Services (and especially listeners) can be used in many different ways, so we cannot make assumption that a few of
them will not get called in the same request.
Services that do flush the database or use other services that flush the database (in any depth) should not live in
namespace Service/
, but in separate namespaces such as Processor/
.
<?php
// namespace Acme\FooBundle\Service; // Incorrect
namespace Acme\FooBundle\Processor; // Correct
use Doctrine\ORM\EntityManagerInterface;
class FooProcessor
{
protected $entityManager;
protected $remoteServiceClient;
public function __construct(EntityManagerInterface $entityManager, RemoteServiceClient $remoteServiceClient)
{
$this->entityManager = $entityManager;
}
public function process()
{
$foo = new Foo('name');
$createdFoo = $this->remoteServiceClient->createFoo($foo);
$foo->setRemoteId($createdFoo['id']);
$this->entityManager->flush();
$this->remoteServiceClient->confirmFoo($foo);
$foo->setStatus('done');
$this->entityManager->flush();
}
}
When we use Doctrine string
type, we try to figure out sensible maximum length for each case separately, we
shouldn't just use 255
as default one.
We should always validate or ensure in some other way that the length of the value fits to the maximum length before storing it to the database.
Why? While maximum length does not affect size in disk, it can affect performance. When INTERNAL TEMPORARY tables are created (for joins, derived tables, counts, group by, order by etc), it's created in RAM only if intermediate results are small enough. This size is calculated taking maximum length of fields, and not actually used space. Thus unnecessary bigger maximum lengths can cause more I/O on the server and slower query times.
We use underscored names for both table names and column names.
We use plural nouns as table names.
Why? This makes SQL statements more natural language - we select some items from a collection.
We add prefix to all databases, consisting of bundle name without vendor.
AcmePageBundle:PageRoute
-> page_page_routes
.
We avoid SQL keywords in table and column names, but leave them as-is in PHP-side.
<?php
class Entity
{
private $key;
}
<field name="key" column="entity_key" type="string"/>
We avoid Doctrine class-map and extending entities.
Why?
- Extending comes with very bad performance (many relations are queried automatically even if they are never used afterwards), and many
instanceof
checks in the code;- this is not extendable, as base bundle must know about all others extending it’s entity;
- we cannot change instance of Entity (change it's subclass) once it's created, which also makes things unnecessarily difficult.
We use such a pattern:
-
Base entity has only the information, which is common to all entities. Thus this information is used inside base bundle in some services.
-
Base entity has field
type
,providerKey
or similar. It defines, which bundle/manager/provider created this entity and knows more information. -
There is Manager or similar service in base bundle, which provides methods to manage this entity (and it’s subclasses). This service has method
addProvider
or similar, acceptingProviderInterface
(or similar) -
There is a compiler pass, which adds Providers (etc., implementing interface from base bundle) to Manager via
add\*
method. We can always use the same class fromlib-dependency-injection
repository, no need to create separate for each case, only if there is some custom logic. -
Providers live in their bundles, optionally having additional information in some separate entities, which possibly have relation to the base entity.
Example:
-
PageBundle
has entityBlock
, which hasid
andproviderKey
. -
BlockManager
has methodrenderBlock(Block \$block)
andaddProvider(ProviderInterface \$provider, $key)
. It lives inPageBundle
. Methods fromProviderInterface
are used only in this manager, (bad:manager→getProvider(block)→render(block)
, good:manager→render(block)
). This allows to refactor some of the code more easily if needed. -
There is compiler pass registered, which adds all tagged providers to manager via
addProvider
method. -
ProviderInterface
lives inPageBundle
and has single method -renderBlock(Block \$block)
. -
TextBundle
has EntityTextBlock
, which has fieldsblock
andtextId
. -
TextBlockProvider
implementsProviderInterface
and lives inTextBundle
.renderBlock
method searches forTextBlock
related to the given block and renders text viatextId
.
Such structure let’s extend services without reverse dependencies. As base bundle has no dependencies, it can be extracted to library and used in other projects.
Also the code is not changed in the base bundle - we can only test the new providers etc., no need to test if we didn’t brake anything in the base bundle.
We only provide getter and setter for Money
object, not for it's fields
We convert to and from Money
object only in getters and setters, we do not use eventing system for that.
It could lead to unsaved fields, as Doctrine does not watch object property, it only watches amount and currency
properties, so in some cases there can be no event at all.
Also this would make understanding structure, debugging and testing harder.
As money object is 1) immutable 2) never compared by reference, we do not store the object itself in Doctrine-persisted Entities. We store only it's internal fields and recreate it when needed. This makes the code simpler.
<?php
class Entity
{
private $priceAmount;
private $priceCurrency;
/**
* @var Money|null $price
*/
public function setPrice(Money $price = null)
{
if ($price === null) {
$this->priceAmount = null;
$this->priceCurrency = null;
} else {
$this->priceAmount = $price->getAmount();
$this->priceCurrency = $price->getCurrency();
}
}
public function getPrice()
{
return $this->priceAmount !== null && $this->priceCurrency !== null
? new Money($this->priceAmount, $this->priceCurrency)
: null;
}
}
If Entity is not persisted by Doctrine, we just store Money object itself as usual.
Inside Doctrine configuration:
<field name="amount" type="decimal" precision="16" scale="6" />
This allows to sum and perform other number-based operations inside the database. Also, it auto-validates amount to be numeric (just in case we didn't handle it), and it takes less space, so also performs better if indexing.
We store currency as a string.
For migrating database structure we always use Doctrine migrations.
There should be no changes inside generated migration if we run doctrine:migrations:diff
after
doctrine:migrations:migrate
.
We do not put custom ALTER
or CREATE
statements inside migration scripts ourselves - we modify xml
file and let
Doctrine to generate migration file for us.
If needed, we can add additional UPDATE
or/and INSERT
statements for data migration corresponding
to our new database structure.
For new projects, we do not put CREATE DATABASE
into migration scripts, but all tables should be created and
then altered by Doctrine migrations.
We use IDENTITY
strategy for ID generation. This makes different structure in PostgreSQL - it works in same way
as in MySQL. If we use AUTO
, sequence is used and ID is set as soon as we persist the entity.
As this behavior is not reproducible in MySQL database, we use IDENTITY
to be compatible with more databases.
This also affects functional tests as sqlite does not have sequences, too.
Example configuration:
<id name="id" type="integer">
<generator strategy="IDENTITY"/>
</id>
Always make sure that parameters are of correct type when building query.
This is especially important when we pass an integer and column type is VARCHAR
.
In this case database tries to cast column value of every row into an integer and compare to passed value - no indexes
get used in such cases. For example:
SELECT * FROM ext_log_entries e0_
WHERE e0_.object_id = 21590
AND e0_.object_class = 'Acme\\UserBundle\\Entity\\User'
ORDER BY e0_.version DESC;
Correct usage:
SELECT * FROM ext_log_entries e0_
WHERE e0_.object_id = '21590'
AND e0_.object_class = 'Acme\\UserBundle\\Entity\\User'
ORDER BY e0_.version DESC;
For PHP 7.0 and above, we might require that correct type (string
in this case) would be passed as an argument.
For lower PHP versions, we could cast an argument when passing to query.
We use controllers as services. That is, every controller must be defined as a service.
If controller methods return Response objects (not for REST controllers), controller can extend base
Symfony Controller. In this case we add a call to setContainer in controller definition (in services/controllers.xml
).
We do not use container in the controller’s code - we inject needed services via constructor.
If controller has no logic, we use FrameworkBundle:Template:template
controller in routing and set template
argument to twig template name to render.
We pass original representation of variables to view, if view can itself transform the variables.
<?php
class Controller
{
public function action()
{
// ...
return $this->render($template, array(
'options' => array('a' => $a, 'b' => $b), // we do not use json_encode() here
));
}
}
<div data-options="{{ options|json_encode }}"></div>
Why? Controller just provides variables, it does not know (or shouldn’t know) how they will be used in template. Furthermore, template sometimes needs both encoded and original versions, so controller would duplicate some code in such case.
We group actions into small controllers - we do not put more than 5-8 actions into one controller.
We try to make controller for each of resources, not just one for whole bundle.
It’s perfectly ok to have controllers containing only one action.
Why? As we use constructor injection, big controllers have many dependencies, which are rarely used in such cases. This is bad for performance. Furthermore, more methods make code more difficult and dependencies not that clear.
Controller class name must always end with Controller
.
We put all available events in final
class, containing only constants with available events.
We name events in past-tense verbs, prefixed by resource for which some action happened.
Why? Events should be used only after something happened (exception: before something happening). Present tense verb would indicate that event should make something, which would be a misuse of event system.
We do not start constants with ON_
.
Why?
on*
indicates action performed when event is dispatched, not the event itself.
We can start constants with BEFORE_
or AFTER_
if it indicated event that is dispatched before or after some action.
We give constant the same value as the constant name, prefixed with bundle name.
Why? For find-replace to find both constant and it’s value usage (in tags). Bundle prefix is needed to avoid collisions.
<?php
final class PageEvents
{
const PAGE_CREATED = 'evp_page.page_created';
}
We use on*
, before*
or after*
method names for event listeners.
We can use the same method for several events if logic is the same.
We name event classes by their data, not some concrete use case.
Wrong:
<?php
class PageCreatedEvent extends Event
{
private $page;
}
Correct:
<?php
class PageEvent extends Event
{
private $page;
}
Also correct:
<?php
class PageChangedEvent extends Event
{
private $page;
private $previousPage;
}
We put following classes to root bundle directory:
- Bundle definition
- Final Events classes for events, raised by this bundle
Command
- for commandsController
- for controllersEntity
- for entitiesEvent
- for eventsListener
- for listenersNormalizer
- for normalizersRepository
- for repositoriesService
- for main services of this bundleWorker
- for workers (handles jobs from RabbitMQ)
We use custom namespaces for integrating with other bundles or when our service has some specific type, is not main facade for that bundle.
Callback/CallbackValidator.php
Callback/CallbackProvider.php
DynamicAsset/AssetGroupProvider.php
PageBlock/MenuProvider.php
Resources
- for resourcesTests
- for tests. Test forVendor/SomeBundle/Namespace/Service
goes toVendor/SomeBundle/Tests/Namespace/ServiceTest
We don't use \Bundle\
sub-namespace after the vendor for new projects - we use Vendor\SomeBundle
directly.
In existing projects we make folder structure the same as it's already used there.
Acme/PageBundle
Controller
PageController.php
PageApiController.php
DynamicAsset
AssetGroupProvider.php
Entity
Page.php
Block.php
Event
PageEvent.php
Listener
LocaleListener.php
RequestSubscriber.php
Menu
PageMenuProvider.php
Normalizer
BlockNormalizer.php
PageNormalizer.php
Resources
...
Service
PageManager.php
BlockProviderInterface.php
BlockManager.php
Tests
Menu
PageMenuProviderTest.php
Service
PageManagerTest.php
PageEvents.php
AcmePageBundle.php
We use either .something
or .getSomething()
, we do not use .getSomething
.
We prefer .something
for getters without arguments.
We prefer using static templates with JS framework and REST API over twig.
If command name consists of several words, we use dashes to separate them. For example, some-namespace:do-something
We register commands as services with dependencies injected into them.
We use lazy loading by always providing attribute with command name in the tag (see documentation).
We use only stable releases for our projects.
We use only supported (at least for security fixes) versions of Symfony.
We prefer LTS version of Symfony framework as it does not require much maintenance on updating vendors.
We can use, if applicable, newer stable version of Symfony, but we must update it periodically (more often than LTS) so that it would still be supported (even if we don't actively maintain that project, so choose carefully).
We prefer to use only 2 different major versions of Symfony for our projects. For example:
- let's say that currently Symfony 3.4 is newest LTS version of Symfony. Symfony 4.1 is stable, but not LTS version. Symfony 2.8 is older LTS version and is still supported;
- some projects are still ran on Symfony 2.8 version;
- we are migrating these projects to Symfony 3.4, some projects are already using it;
- we avoid using Symfony 4.1 for new projects (or avoid updating current projects to this version) as this would make 3 different major versions in use;
- when all the projects are updated to Symfony 3.4, we could update our project to use Symfony 4.1 (even non-LTS) or use this for new projects.
Why not using 3 different major versions? This would make quite hard to maintain several Symfony versions in our libraries and bundles. If we would drop 2.x support (to support only 3.x and 4.x), this would make difficult to correct some bug or add improvement to those projects that still use 2.x version.
We always use skeleton to bootstrap new projects as this allows to easily use any (configuration) fixes or improvements that were made to each and every project during the years. It also includes all needed basic features that could be depended on by our libraries or bundles which could be installed afterwards.
Keep in mind that there are different skeletons for WEB, REST API and processing nodes.
For REST controllers, we use PayseraRestBundle
and normalizers.
Controller methods return entities or scalar variables.
Each method is configured with normalizer and denormalizer if needed.
We do not use request object from the controller. We define normalizers to give only the needed information to the controller. We can use plain normalizer or plain item normalizer if needed.
We use result provider to give Result
entities from REST controller.
In server side we do not use subclasses and class-maps - we use services that give or process needed information.
On the client side, on the other hand, we have full model (via subclasses or just put into one class).
On the server side:
<?php
class MainEntity
{
protected $id;
protected $type;
}
class SubEntity
{
protected $id;
protected $mainEntity;
protected $subValue;
}
class AnotherSubEntity
{
protected $id;
protected $mainEntity;
protected $anotherSubValue;
}
On the client side:
<?php
class Entity
{
protected $id;
protected $type;
protected $subValue;
protected $anotherSubValue;
}
OR
<?php
class Entity
{
protected $id;
}
class SubEntity extends Entity
{
protected $subValue;
}
class AnotherSubEntity extends Entity
{
protected $anotherSubValue;
}
We always check permissions in REST controllers (access to resource) and/or listeners (access to API).
We check permissions using SecurityContext
and writing custom voters, which implement VoterInterface
.
We prefer cursor-based pagination.
This describes conventions used to migrate from legacy code (which was written before integrating Symfony framework into the code base).
Assumption is made that Symfony is already integrated and can be fully used.
All new features are implemented as new Symfony bundles and/or separate components.
New code always follow all of the current conventions.
To continuously move away from legacy code, dependencies must be taken into account. If new code would directly depend on the legacy one, it would be hard to keep track where one ends and another starts. Also it would be hard to refactor some part of legacy code without affecting too much of already written (and still considered new) code.
This is why all the code should be split into two parts:
- current code. This parts adheres to the conventions and depends only on current code or vendors;
- legacy code. This is both the legacy code itself and the code for adapting legacy code for newer functionality.
When writing new code, it must have clear dependencies. It can directly depend only on current code (other components, vendors or other bundles).
Some bundles and components can belong to legacy code. This code can depend on legacy code or anything else. We try to keep it as thin as possible - only for adapting the legacy code or integrating it with Symfony.
Directly depend means that there is no dependencies in the code itself, not that in run-time new code could not call the legacy one (if this would be available, we wouldn't even need it at all).
If bundle need information about something available only in legacy code, bundle defines an interface without any implementation and other services depend on this interface.
This interface can also be defined in any other bundle that this bundle depends on (for example, UserBundle).
We provide semantic configuration for this bundle via Configuration
and Extension
classes.
In the configuration we take service ID, which implements our needed interface.
We add alias in Extension class to provided service ID.
Imagine that we have some legacy code:
<?php
function get_user_data($user_id) {
return get_db()->query('SELECT * FROM users WHERE id = ?', $user_id);
}
We write some functionality that needs to get user's email and name. We could use legacy code directly like in the following example, but we shouldn't:
<?php
namespace Acme\NewsletterBundle\Service;
class NewsletterSender
{
public function sendNewsletter(Newsletter $newsletter)
{
$userId = $newsletter->getUserId();
$userData = get_user_data($userId); // don't do this! ;)
$userEmail = $userData['email'];
$userName = $userData['name'];
// ...
}
}
Instead, we create an interface that provides our needed functionality:
<?php
namespace Acme\NewsletterBundle\Service;
use Acme\NewsletterBundle\Entity\UserData;
interface UserDataProviderInterface
{
public function getUserData(int $userId): UserData;
}
<?php
namespace Acme\NewsletterBundle\Entity;
class UserData
{
/**
* @var string
*/
private $email;
/**
* @var string
*/
private $name;
/**
* @return string
*/
public function getEmail()
{
return $this->email;
}
/**
* @param string $email
* @return $this
*/
public function setEmail($email)
{
$this->email = $email;
return $this;
}
/**
* @return string
*/
public function getName()
{
return $this->name;
}
/**
* @param string $name
* @return $this
*/
public function setName($name)
{
$this->name = $name;
return $this;
}
}
And we refactor our service:
<?php
namespace Acme\NewsletterBundle\Service;
class NewsletterSender
{
private $userDataProvider;
public function __construct(UserDataProviderInterface $userDataProvider)
{
$this->userDataProvider = $userDataProvider;
}
public function sendNewsletter(Newsletter $newsletter)
{
$userData = $this->userDataProvider->getUserData($newsletter->getUserId());
$userEmail = $userData->getEmail();
$userName = $userData->getName();
// ...
}
}
This allows to have new code that has no dependencies on the legacy one.
What we need to do next is implement the interface. To still keep legacy code and current code separate, we do that in another bundle.
<?php
namespace Acme\IntegrationBundle\Service;
use Acme\NewsletterBundle\Entity\UserData;
use Acme\NewsletterBundle\Service\UserDataProviderInterface;
class UserDataProvider implements UserDataProviderInterface
{
public function getUserData($userId): UserData
{
$userData = get_user_data($userId);
if ($userData === false) {
throw new \Exception('User not found'); // we could create separate exception for this
}
return (new UserData())
->setEmail($userData['email'])
->setName($userData['name'])
;
}
}
We configure the service as usual (inside IntegrationBundle
in this case):
<service id="integration.user_data_provider"
class="Acme\IntegrationBundle\Service\UserDataProvider"/>
For our bundle to get the service from outside, we modify Configuration
and Extension
classes and configure
the service in config.yml
:
<?php
namespace Acme\NewsletterBundle\DependencyInjection;
// ...
class Configuration implements ConfigurationInterface
{
public function getConfigTreeBuilder()
{
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('acme_newsletter');
$children = $rootNode->children();
$children->scalarNode('user_data_provider')->isRequired();
return $treeBuilder;
}
}
<?php
namespace Acme\NewsletterBundle\DependencyInjection;
// ...
class AcmeNewsletterExtension extends Extension
{
public function load(array $configs, ContainerBuilder $container)
{
$configuration = new Configuration();
$config = $this->processConfiguration($configuration, $configs);
$loader = new XmlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
$loader->load('services.xml');
$container->setAlias('acme_newsletter.user_data_provider', $config['user_data_provider']);
}
}
acme_newsletter:
user_data_provider: integration.user_data_provider
Now we can use our service inside our bundle:
<service id="acme_newsletter.newsletter_sender"
class="Acme\NewsletterBundle\Service\NewsletterSender">
<argument type="service" id="acme_newsletter.user_data_provider"/>
</service>
Even for database tables that are not managed by Doctrine, we use Doctrine migrations for migrating their structure. This enables clear process for upgrading the database structure and makes it reproducible in all environments.
We always use semantic versioning on library repositories.
Why? When backward incompatible change is made in library, we have two options:
- update all client side projects with new library version;
- every time when we update library check for previous backward incompatible changes, not related to currently added feature.
As it happens, sometimes none of these 2 takes place.
For libraries where semantic vensioning is used, we maintain the CHANGELOG.md
file.
If the file is missing, we create it and port changes from other sources, like UPGRADE.md
(deleting that file afterwards).
Any commit must also have a change in CHANGELOG.md
file with described changes.
We tag each and every commit (except instant bug-fixes or commits before separate merge commits).
-
Right before tagging, we make sure we're on master branch and always run
git tag --sort=v:refname
to see current tags. -
Before tagging, we see available branches on repository as version can be created by branch alias, not only by tags.
-
We bump MAJOR, MINOR or PATCH version by one from latest version available (or parent commit if there are few active branches).
3.1. If we make backward incompatible change, we bump the MAJOR version.
3.2. If we add new feature (any new arguments, method calls, classes etc.), we bump MINOR version.
3.3. If we do not change API of library in any way, just change the internals (usually bug-fixes), we bump PATCH version.
We use annotated tags so that time and author would be present. Example command, 1.2.3
taken as example tag:
git tag -a -m "" 1.2.3
-m ""
sets message to the tag, but as all commits are already with messages, we can leave it empty.
After tagging we need to call git push --tags
- this pushes our tags to origin. We only do this after we've
landed our changes.
The tagging policy is always visible in CHANGELOG.md
file - we don't use Unreleased
block for internal libraries
that are updated for some concrete purpose (to be updated right afterwards in another project).
It's important to keep it updated and in-sync if any other changes are made in master after our diff - one should be careful when rebasing this file.
Until library is stable, versions can change quite often. In this case we still use tags and semantic versioning,
but use 0
as MAJOR version, which allows any backward incompatible change be made in any MINOR version bump.
If API of library is already quite stable, we use 1
as a MAJOR version initially.
We use as generic constraints as possible for required libraries. This avoids updating some library graph just because of some conservative constraint, and not because it is really needed.
For example, we have lib-a@1.0.0
and lib-b@1.0.0
, which depends on lib-a: ^1.0
.
If we roll out MAJOR release for lib-a
(aka lib-a@2.0.0
), at some time we need to modify constraint on lib-b
:
- If
lib-b
works with bothlib-a@1.0.0
andlib-a@2.0.0
(change did not affect this library), we uselib-a: >=1.0.0,<3.0
. This allows to use both1.0
and2.0
with our new version oflib-b
. - If
lib-b
works only withlib-a@2.0.0
and not withlib-a@1.0.0
, we modify constraint as usual tolib-a: ^2.0.0
. - If
lib-b
does not work withlib-a@2.0.0
, we can either leave constraint to^1.0
or update code and use (1) or (2) depending on compatibility with1.0
version. Latter is recommended, as this allows to use newest possible versions (sooner or later we will need to update them).
Version in lib-b
in any of these cases is not required to make a MAJOR bump - if API of lib-b
is left the same,
we can make PATCH update. We can even entirely drop out lib-a
and use some alternative library, as long as API
leaves the same.
This means that if app-x
(or any other library) uses functions or classes from lib-a
,
it must require it in composer.json
.
If it just depends on lib-b
, it does not care about lib-a
versions or even if it is installed at all.
If we require vendor library, we use constraint depending on versioning strategy of that library.
If it states, that it follows semantic versioning (and we believe it does),
we use something like ^1.2.3
- this is added by default if we run composer require vendor/library
.
If it has some other strategy, we must correct the constraint so that we would not get unexpected backward incompatible
changes. For example, Symfony components did break compatibility on minor releases (up to 2.3 version).
In that case, we should have not used ^
in constraint.
Basically, we must always assume that composer update
can be run at any time and everything should still
work as expected.
Due to the same reason, we never require dev-master
.
If vendor library has no versions defined, we require specific commit:
"vendor/package": "dev-master#8e45af93e4dc39c22effdc4cd6e5529e25c31735"