eZ Community » Blogs » Gaetano Giunta » eZ Publish 5 Coding best Practices

By

eZ Publish 5 Coding best Practices

Friday 19 September 2014 12:13:52 pm

  • Currently 5 out of 5 Stars.
  • 1
  • 2
  • 3
  • 4
  • 5

A question often asked by partners / customers / developers is about "what are the best practices for developing eZ Publish 5 projects".

It is true that the community at large does not have the same amount of experience with the new version as it had with the legacy one. Any feedback is thus welcome, such as the excellent one from Donat.

Also, compared to eZ4, Symfony often offers looser guidelines and a more open structure, which might leave developers stranded and development team members in conflict with each other.

Having had the opportunity to participate in a couple of "biggish" projects based on eZ Publish 5, I have made some notes on development practices which helped the teams develop better code faster.

Read on, and please do comment on anything which you feel is missing / inappropriate. This is supposed to be a work-in-progress, not a cast-in-stone manual.

Naming

  1. PHP Class/Variable names
    1. Casing: CamelCase is used by Symfony and eZ Publish, so do we
    2. No prefixing of class names with Myproject, since everything is going to be in a bundle anyway and thus have the project name in the namespace
    3. avoid generic names like "Helper", "Manager". Try to use concise but descriptive names. If you can not find a good name, it probably means your code will be confused anyway
    4. Avoid usage of the terms "object" and "node" in favour of "content", "location"
    5. Distinguish f.e. between
      • Location and a LocationId
      • FileName, FilePath and FileContents
    6. Where to put php classes in bundles
      • <TBD>
      • Avoid putting classes at the bundle root
      • If you have many classes of the same type, create a folder for them, e.g. MyBundle\EventListeners
  2. Template variable names
    1. for legacy templates, variable names should not be using CamelCase, but snake_case
    2. the same applies for custom (legacy) template operators and fetch functions
  3. Bundle names
    1. All bundles should be named using a <Project><Topic>Bundle scheme
    2. namespace is <Project>\<Topic>Bundle
    3. the shortbundlename mentioned below corresponds to a lower-case version of Topic. If Topic is a composite word, using an underscore to separate words is ok
  4. Configuration parameters
    1. configuration files shall use lower-case filenames
    2. each service created or parameter added shall use the syntax <project>_<shortbundlename>.etc.etc, fully lower-case
    3. service names can be postfixed with .service if absolutely needed. But you are probably better off without
      • please try not to use .service in the middle of the name. F.e. myproject_search.service.filter.authors => myproject_search.filter.authors.service
    4. the yaml files used for eZ Publish template override rules (view provider configuration) will be named override.yml (override_something.yml in case a bundle contains logically separate parts)
      • take care about having multiple override rules which would match in the same situation, eg. class folder and node X which is a folder. In this case the order of rules is important
  5. Symfony Routes
    1. each route shall be named using a myproject_<shortbundlename>_ prefix. Fully lower-case
  6. Symfony console commands
    1. each command name should respect the format myproject:<shortbundlename>:<action>
    2. usage of options is preferred over arguments, as they make the purpose of each argument more clear
    3. the options names should use dashes to separate words, not underscores. e.g. --location-id
  7. Names of content structures coming from the database
    1. never hardcode in a constant or string a location-id or content-id
    2. try not to hardcode in a php constant or string a class-id
    3. prefer identifiers over ids where applicable (classes, sections, etc)
  8. Controllers
    1. controllers used to display content should be organized with 1 controller per content class (unless of course there are controllers doing more stuff)
    2. all controller actions which are implemented as overrides for eZPublish location view should be named viewLocationAction or viewLocationXXXAction. This makes it easy to spot them
  9. Template names
    1. use underscore in names, suffix with <format>.twig
    2. where to put them: using the same logic as for legacy (template path is derived from the view name)
  10. File names
    1. legacy php code should use all-lowercase filenames
    2. new code should use filenames which match class names in CamelCase, to allow autoloading

