SWTOR Server Status

This looks fun. You should also run your code through the coder module. It found some nitpicking white space issues but also a possible security issue with your use of preg_match on line 57 in the .module

I just did a quick review and it found a few more whitespace issues.

FILE: /mnt/www/html/pareviewsh/pareview_temp/swtor_server_status.module
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
  12 | ERROR | Whitespace found at end of line
  14 | ERROR | Whitespace found at end of line
  33 | ERROR | Whitespace found at end of line
 266 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Otherwise I looked at the code and it is very succinct and solves the desired use case well.

My only question would be at:

http://drupalcode.org/sandbox/aw030/1913016.git/blob/refs/heads/7.x-1.x:...

 341       '' => t('English'),
 342       'de' => t('German'),
 343       'fr' => t('French')),

Are these the only languages provided by SWTOR?

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

1. swtor_server_status.info: the commented out lines should be removed to avoid confusion?
2. "file_get_contents($swtor_status));": many drupal environments have remote file inclusion disabled, so this will not work. Use drupal_http_request() instead.
3. "'access arguments' => array('administer content'),": wrong permission? This has nothing to do with nodes, so it does not seem appropriate. Use "administer site configuration" or provide your own permission instead.
4. "variable_get('swtor_server_status_guild_show', '0')": all variables defined by your module need to be removed in hook_uninstall().
5. swtor_server_status_form(): use system_settings_form() here then you don't need swtor_server_status_form_submit().
6. "@author aw030 mail@aw030.de": we don't use @author tags in Drupal, you can give credit in README.txt
7. swtor_server_status_block_view(): this is vulnerable to XSS exploits. The third party retrieved data has to be treated as untrusted user provided input and has to be sanitized before printing. Please read http://drupal.org/node/28984 again. Attached is a patch that simulates an XSS attack by overriding the retrieved data with a malicious javascript snippet.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

*for better understanding

manual review:

• swtor_server_status_getdata(): I think you should not do sanitization with cehck_plain() here but in swtor_server_status_block_view() where it is actually used for printing. It is best pratice to keep data unaltered internally and then sanitize it only when necessary on output.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to finally approve this.

Installed OK.
User review:
I'd like to see the project detail (as shown on the modules admin page) use the full name instead of the acronym in the description, thought that's mainly because I found it hard to find and unfamiliar

SWTOR Server Status
SWTOR Server Status block module.

- didn't really take advantage of the description field to describe anything :-)

Installation instructions on the project page were good.

I expected the options for configuring the block would be at the block configuration screen ( admin/structure/block/manage/swtor_server_status/swtor_server_status/configure ) - I think that would be a tidier place for this if there is only to be one block. It's just one click away if using the inline edit tools that are on by default in D7.
However, really good and reasonably clear admin UI options (considering I don't know this system at all!)

Perhaps you could have the css display options ON by default in first install, and provide the option to disable or override them later, instead of vice versa.

Code review:

I'm puzzled that immediately after starting to use SimpleXMLElement to parse server responses, you then switch to use substr() to get data out of it, but I'll let that slide.

You *appear* to be using check_plain in most of the data fields I can see. So it's currently probably OK, but it's a vector to remain vigilant about.

Instead of iterating through all servers looking for their names each time

foreach ($swtor_server_status['data'] as $k => $item) {
      if ($server_name_compare == $item['name']) {

I would have just indexed the data array by the server name at the time. You have the label available when you are creating the array.

Good use of translation throughout.
I'd expect that the display (and the work you've done to toggle the wrapper elements etc) could easily be delegated to a theme.tpl instead of using the render array anf #type=html_tag so heavily, ...but it's fine as is.
eg

        $block['content']['swtor_guild_link'] = array(
          '#theme' => 'html_tag',
          '#tag' => 'a',
          '#attributes' => array(
            'class' => 'swtor-guild-link',
            'href' => variable_get('swtor_server_status_guild_link', '#'),
          ),
          '#value' => variable_get('swtor_server_status_guild', 'MyGuild'),
        );

... we usually just use l() for that :-)

I notice that the provided css ONLY gets included if the 'guild' option is added. That's a little unexpected. Is it by design?

OK. Code is fine. Behavior is fine,. the issues above (especially the XSS) have been addressed.
Looks good to go by me.

@aw030 , you should now have the ability to promote your sandbox to a full project.

To copy & paste Klausi's standard welcome message:
----------------------
Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

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 get 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.
----------------------
Thanks Klausi too.
.dan.

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