With the new Magento Marketplace, a submitted extension goes through several validation states in order to be approved and available via the Marketplace.

One of them is the technical review, from which you can get a technical report like the following:

Marketplace Technical Report

As you can see, 200+ warnings kinda scared the hell out of me, is there any resource to help fixing every warning apart from the list available in the docs: http://docs.magento.com/marketplace/user_guide/extensions/review-code-validation.html?

up vote 31 down vote accepted

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.

  • 1
    In case i’ve understood this properly, these recommendations refer to all M2 extensions (even for a personal use)? – Siarhey Uchukhlebau Apr 15 '16 at 13:46
  • @SiarheyUchukhlebau yes that's the technical report you get once you upload an extension on the Magento Marketplace – Raphael at Digital Pianism Apr 15 '16 at 13:46
  • 1
    You should keep the opening brace on the same line: isn't it the other way around? Great post by the way – Claudiu Creanga Apr 18 '16 at 9:57
  • @ClaudiuCreanga I think you're right let me double check ;) – Raphael at Digital Pianism Apr 18 '16 at 10:01
  • Is it possible to push through an extension which has more than 10k warning message? Or they reject everything which has warnings? – Roland Soós Apr 18 '16 at 10:41

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.
  • my extension display warning but none of above so is it possible to my extension is got rejected because of warning ? – Sanjay Gohil Sep 25 '17 at 7:19
  • I posted this a while back - there may be new ones added since then. When you log into your developer account, click on the extension, and view the error log, what's the message you are seeing, and what level are they errors? If they aren't severity 10, then you may be rejected for some other reason. What does the rejection email say? – Dewi Morgan Sep 26 '17 at 2:50

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

  • This is extraordinarily poor programming style. Please don't abuse the @noEscape and @escapeNotValidated commands in this way: if you do, they're likely to be deprecated and then disallowed by the MEQP system. See the bottom of my answer for a wide array of better ways to escape your data. – Dewi Morgan Jan 2 at 16:52
  • 1
    @Dewi Morgan: Thanks for your valuable information. – Sankar_k Jan 3 at 6:21

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.