Configuration

  1. Use symfony parameters which can be read via the configuration resolver when applicable. This allows those parameters to be changed per-environment and per-siteaccess

Logging

  1. Be liberal with logging - any unexpected-but-not-fatal situation should be logged
  2. Logging is done using the Symfony logger facility. Just inject @?logger into your services (nb: best practice is to always check in your code if the logger object is instantiated before using it. For project-specific code just making sure that a logger IS configured and assuming in code that it present can do as well)
  3. Any log message generated by the application should use a specific "channel", which allows to easily separate the log messages generated by custom code from the ones generated by Symfony/eZ Publish
    1. the default channel is myproject
    2. the way to specify a channel is to add a tag to the service which uses the logger: { name: monolog.logger, channel: myproject }

Security

  1. XSS prevention
    1. Always wash your variables in legacy templates
    2. twig already does it for new templates, but take care about the context (json / javascript / xml / plaintext mails, etc)
  2. SQL-Injection prevention
    1. Any sql command created by custom php code shall escape the parameters received
    2. Parameter binding is preferred to manual creation of sql strings
    3. Even better: use the Dcotrine ORM. The Doctrine DBAL is the default db connection layer since eZPublish 5.3
  3. Never change the current repository user to admin unless there is a good reason to
    1. fatal errors and other unexpected situations might leave the user logged in as admin
    2. the policy system is very flexible. Users should have the rights to access all content they see displayed on the page
    3. are you sure that the user as seen from Legacy-Kernel is switched as well to admin at the same time you do that for the new Kernel (and back)?

Performance

The first 90%

  1. Learn how to get the best performance from eZPublish
    1. never fetch data that you do not need
    2. never fetch data without offset and limit if you are not certain that the amount you get is limited
    3. set proper caching headers on all controllers responses
    4. use properly the H in HMVC: if a page displays content from many different sources in different zones, use subcontrollers to render them with different caching policies
    5. try not to sort/filter contents based on a field value, as that makes the query much slower
    6. fetching a location by url alias is slower than by id or remote id
    7. if you can not do without sorting/filtering on a field value, and the query is slow, use Solr instead
    8. avoid calls to the legacy kernel when possible
      1. whenever using runcallback(), use false as 2nd parameter to avoid a full legacy kernel reload
      2. the fact that the kernel is given to you as a closure has a reason. Do not resolve the closure at service constructor time, do it only when you need the legacy kernel
  2. The Symfony debug toolbar is your friend
    1. always check the number of API requests used to display a page

The rest

  1. The most performing code is code which is never executed
    1. good code is code to which nothing can be removed any more (not code to which nothing can be added anymore)
    2. you will not be paid by the lines of code you write
    3. do not reinvent a framework on top a CMS
  2. Do not copy and paste
    1. look if similar functionality has already been implemented somewhere
    2. when you are writing the same function call in 3 different classes, you should probably make them children of a superclass (where common functionality is implemented), or refactor away the common functionality in a helper
    3. always use the same method to identify a location or content: be it by url, id, remote id, do not use different ways to fetch it in different parts of code
  3. Do not create a class which is just a wrapper around an array
    1. If needed, refactor later turning you array into a class and adding to it the needed interfaces so that the rest of the code can remain untouched
  4. If a long method chain is used multiple times inside in a loop, consider assigning it to a variable outside the loop
  5. Use array indexes to good effect
    1. Ex: if you need to find if a Content with Id "X" is in an array, try to index that array by object id, instead of looping over it
  6. Do not create useless getter/setter methods
    1. (see point 1)
    2. If a class member is only used by the class itself, and the class will probably not be subclassed
      1. do not declare a private variable plus a protected setter and a protected getter
      2. use a protected variable and access it directly
  7. Do not create constructors which only call the parent constructor and do nothing else

