41

While Magento does a lot 'out of the box', we've found there are inevitably features and facilities needed for client stores that require a 3rd party extension.

However, given the nature of the medium, it can be a risky proposition to introduce 'foreign' code to such a complex system dealing with commercial transactions.

What do you look for when evaluating Magento extensions? What are the 'red flags' you've come across (performance hogs, security risks, architectural bad practices)?

  • 3
    Slightly glib, but grep 'eval' * -R. If you see it, run. – Aaron Bonner Jan 31 '13 at 16:01
27

Here are some thoughts on evaluating 3rd Party Modules:

Basics:

  • Current Magento Version Support - Does it support the latest version of Magento (including the current one we are developing it for)?

    If a module does not support the latest release of Magento, it will probably be difficult to make it work without spending precious development time on it.

  • Support - Do the developers who created the module support the product?

    One of the signs of a healthy module is that the developers actively support it. If they don't support it is a red flag, why won't they support the product if it's good?

    Additionally, when a module is supported, we can usually get important info from the developers with a simple email (for example, does this module use jQuery or Prototype).

  • Reviews - What are other users saying? How was their experience?

    By reading the reviews we can get a better sense of the big picture, is there a installation issue? Do the developers respond in a timely and helpful manner? Does it work as advertised?

  • Refund - Will they give you back your money if it doesn't work out as intended?

    Many times we want to try out the module so that we can test it, if it works and meets our specifications great! But if it doesn't, we want the option of returning it and getting a refund for it.

Intermediate:

  • Class Overrides - Does the module override any core classes?

    Generally speaking a good module should not override any core classes, rather it should use Observers.

    One reason for this, is that it can make upgrading Magento difficult. Additionally, other modules might be depending on one output from a given function, and this module is providing a different one.

    Sometimes this is not possible to do, if this is the case, there should be a very good reason why it is overriding a core class.

  • Layout updates - Does the module change some of my layout settings?

    Some modules change the layout settings to your site (for example: product page), make sure it doesn't break your current layout, and if it does what would be required (read: how much time will it take us) to fix it.

  • Template changes - Does the module include templates that change my current design?

    Will this module introduce new templates? If yes, will they break my design? How much time will it take to have the design the way we want?

  • Dependencies - Does the module depend on any other module?

    If the module depends on others, we need to make sure they are there and installed. Additionally we need to ask ourselves, are we going to want to turn off the module it depends on in the future?

Advanced:

  • SQL Upgrade scripts - Does the module update the DB in some way?

    Once a module updates the database we need to make sure of a few things.

    Does it update a core table? If yes, that's not good, we like our databases clean and ready for upgrade.

    Does it store the information in a sensible way? If we want to get the data raw from the database ourselves, would we be able to make sense of it?

  • Events - Does the module observe or dispatch any events?

    If a module dispatches or observes events, we want to know:

    Which events is it observing / dispatching? Will this affect another module working in the system. For example, if one of our modules changes the name of our products on-load to uppercase, and this module adds the word 'free' to the name of the product on-load, how will it work? Will the word 'free' also come out upper cased?

  • Code Review - Does the module use acceptable coding techniques?

    This has more to do with PHP coding techniques than Magento.

    Does the code use Try / Catch blocks?

    Does the code escape user input?

    The specifics of this really depend on our skill level / requirements.

  • Potential issues - What potential issues can come up as a result of installing this module?

    Try to imagine the top five problems that could come up if we install this module, surprising as it may be, it really gives insight in to the project as a whole.

Bottom Line:

All these things are nice to have in an ideal world, in real world scenarios we need to do this thing called 'compromise' :)

Additionally, these guidelines are meant to be as a help to us, not to hinder us, as a result if we are only installing one module, let's say a social sharing module, and it's for a client who needs a simple site setup, there is no sense in doing a ton of research.

In other words: It's all about being efficient with our time, if using this (item in the) guideline helps me save time in the long run use it, if not drop it and save your sanity.

10

Some Magento specific "bad practice" red flags are:

  • any code in app/code/local/Mage
  • overwritten templates (files in app/design that already exist in the core)
  • rewrites of essential classes like catalog/product. In general I look carefully at every rewrite, to see if it could have been avoided
  • severe violations of the Zend/Magento coding standards. While this is only about code formatting, I conclude that devs who are careless about standards will most likely be careless about other, more important, things too.
  • changes in core database tables
  • hard coded text in templates (not using the translation mechanism) and elsewhere
  • business logic in templates (rule of thumb: any occurance of Mage::getModel in the templates directory is usually a bad sign)

Some PHP related red flags (this is a very selective and incomplete list):

  • any Notices and Warnings with enabled error reporting (E_STRICT)
  • usage of the @ operator
  • not sanitizing user data before output

