How to fix warnings / errors raised by the Magento Marketplace technical review report?

After an hour going through the report I came up with the following list, it could be helpful for everyone I think.

I will try to keep it updated as soon as I find more warnings/errors:

Warnings

Line exceeds 80 characters; contains X characters

Or

Line exceeds maximum limit of 100 characters; contains X characters

Those one are the one I've seen the most, they are self explanatory, it is a good practice to keep coding lines small in order to keep a clean and readable code.

No space found after comma in function call

You have called a function that receives parameters and didn't add a space after the comma. Example: strrchr($bla,".") should be strrchr($bla, ".")

Expected \"while (...) {\n\"; found \"while (...)\n {\n\"

Expected \"foreach (...) {\n\"; found \"foreach (...)\n {\n\"

Expected \"if (...) {\n\"; found \"if (...)\n {\n\"

Expected \"} else {\n\"; found \"}\n else {\n\"

That means you have returned a line before the opening bracket of those PHP statements.

Example of a bad syntax with an if/else statement:

if (true)
{
}
else
{
}

Should be

if (true) {
} else {
}

The closing parenthesis and the opening brace of a multi-line function declaration must be on the same line

Most of the time, it happens in the constructor where you declare something like this:

public function __construct(
    ProductFactory $productFactory,
    Test $test
)
{
}

Whereas it should be:

public function __construct(
    ProductFactory $productFactory,
    Test $test
) {
}

End of line character is invalid; expected \"\n\" but found \"\r\n\"

Happens most of the time at the beginning of the file, it's caused by the way your IDE encodes the return character.

Variable \"your_variable\" is not in valid camel caps format

Every variable must use the camel caps format, so $your_variable should be $yourVariable

Variable \"one2Three\" contains numbers but this is discouraged

Avoid using numbers in your variables

Inline control structures are not allowed

You should not be using inline control structures such as:

else $test = true;

You should use:

else {
    $test = true;
}

Opening brace of a class must be on the line after the definition

You have returned a line when declaring a class:

