Why are so many PHP functions disallowed in the Magento ECG Coding Standard?

by

The question:

The Magento ECG Coding Standard seems to be (at least kind of) official as standard for Magento 1 extensions:

https://github.com/magento-ecg/coding-standard

But I don’t understand the reasoning behind all rules, and the code sniffer rules with their messages alone do not help much. Is there any detailed documentation about the standard? I know the common best practices and the developers guide but cannot find anything specific about these coding standards.

What troubles me most is the strictness about not using PHP functions.

For example: Why is every single file system related PHP function forbidden?

I guess, you are supposed to use Varien_Io_File, Varien_File_Object etc. but even the core developers are not aware of all the Varien classes and you often find things like in Mage_ImportExport_Model_Import_Adapter_Csv:

    $this->_fileHandler = fopen($this->_source, 'r');

So, the core is not the best example, as often.

Other IMHO questionable forbidden functions:

  • mb_parse_str
  • parse_str
  • parse_url

  • base64_decode

    • yeah, it’s used in backdoors but banning eval should be enough and there are legit use cases, like encoding binary data. And other than json_decode (which is not forbidden) there is no core helper available for this.

Source: https://github.com/magento-ecg/coding-standard/blob/master/Sniffs/Security/ForbiddenFunctionSniff.php

Essentially, my question boils down to: Where is this standard documented? And/or is there a list of “things to use instead of these native PHP functions”?

The Solutions:

Below are the methods you can try. The first solution is probably the best. Try others if the first one doesn’t work. Senior developers aren’t just copying/pasting – they read the methods carefully & apply them wisely to each case.

Method 1

Got an unofficial answer from the ECG team on that:

First of all, the flagged functions are not necessarily disallowed – they should trigger a manual review on usage to ensure legitimate use. It’s a review helper tool, not good/bad code scoring tool.

Second, the assumption was it is better to use Magento wrappers (functions/classes) if they exists as they might offer additional functionality or protection.

Third, as for specific calls, base64_decode is used often for malicious injected code, and the rest like parse_str might be vulnerable, especially handling unknown or user provided input – see for example http://php-security.org/2010/05/31/mops-2010-049-php-parse_str-interruption-memory-corruption-vulnerability/

Again, this is flagging it for review not rejecting the code as bad.

Hope it helps.

Method 2

The Coding Standard has two goals.

  1. make finding of possibly problematic parts a lot easier. An experienced developer already knows which parts of a new module he needs to review, and this standard marks and lists them for him.
    It is not so he can remove this parts, but to review if they are necessary, problematic or both.

  2. support inexperienced developers on which things he should avoid. While all the marked functions can be good solutions for specific problems, they are very easy to use in a problematic way. It leads developers to think more about the problem, and often to a better solutions which does not conflict with the standard.

And sometimes, even the most experienced developers blindly follow the standard and create the most cruel workarounds, just to not offend a community forced standard.

a bit additional informations

File functions often allow usage of protocoll wrappers, means a file path starting with http:// leads to an http reaquest, which is often used for “calling home”, and from time to time kills a shop, because the server is not reachable(30seconds default timeout) and its built into a very central place.

everyithing sql realted, noone belives how many sql injection holes still exists out there. A great example was, as the Search on the Mysql website had one.

there is a core helper for json_decode somewhere, but it has a very old implementation, just use json_decode. No Idea why it should be forbidden.

gettext is an interesting php part, I remember it uses some native OS implementation which could have problems, if you use it in parrallel with different languages (like it usually happens when you have more then one language in your shop. Never really investigated this much, but there also should be no need for it in Magento Context.

Going further trough the list, that seems to be just a list with as much functions as possible.
The history is actually funny, seems they removed some of the functions from the list, after they noticed, they make use of it. 😀


All methods was sourced from stackoverflow.com or stackexchange.com, is licensed under cc by-sa 2.5, cc by-sa 3.0 and cc by-sa 4.0

Leave a Comment