[D7] Minecraft Status

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2775795

Project 2: https://www.drupal.org/node/2775227

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxmrmysterious25022775...

Fixed the git clone URL in the issue summary for non-maintainer users.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Re-organized the OP a bit to make it more obvious which links are relevant to this issue.

Hey mrmysterious2502,

I wanna start off by saying your code looks pretty solid, and was pretty easy to review which is always nice. [shameless plug incoming] you should check out PlayMC, a minecraft server the company I work for built and maintains :)

The format of the review is going to follow a format where I point out any Drupal Coding Standards issues in a file and then do a function-by-function analysis of the code, highlighting any issues I find. Keep in mind that any coding standard problems I find I won't repeat for each file, so make sure to go through your entire code base to catch any potential problems.

minecraft_status.module
General coding style comments:
• Your functions should not begin and/or end with blank lines. eg. do

function module_foo() {
  // Do something here.
}

instead of

function module_foo {

  // Do something here.

}

• The previous comment applies to your switch statements too. Also, a space is mandatory between 'switch' and the parenthesis. eg.

switch ($var) {
  case 'first':
    $result = 'first';
    break;

  case 'second':
    $result = 'second';
    break;

  default:
    $result = 'default';
}

return $result;

• Long array declarations should be broken up onto several lines (look at line 98 in the file as an example that needs to be fixed):

...
  '#options' => array(
    0 => t('Minecraft Ping Protocol'),
    1 => t('Minecraft Ping Protocol (Pre 1.7)'),
    2 => t('Gamespy4 Protocol'),
  ),
...

• All control structures (if, elseif, for, while, switch, etc.) must have a space between the word and the opening parenthesis.
Ex. if (true) {...} instead of if(true) {...}

functions:
minecraft_status_help: There is no need to call 'break;' after a 'return' statement. Although, best practice would be to store your help string in a variable, break out of the switch statement and then return the variable instead.

_minecraft_status_block_content:
• You're missing a doc-block for this function.
• Merely a suggestion, but it would be much easier to read if, instead of having 'case 0/1/2', you defined PING, OLD_PING, and GS4 to have the values 0/1/2 and use those in your switch statement. Ex.

define('MINECRAFT_STATUS_PING', 0);
define('MINECRAFT_STATUS_OLD_PING', 1);
define('MINECRAFT_STATUS_GS4', 2);

...
function _minecraft_status_block_content() {
  $info = array();

  //Query server
  switch (variable_get('minecraft_status_query_method', '')) {
    case MINECRAFT_STATUS_PING:
      ...
      break;

    case MINECRAFT_STATUS_OLD_PING:
      ...

• Any time you use 'variable_get' it is best practice to provide it a default value, even if it's null. variable_get('foo', 'default_value');
• You're doing a lot of weird string concatenation. For instance, line 187 $info['status'] = '' . t('Online') . ''; should instead just be $info['status'] = t('Online');
• Lines 203, 204, 205, 227, 228, 229: there is no need to wrap '!' with the t() function. '!' doesn't translate to anything.

minecraft_status_uninstall:
• This function belongs in a .install file. You have to create a file called minecraft_status.install and move this in there instead.
• Use variable_del(), not variable_set(), to delete custom variables your module creates.

css/minecraft_status.css
General coding comments: (Please review this document regarding Drupal CSS standards)
Make sure to remove all the blank lines you have inside of your style blocks.
In general, it's almost always best to try to avoid IDs as a selector, especially ones as generic as "offline" and "online". If there's another way you can target those elements, or change the name to something a bit more unique that would be best I think.

templates/minecraft-status--block.tpl.php
Extreme nit-pickyness :) Please replace 'echo' with 'print'

Changing to needs work and removing the PAReview bonus. Please do 3 more reviews to regain a permanent bonus :)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

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

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./README.txt:                                ASCII text, with CRLF line terminators
  ./css/minecraft_status.css:                  ASCII text, with CRLF line terminators
  ./minecraft_status.module:                   PHP script, ASCII text, with CRLF line terminators
  ./minecraft_status.info:                     ASCII text, with CRLF line terminators
  ./templates/minecraft-status--block.tpl.php: ASCII text, with CRLF line terminators
  ./minecraft_status.install:                  PHP script, ASCII text, with CRLF line terminators
  README.txt
  css/minecraft_status.css
  minecraft_status.info
  minecraft_status.install
  minecraft_status.module
  templates/minecraft-status--block.tpl.php
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