Some performance related red flags:

  • database queries inside of loops
  • loading a whole collection just to use a part of it

Also look out for general Code Smells, those are not necessary red flags but help to estimate the overall quality.

4

Step #1 — Can your client afford to support this extension if the developer goes AWOL?

If no, you cannot use the extension.

If yes, proceed to extension evaluation.

2

The fine folks at Inchoo have an article on analyzing 3rd party module code. The article mentions class rewrites, cron jobs, layout updates, and event observers.

I've found event observer code to have the potential for the highest performance hit. Look out for resource intensive, 3rd party code that runs for frequently dispatched events. Events like, controller_action_predispatch, or collection loadings.

I use this utility module from Prattski to get a nice overview of rewrites and observers.

  • What would events cause a performance hit? I read the dispatching code and it looks pretty lean. The only problem would be the actual loaded code... – boruch Jan 31 '13 at 19:51
  • @boruch This sounded dubious to me too. The article has not the quality I am used to from Inchoo, especially this part is misleading. Observers are in most cases the cleanest solution for extensions and I am sure that the article was not meant to discourage their usage, but it could easily be read this way. What they say is that it should always be preferred to use *_after events instead of *_before events if possible. Performance-wise there is no statement about observers at all. – Fabian Schmengler Feb 1 '13 at 8:30
  • @Tyler Hebel: controller_action_predispatch is dispatched once per request, so that's maybe not the best example. But although you only mention a high potential for performance problems and there are events that get dispatched many more times per request, I disagree: observers are not more or less prone to performance problems than any other code. If you do things that really impact performance like load all products this is a problem on its own, regardless where it happens. – Fabian Schmengler Feb 1 '13 at 8:34
  • @fab -- I was referring to the post rather then the inchoo article. I agree about the quality of the article being mediocre. As far as before vs after, it is obviously better to use after (Performance, bugs and integrity) but many times it's simply impossible. For example many times we need to redirect the user from the observer. The only was to do that is to observer->getController->redirect on a before event. This is a much better way then overriding controllers.. – boruch Feb 1 '13 at 15:09
1

Having any templates and skin assets located in default/default (or pro or enterprise) instead of base/default is pretty annoying.

Obfuscated code is something to watch out for - search for calls to eval(), base64_decode(), and the like. This is often used for license validation, but can be malicious or scary - I've seen at least one component that eval'd arbitary code from a RSS feed.

Look for dl() calls - at least one payment gateway component I've seen requires installing a PHP extension to do its connections!

0

You are right.

There is unfortunately no magic: you have to test them, check the code to see if it is well developped, have a good support for commercial modules thanks to their forum or rapidity to answer to your questions...

There are some reviews on Magento Connect and the popularity of an extension can help to know if it is valuable or not but honestly you can find very popular modules with lots of bug. I had a good example last week with a MailChimp module, mainly well done but I had to fix some bugs and provided them to the developer. There are always risks, you have to test.

WebShopApp had the idea to push to this direction, i mean to bring a platform to get good information about the modules. The idea was to push Magento in this direction. So the module quality is an actual question.

0

My advice: pay extreme attention to the modules which have install/upgrade scripts which change values in core tables because it's not always easy to rollback that kind of changes.

0

The #1 test that I can come up with is to find a zero-day exploit in their code (usually not very difficult with Magento extensions), report only the resulting damage of a mock exploit to their security team (giving no indication of which part of the code is vulnerable), and start the stop-watch -- because this is exactly what is going to happen when your site gets hacked. When their support staff asks for global FTP and mysql access, politely decline stating that it is in violation of the PCI-DSS and offer to let them have read-only access to your source code repository.

The #2 test that I perform is to call the vendor up and catch them off guard. Ask them what kind of behavioral / unit testing that they do, what source-control system they use, which versions of PHP they test on, which versions of Magento are tested, which web-servers are tested on, whether or not they use browser-stack for testing front-end components, etc... If the vendor does not know what you're talking about, goes silent or wants to "get an expert to email you back", run like hell because they most likely use numbered zip files for "version control" and only fix bugs 3 months after their customers report them.

Speaking of the PCI-DSS, all system modifications are also required to have a reversion strategy. With modules that add non-nullable columns to core tables, this becomes nigh impossible while still maintaining a reversion strategy that would pass an audit. Run like hell from any modules that will cause issues (read: SQL Errors) when disabled.

PCI-DSS v3

6.4.5.4 Back-out procedures.

Verify that back-out procedures are prepared for each sampled change.

For each change, there should be documented back-out procedures in case the change fails or adversely affects the security of an application or system, to allow the system to be restored back to its previous state

This, in addition to the other answers. IMO there should be a wall of shame for some of the dangerous crap that has been spawned on this platform.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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