vonfaff [vonfaff]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

I am assigning this report to me, as I will review it tomorrow morning (which means in about 16 hours from now).

1. Database schema descriptions should not be passed to t(); see what done from Drupal core modules.
2. In the installation file, the message reported to the user through drupal_set_message() is not translated.
3. The code is using simplexml_load_string() without first check if the function is available; if that extension is available only on PHP 5, then the module should explicit such dependency in the .info file (Drupal 6 requires PHP 4 or higher).
4. The module is not removing the Drupal variables when it is uninstalled.
5. Strings used in the user interface should be in sentence case. There are some strings that are in title case.
6. See http://drupal.org/coding-standards to understand how a module should be written. In particular see how the code should be formatted, and how constants, Drupal variables, and functions should be named.

7.     require_once(drupal_get_path('module', 'user') . "/user.admin.inc");
   

   There is a Drupal function to use in such cases.

8. The call to form_set_error() done in eveauth_character_validate() should be replaced with a call to form_error() (the first argument of form_error() is the same first argument of a element validation function. I am not sure the first argument of form_set_error() is correct, when the form element is declared as $form['api']['character'].

9.                 '#title' => t($alliance["name"].' API validation'),
   

   The first argument of t() must be a literal string; differently, the script that extracts the string to translate to create the translation template will not be able to extract the string, which would not be translatable (if not in the case another module uses the same exact string, but it's rather difficult it happens, when the string is dynamically changed).

10.     $user_api   = db_result(db_query('SELECT api FROM eveauth WHERE drupalid="'.$user->uid.'"', 2));
        $user_id    = db_result(db_query('SELECT userid FROM eveauth WHERE drupalid="'.$user->uid.'"', 2));
    

    The code should use query placeholders.

11.         $html[$key] = "<img src=\"http://img.eve.is/serv.asp?s=256&c=".$character."\" style=\"float: left; margin-right: 8px;\">
                    <h2>".t('Character Sheet for '.$doc->result->name)."</h2><br />
                    <strong>".t('Race:')."</strong>
                      ".
                              $doc->result->race."<br />
                    <strong>".t('Bloodline:')."</strong>
                      ".
                              $doc->result->bloodLine."<br />
                    <strong>".t('Gender:')."</strong>
                      ".
                              $doc->result->gender."<br />
                    <strong>".t('Place of employment:')."</strong>
                      ".
                              $doc->result->corporationName."<br /><br /><hr /><br />
                    <strong>".t('Currently training:')."</strong>
                      ".
                              $skill." to <strong>level ".$training->result->trainingToLevel."</strong> (Training will end on ".$training->result->trainingEndTime.")<br />
                    <strong>".t('Skill points:')."</strong>
                      ".
                              $doc->result->cloneSkillPoints."<br />
                    <strong>".t('Clone status:')."</strong>
                      ".
    
                              $doc->result->cloneName."<br />
                    <strong>".t('Monetary assets:')."</strong>
                      ".
                              convertcash($doc->result->balance)."<br /><br />
                    <h3>Character Skill sheet</h3>";
    

    The code is generating a lot of HTML output; it should use theme functions.

There have not been replies in the past week. I am marking this application as won't fix.

I've downloaded the "won't fix" version and am working to correct the issues cited above, as well as updating the module to be compatible with v7.x. I can't take the pledge as 7.0's in final release candidate stage, but I will keep you advised of my progress.

Also, I'm going to need links to learn about submitting the work when it's completed. If you might point me in the right direction... :)

Thanks,
Paul

Code in Drupal repository should be conformed to Drupal coding standards. If you can't verify it yourself, there is a big chance that your application won't be validated. I haven't seen any application that kiamlaluno does a one phrase review ;-)

See http://drupal.org/coding-standards to understand how a module should be written. In particular see how the code should be formatted, and how constants, Drupal variables, and functions should be named.

The sentence explain exactly what it is wrong with the code, and the coding standards explain how a module function should be named.
For a module called example.module, can a function be called test_check_title()?

I reported what needed to be changed on June 23, and I didn't get any reply, not even at May 8; in such case, I normally close the application because I get the applicant lost interest.

I suggested installing and enabling the Devel and Coder modules. I found Coder's Code Review invaluable especially in formatting issues. After one marathon editing session using Coder, I managed to fix the won't fix version to 7.x compatibility under the Minor error detection level, with an end result of only 1 critical error.

All in all, a fair amount of work, but well worth the results.

Paul

There is an Eve Online module under development at: Eve Online HTTP Tools that you might want to look at. The developer is an active member of the security review team, so the code there appears to be much more in line with best practices as it applies to Drupal development.

Paul

added:
The module at the above link is an API Integration module that may be useful as you expand the scope of the eveauth project as described above. Duplication may not be bad, with a couple of different approaches, the better way generally emerges. This seems especially true when as of now there is *no* module that reliably performs these tasks.

Paul

I am changing the status as per previous comment. It seems the proposed module is duplicating the work done in an existing project.

Ok, just to clarify a couple of things.

I suggested the look at EVE Online HTTP Tools to make you aware of what already existed, in the event you didn't know about it.

That the critical error occurred I can't speak to, but I suspect that something had not been created that was needed.

I haven't offered any comments about testing, so I don't know what's being referred to either.

Sorry for any misunderstandings.

Paul

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue as Won't fix.
Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.