FILE: /root/repos/pareviewsh/pareview_temp/css/minecraft_status.css
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
  1 | ERROR | [x] End of line character is invalid; expected "\n" but
    |       |     found "\r\n"
 63 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/minecraft_status.module
--------------------------------------------------------------------------
FOUND 30 ERRORS AND 7 WARNINGS AFFECTING 29 LINES
--------------------------------------------------------------------------
   1 | ERROR   | [x] The PHP open tag must be followed by exactly one
     |         |     blank line
   1 | ERROR   | [x] End of line character is invalid; expected "\n" but
     |         |     found "\r\n"
   1 | ERROR   | [x] Whitespace found at end of line
   5 | ERROR   | [x] There must be exactly one blank line after the file
     |         |     comment
  19 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
  19 | ERROR   | [ ] Missing parameter name
  21 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
  21 | ERROR   | [ ] Missing parameter name
  29 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
  37 | ERROR   | [x] Whitespace found at end of line
  44 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected
     |         |     "NULL" but found "null"
  45 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected
     |         |     "NULL" but found "null"
  46 | ERROR   | [x] TRUE, FALSE and NULL must be uppercase; expected
     |         |     "NULL" but found "null"
  55 | ERROR   | [x] Whitespace found at end of line
  68 | ERROR   | [x] Whitespace found at end of line
  71 | ERROR   | [x] Whitespace found at end of line
  72 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
  72 | ERROR   | [ ] Missing parameter name
  73 | ERROR   | [x] Whitespace found at end of line
 104 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
 109 | ERROR   | [x] Whitespace found at end of line
 112 | ERROR   | [x] Whitespace found at end of line
 114 | ERROR   | [x] Whitespace found at end of line
 115 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
 115 | ERROR   | [ ] Missing parameter name
 118 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
 118 | ERROR   | [ ] Missing parameter name
 127 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
 136 | ERROR   | [x] Whitespace found at end of line
 137 | WARNING | [ ] Hook implementations should not duplicate @param
     |         |     documentation
 137 | ERROR   | [ ] Missing parameter name
 147 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
 154 | WARNING | [ ] Format should be "* Implements hook_foo().", "*
     |         |     Implements hook_foo_BAR_ID_bar() for xyz_bar().",,
     |         |     "* Implements hook_foo_BAR_ID_bar() for
     |         |     xyz-bar.html.twig.", "* Implements
     |         |     hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.", or "*
     |         |     Implements hook_foo_BAR_ID_bar() for block
     |         |     templates."
 155 | ERROR   | [x] Whitespace found at end of line
 161 | ERROR   | [x] Inline comments must end in full-stops, exclamation
     |         |     marks, colons, question marks, or closing
     |         |     parentheses
 187 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
 206 | ERROR   | [x] Case breaking statements must be followed by a
     |         |     single blank line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 24 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/minecraft_status.info
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...s/pareviewsh/pareview_temp/templates/minecraft-status--block.tpl.php
--------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
  1 | ERROR | [x] Missing file doc comment
  1 | ERROR | [x] End of line character is invalid; expected "\n" but
    |       |     found "\r\n"
 33 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/minecraft_status.install
--------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------
  1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
    |       |     line
  1 | ERROR | [x] End of line character is invalid; expected "\n" but
    |       |     found "\r\n"
  6 | ERROR | [x] Whitespace found at end of line
 20 | ERROR | [x] Whitespace found at end of line
 27 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 110ms; Memory: 8Mb

Please fix the above before your project application can proceed. Normally, perfect adherence to coding standards is not an application blocker, but with the amount picked up above, I would suggest that at the very least these are fixed before RTBC status is set.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.