Context / Explanation for the Rules

[pocallaghan]

Is there anywhere that actually explain why some of these things are errors or warnings? For example a common error I'm seeing amongst third party (and some of my own) modules is "The use of function curl_init() is forbidden". Why is this forbidden? I assume it's because you can't guarantee the curl extension is installed? What's the 'best practice' alternative that we should be doing? It's rather frustrating finding out your code isn't considered best practice but not being able to find out what the best practice is.

Whilst this isn't an 'issue' per se with this repo, I feel if such a thing does exist it should at least be referenced in the Readme.

[tkn98]

The rules are not overly documented, but one thing you can do each time you wonder about an error or message is to check from which rule file it comes from.

In case of your error it's from Ecg_Sniffs_Security_ForbiddenFunctionSniff (coding-standard/Sniffs/Security/ForbiddenFunctionSniff.php) so this is checked for security review (from the context where the file is located and on how the class has been named, see as well the other deactivated methods there in).

This method works with any PHP CodeSniffer based rulesets btw, so this method is not specific to the magento-ecg coding-standard in particular.

[pocallaghan]

Sure, I'm aware of the file as I had to modify it since it's not compatible with with PHP_CodeSniffer 2.x (I saw another ticket regarding this so master may be fixed, though I'm on the 5.3 compatibility tag). However this is just a long list of functions the standard is saying don't use. I know I "shouldn't" use it, that's what the code sniff 'ERROR' tells me. I want to know why.

Let's take the logical assumption (as you suggest) that the namespacing under Security means that there are security implications. For some of the functions listed I can understand this, mysql_* for example is widely considered in the php world as unsecure. However I have never read any information about pathinfo being unsecure (happy to be directed to evidence to the contrary) and more importantly it's used multiple times in core. So either:

1. There is a true security issue inherent with the function and Magento is vulnerable to it.
2. The function has the potential for misuse.
3. There isn't a security consideration at all and somebody just decided to put it in the list because they didn't want people to use it.

Given these scenarios I really hope it isn't 1 for obvious reason. If it's 2 then blanket banning the function seems like the wrong approach, it seems to me at the worst this should be a lower severity issue. If it's 3 then things are a lot more murky as it depends on the reasons behind the person adding it to the list. The only 2 scenarios I can really think of are that a.) They just don't like the function or b.) they believe there's a better way of achieving the same task. Neither of these seem like justification for banning a function under a security namespace. If it's purely a best practice thing and there's a preferred way of doing it using one of the Varien_* classes for example, then that's great, but it shouldn't be under security and without people knowing what the better practice is how are people going to improve the quality of their code?

I appreciate this may not be the best forum to air my views on this, I chose here purely because I'm unaware of a better one.

[tkn98]

Well, take it a bit more lightly. PHP CodeSniffer does static code-analysis. It should point you to some issues you have to review yourself. It is just a somewhat dumb program that does only what it has been configured for.

The Ecg_Sniffs_Security_ForbiddenFunctionSniff contains a selection of function rules that have been written into the file. For the concrete motivation for each of the entries I can not tell you as I'm not that profound with this project nor did I wrote these rules.

But in any case, do not take a warning or message too seriously letter by letter. It's first of all to make you aware about a potential problem with the code. From a quick look over the list given it's useful to spot potential problems.

This is especially true for security as it does not come out of the box.

[zlik]

Hi Peter,

Thank you for your request. There's nothing wrong in using PHP functions like curl_init() or fopen(), however usually we recommend using built-in Magento or ZF high level adapters/helpers (e.g. Varien_Http_Adapter_Curl or Varien_Io_File) rather than low level functions to follow general Magento coding style.

This is correct that no static code analysis tool can provide 100% level of confidence that the detected pattern is actually an issue. As was said by Tom, please consider the findings as potential problems or indicators of a low code quality.

Thank you for providing the feedback, We'll work to improve README with better explanations and answers to frequent questions.

Warm regards,
Alex