[D7] XMLCharts - Industrial and Precious Metals Prices

Hi there,
I took a quick look at your module code, and I have a few comments.

General: the comments describing functions/methods need more detail. See https://www.drupal.org/node/1354#functions, under the heading "API documentation standards for functions." Guidelines include:

• Each parameter of a function must be documented with a @param tag.
• If a function has a return value, it must be documented with a @return tag.

It does say that if a function is easily described in a single sentence, you may omit the @param and @return tags, but in this case, most of your function comments just repeat the function/method name. A little more description would be helpful.
Also, for xmlcharts.module, the documentation of your hook implementations is fine--no need to change those. Most of the functions that need additional documentation are in xmlcharts.inc, with just a few non-hook functions in xmlcharts.module needing additional documentation.

I installed the module and played with the blocks, and overall it looks great. Just fix those comments and I think this will be good to go!

Hello,

The line no.52 in the xmlcharts.inc file should be if ($cache == cache_get($token)) instead of if ($cache = cache_get($token)) .

I tested your blocks in the footer of http://02g.org/. They work. I tested the removal of the link from the first block and it worked. I suggest you add rel="nofollow" for Google. This would be the one change to the code I would require before using the code on a non demo site.

Memory usage suggestion. Based on working on sites with hundreds of little modules loaded, reducing memory for regular page loads is a priority.

The .module file contains a chuck of cron code that is used once per hour or less. The code could be moved out to a class or separate file with only a short wrapper function in the .module file. xmlcharts_check_time() would move with the cron code.

Hi Andrew,

I also looked at your code here and here is summary of review.

Maybe, you could use more standard titles with your README first file.
Some screen captures could be also nice for any new users.

This is useful module for society for sure.

Regards,
Jouni

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

I am not doing security checks / Drupal API usage checks yet.

This review uses the Project Application Review Template.

Sorry for the delay with this. The review admins are pretty backed up with applications. This is next up for me, and I will have it done today or tomorrow.

Automated Review

Review of the 7.x-1.x branch (commit b8d4515):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows / No: Does not follow the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

This is a touch judgement call, but we have held others to this standard. You are outputting the data from the webservice unsanitized. Since it is third-party data, it is considered unsafe. xmlcharts_block_view()
should be using t() w/ placeholders and drupal_attributes() at a minimum to sanitize this. This is a security problem, yet a pretty remote one.

Coding style & Drupal API usage

You can use drupal_json_decode() instead of json_decode().

XMLChartsData::xmlMetalsObject(), use placeholders with watchdog(). My trace is that this is not exploitable. Also elsewhere. See https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/watc...

Use REQUEST_TIME instead of time() directly.

xmlcharts_uninstall(), doing wildcard deletion of variables isn't the best idea, as you may accidentally zap another module's variables by accident. It is best to track your own and call varianle_del() multiple times.

xmlcharts may not be the best namespace, considering this is not a generic module for xmlcharts, but one specifically for the precious metals market.

The variable_set() in xmlcharts_block_view() may not be the best idea as this will have performance impacts. I thin kyou can just rely on default values from variable_get() here.

xmlcharts_block_view(), build up render array and not #markup. Also consider a theme function.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

(*) Sanitization needed xmlcharts_block_view is the only blocking issue.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I like the simplicity of your approach. Really easy to configure the currency etc.

XMLChartsData::xmlMetalsObject(), use placeholders with watchdog()

This becomes relevant if someone extends XMLcharts to produce their own display, perhaps data from another source, or to use the agrarian feed, or you change code to discover the tokens from the source.

Since it is third-party data, it is considered unsafe.

I have had third parties do stupid things with data. Plus their site may have an exposure where their data can be compromised.

The XMLCharts module name is a bit generic. They list a few feeds:
http://www.xmlcharts.com/precious-metals.html Precious Metals XML Price Feed
http://www.xmlcharts.com/agrarian.html Agrarian XML Price Feed

XMLCharts Precious Metals could be a better name. Or you could advertise your development expertise to add other feeds from XMLCharts.com. XMLCharts.com might pay you to expand your code.

In the readme and on the project page, after the current introduction, you could mention that XMLCharts.com has other feeds and subscription services. Someone searching for a way to get an agrarian or energy feed can find your module from a google search.

After the module is published, approach XMLCharts to add a link to your module on their page. The link might convince a Web developer to use Drupal instead of Wordpress for their metals site. :-)

@awasson - I just had a very quick scan through the codebase (haven't installed/used the module yet). One thing that instantly stood out is markup being passed to the translate ( t() ) function throughout the .module file (ex. line 226, 297, 313, etc.). The t() function is meant to translate text, and shouldn't be receiving markup at all. Ideally, all markup should be ran through theming functions and rendered with theme(), but for very small pieces of markup you can get away with hardcoding it.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Review of the 7.x-1.x branch (commit 33a2276):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follow the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meet the security requirements.

Coding style & Drupal API usage

It is always better to take out HTML from t() function. Also, considering using theme_html_tag() instead of writing direct html tag.

But that are not critical application blocker, otherwise I think this is RTBC.

Thanks for your contribution, Andrew Wasson (awasson)!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.