30

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

Improve this question
6
  • 1
    Magento built on Zend Framework. Why don't you use Zend standards? Commented Apr 9, 2015 at 11:57
  • ECG does more Magento specific checks, like if there are models loaded in loops. This is not about basic style checks like indentation and brackets. Commented Apr 9, 2015 at 12:06
  • 1
    Listing "Raw SQL queries" as forbidden also seems naive. Of course you don't do raw SQL in most situations, but there are definitely times where it is not only appropriate, but necessary (ie complex imports/exports) Commented Apr 13, 2015 at 22:58
  • 1
    @pspahn I see your point, but shouldn't the Zend_Db query builder be capable of generating any SQL queries? Commented Apr 14, 2015 at 6:22
  • Sure, but can't you also create a select statement through Zend_Db using raw SQL as input? I assumed that's what github.com/kalenjordan/custom-reports does in the backend. Commented Apr 14, 2015 at 15:53

2 Answers 2

Reset to default
30
+100

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.

Improve this answer
1
  • Then why are they writing "the function is forbidden" instead of "you should review the code to ensure its legitimate use"?! Commented Aug 27, 2019 at 12:39
11

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. :D

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.