class Test
{

You should keep the opening brace on the same line:

class Test {

Private member variable \"yourVariable\" must contain a leading underscore

Protected member variable \"yourVariable\" must contain a leading underscore

You should add a leading underscore to your protected and private member variables: $_yourVariable

As opposite of those two, if you add an underscore in your public variable you can get:

Public member variable \"_yourVariable\" must not contain a leading underscore.

The method parameter $bla is never used

You have passed a parameter to a method but you never use it.

Multi-line function declaration not indented correctly; expected 8 spaces but found X

You have added too much indentation to your function declaration parameters:

public function __construct(ProductRepository $productRepository,
                            ListsInterface $listsInterface,
                            Data $helper
) {

Should be:

public function __construct(ProductRepository $productRepository,
    ListsInterface $listsInterface,
    Data $helper
) {

Possible useless method overriding detected

You're overriding a method without adding modifications, example:

public function __construct(Context $context) {
    parent::__construct($context);
}

Model LSD method load() detected in loop

You're using the load() method inside a loop which in not recommended and has to be avoided.

Most likely your code looks like this:

foreach(...) {
    $model->load();
}

If you're loading a model in a loop it's indeed pretty bad in terms of performance. If you only need to retrieve a few attributes, you should use collections instead.

Function's cyclomatic complexity (X) exceeeds 10; consider refactoring the function

If you're not familiar with cyclomatic complexity I suggest you have a read at this post: https://pdepend.org/documentation/software-metrics/cyclomatic-complexity.html . This warning basically means that there is too many loops and conditions in your function.

Direct object instantiation is discourage in Magento 2

It is caused by the fact that you're instantiating an object directly by calling the class, for example:

new \Zend_Filter_LocalizedToNormalized

You should use dependency injection or a last resort, the object manager.

Comments refer to a TODO task

One of your comment contains the following @TODO flag.

Avoid IF statements that are always true or false

You have create a condition that seems to always be true or false.

For example:

$variable = "6";
...
// More code that doesn't change $variable
...
if ($variable)

Errors

Namespace for \"Class\" class is not specified.

You are missing the use Path\To\Class; statement at the beginning of your class.


Using Codesniffer with the MEQP1 or MEQP2 ruleset (depending on your version of Magento) will give you an idea about the Magento ruleset: https://github.com/magento/marketplace-eqp/tree/master/

This ruleset and the one running on Marketplace's submit process are NOT always perfectly in synch (though of course, that's the ideal), so you might get rejected for codesniffer errors even if it passes the latest version on Github.

Some of the more common "severity-10" errors (the only errors your extension will be rejected for), and their listed recommendations, include:

A closing tag is not permitted at the end of a PHP file

Recommendation: Remove PHP closing tag.

Call-time pass-by-reference calls are prohibited

Recommendation: Read documentation on references in PHP 5 and refactor your code. References: http://php.net/manual/en/language.references.pass.php

Direct use of $_ENV Superglobal detected.

Direct use of $_GET Superglobal detected.

Direct use of $_POST Superglobal detected.

Direct use of $_REQUEST Superglobal detected.

Direct use of $_SESSION Superglobal detected.

Direct use of $GLOBALS Superglobal detected.

Recommendation: Use corresponding wrapper objects in order to get cookie, session or request data.

Function set_magic_quotes_runtime() has been deprecated

Recommendation: Deprecated functions should not be used as they may be removed at any time from a future version. [Probably a generic error for all deprecations]

Identical operator === is not used for testing the return value of strpos function

Identical operator === is not used for testing the return value of stripos function

Recommendation: Use the === operator for testing the return value of this function.

Incorrect usage of back quote string constant. Back quotes should be always inside strings.

Recommendation: [no separate recommendation. I imagine this one is to prevent exec through backquotes.]

Missing the _isAllowed() ACL method in the [ClassName] class.

Recommendation: Very carefully manage the setting, management, and handling of privileges. ACL resource should be defined in adminhtml.xml file for each adminhtml controller and _isAllowed() method should be implemented.

Namespace for [ExceptionClassName] class is not specified.

Recommendation: Specify Exception namespace.

PHP syntax error: Call-time pass-by-reference has been removed

Recommendation: Fix syntax error. [This one accompanies the above. I imagine that a similar generic error is given for all other PHP syntax errors]

Possible Magento 2 design violation. Detected typical Magento 1 construction.

Recommendation: [This comes with no recommendation, but describes code where class usage like Mage::blah or Mage_blah_blah::blah is detected - these are classes which only exist in Magento 1 and will not work in Magento 2. A good idea is to search your M2 extension for the regex Mage(\b|_) to pre-check for M1 usages.]

resource is a reserved word in PHP 7.

Recommendation: [No separate recommendation. Simply renaming the word to something else should work. I imagine this error exists for all reserved words.]

The opening PHP tag must be the first content in the file

Recommendation: Remove all characters before PHP Opening Tag.

Use of die language construct is discouraged.

Use of exit language construct is discouraged.

Recommendation: setBody() response object method should be used.

Use of echo language construct is discouraged.

Use of print language construct is discouraged.

Recommendation: Architecture of the extension should be changed in order to avoid usage of echo, header etc. in classes, consider using setBody() method of the response object.

Use of eval() is discouraged

Recommendation: Avoid using eval().

Unlike these errors, which cause your extension to be rejected, warnings are currently listed merely as a courtesy, to help improve your extension's code. You will NOT be rejected from tech review for warnings, however many there are.

Of course, this rule may be tightened up in future, and the codesniffer ruleset is under constant review, so seeing how many warnings you can resolve is always a good plan. The warnings can also indicate systemic issues with your codebase.

Some reasons for rejection from technical review don't currently show up on the online report, and are only given in the email.

Things like copy-paste violations and malware detected will only have messages shown in the email you get letting you know that your extension was not accepted, so read the email carefully.

An archive of these emails is not currently visible from the developer portal, so if you delete them without reading, or file them to junk, then they're gone.

Magento's Level 1 reviewers sometimes put extra information in this email, either just helpful things that they thought you might want to know about, like "this array key 'sever' should probably be 'server'," or the rationales for their rejection and suggestions on how to quickly resolve it, like "You copied a whole Magento core file and just changed the classpath: you can replace this with a class preference setting instead.", or "You copied a whole Magento core file just to change a couple of public functions: you can use plugins for this instead."

If you don't read these, and just look at the codesniffer report, you may end up trying to fix the wrong problems.

Note that the unescaped output detected message should NOT be dodged using the @escapeNotVerified or @noEscape comments. This will likely be disallowed in future versions of Magento. Instead, use one of the following:

  • Any static string in single quotes.
  • A static string in double quotes, with no inline variables.
  • [recommended] A value escaped with one of the escape methods from \Magento\Framework\View\Element\AbstractBlock (escapeHtml(), escapeUrl(), escapeQuote(), escapeXssInUrl()).
  • A value cast to a numeric type (at least bool and int, maybe others?)
  • Any method call with the word "html" in the name, like printBannerHtml(). Don't abuse this one! Make sure that your blahHtml() method really does correctly escape all variables.

Error:

Unescaped output detected

Error in .phtml file

<ul class="form-list" id="payment_form_<?php echo $code ?>" style="display:none;">

You shout use:

<ul class="form-list" id="payment_form_<?php /* @noEscape */ echo $code ?>" style="display:none;">

Refer the Templates XSS security for http://devdocs.magento.com/guides/v2.0/frontend-dev-guide/templates/template-security.html#escape-functions-for-templates

Tags:

Magento2

Marketplace

Extensions

Recent Posts