Testing

  1. Agree on some Testing practice and stick to it
  2. This includes not setting on a standard which you know you will not have the resources to implement
    1. Good code should be testable
    2. Test suites made entirely of “self::markTestIncomplete” are pure line noise
    3. Unit testing will probably see a high churn rate until your code base stabilizes, as well as during refactoring. If you have limited resources for testing, functional/integration testing is probably where you want to focus instead (note: this very personal advice by myself - please lets not start a huge discussion in testing methodologies here, as it is worth a best-practices volume on its own)

 

Formatting

  1. Whitespace
    • 4 Spaces, not tabs
    • Proper indents
    • Follow eZ whitespace conventions: spaces after opening a parenthesis and before closing it, after commas, around operators, etc...
    • A white space between // and the comments phrase, please - unless you are commenting away code
    • separate class methods with an empty line
    • do not put multiple consecutive empty lines anywhere, unless there is some remarkable separation of stuff happening
  2. All control statements should use {curly brackets}
  3. Line Length < 100 characters
    • or 120, or 80. Pick one you and all team members feel comfortable with
  4. Favour usage of the "blame" tool to find out who modified code
    • resist the urge to go around changing indentation on other people's code
    • when writing multiline function calls, lay them out so that adding a new parameter does not involve changing the existing lines
      1. a dangling comma after the last element of an array is fine (allows to add more elements without changing the line of the last one)
      2. do not vertically align the values on right side of assignments, as adding a longer name on the left side means that the whole block will have to be realigned
  5. Tips:

"Good quality" code

  1. Hardcoded values
    1. see above
  2. Comments / PHP Doc Blocks.
    1. please explain why you do stuff, not how - "how" should be clear from reading the code
    2. add links to JIRA issues (both private trackers and eZPublish trackers, bugs.php.net etc)
    3. try to update php doc blocks whenever you change the signature of a method. Lying documentation is worse than no-documentation
  3. Cyclomatic complexity
    1. Methods should be < 50 lines
    2. Limit depth/complexity of if statements - 2 depth max.
    3. Keep any state combinations to max of 3 depths maximum
    4. These are not HARD rules
  4. Limit use of static methods
    1. Bad for testability
    2. If you really have to do that, prefer usage of static::method() over self::method(), as it still allows subclasses to reimplement methods
  5. Lots of commits vs. 1 large commit
    1. Must include issue # at start of commit message
    2. i.e. IPM-012 This is what was done
    3. Please describe what was done. Avoid commit messages like “bugfix”
    4. Try to only tackle one issue/concern for each commit. One more “commit” command will not kill you
  6. Abuse of Symfony services
    1. do not create a service just for fun
    2. they make it harder to navigate through the code - use an IDE which has good support for them, such as PHPStorm or Eclipes
  7. Separation of concerns
    1. Think hard about what a php class is supposed to "be" when you start implementing it, and why
    2. Then think about it five minutes more after the first code pass
    3. do not put long/complex logic in controllers or legacy views, put it in "entity classes" (a.k.a. business-logic classes)
    4. the same goes for custom template operators or command line scripts, etc...
    5. business-logic classes should not meddle with cookies, the Request object or anything in the HTTP layer
    6. legacy code: business-logic classes should not expose directly fetch functions, but leave that to an xxxFunctionCollection class
    7. legacy code: xxxFunctionCollection classes should just wrap methods from business-logic classes, not implement any on their own (except wrapping results in "return" array, and decoding input params if needed)
    8. if a class takes care of serializing some data using an active-record or other ORM pattern, other classes should not access directly the db structure it uses, but always go through that class
    9. Use liberally custom Symfony events (and of course the existing ones) to create custom extension points
  8. Classes which take care of multiple different concerns are bad
    1. Lay out one after another the steps of the algorithm you are trying to implement (eg. fetch data, transform it, save it). Those steps should be independent from each other's implementation
    2. Ex: a class which is used to manage logging of any kind should not implement an API with createMessage() and Store(), it should expose just a logMessage() method. Storing the info is a private concern
    3. Avoid circular dependencies
      1. between Bundles
      2. between php classes
Proudly Developed with from