[D7] OMNI EVE API

The latest code is available in the 7.x-1.x-dev branch, once I am happy with it this weekend, I will be pushing the changes to 7.x-1.x.

The first thing that you can start to look at is the automated report for your project. This will help identify some areas where you do not meet Drupal's Coding Standards.

I have gone through the code validator and resolved any complaints in had in the 7.x-1.x-dev branch.

The code in general looks OK, though I did not run it. Looking at the application checklist you do not have a readme.txt. Two of the most noted things in the automated reports were your function declarations, object operators, and casting.

I am doing a README.txt as I type this :) Everything you have pointed was just recently fixed and pushed :D

In omni_eve_api.api.inc perhaps you could use a switch statement instead of elseifs to make it prettier.
Make sure that all user read text is passed through t() function.

I know exactly where you mean, and I did this as I was going through and adding function comments.
I will verify that all user read text is passed through t() and will verify that casting is pushed on all my variables.

I would also suggest double checking that a similar project in not already created, as Drupal. A quick google resulted in some talk about some similar projects but I did not find any Drupal project page.

I have, and there are some projects that have are related to EVE, but nothing attempts to do what this project is doing. That was the reason I started this project :)

Thanks for all the help, do I just make a reply when I have an update this weekend to push for a full project ?

omni_eve_api.install
- Modules should implement hook_schema_alter() when altering schema of another table. In this module, there is a schema altering in the hook_install and hook_uninstall that can replaced with hook_schema_alter().

I do this in omni_eve_api.module

/**
 * Implements hook_schema_alter().
 */
function omni_eve_api_schema_alter(&$schema) {
  $schema['users']['fields']['charactername'] = array(
    'type' => 'varchar',
    'not null' => TRUE,
    'default' => '',
    'length' => 50,
    'description' => 'Main EVE Character Name',
  );
  $schema['users']['fields']['characterid'] = array(
    'type' => 'int',
    'not null' => TRUE,
    'default' => 0,
    'description' => 'Main EVE Character ID',
  );
}

Is this comment not correct ? https://drupal.org/node/185596#comment-941821
Or do I need to move my hook_scheme_alter() to the omni_eve_api.install file?

omni_eve_api.admin.inc
- Module calls omni_eve_api_role_list() twice, in the same form builder function, and omni_eve_api_role_list() has a database query without static caching. You can reuse the first call's return value in the other.
- Some of your t() calls has a static URL hardcoded. Consider using a placeholder so even if you change the URL in a module update, the translations are valid.

First one done :) Nice catch.
I changed all my t() calls that had a URL as you suggested, makes sense as they have changed in the past.

omni_eve_api.module
- Do you really need to load all the .inc files in the .module file in the global scope ?
All .module files will be loaded in every Drupal process, and loading files in the global scope will cause these files to be loaded in every Drupal load. Consider loading them only when necessary. You can, for example, add a file in a menu hook and Drupal will load file before calling the page callback.
There is hook_hook_info that might be useful.

I will put this on my todo list :) For now I just wanted access to all the files, as I was not sure what was being called at any given time. But it makes perfect sense to do what you suggest.

omni_eve_api.register.inc
- I found some drupal_goto() calls in form alter functions. Note that drupal_goto() sends redirect and exit()'s the process so the rest of the page will not be executed.
To properly redirect a form, set the destination in $form_state['redirect] (make sure you pass $form_state by reference in the function definition), and the redirect will happen after everything else is done.

The reason I did a drupal_goto(), is due to that it should never occur. If I do a $form_state['redirect] when the form is being built, it still renders the form, which I don't want to happen as the checks are in place so if someone goes directly to the 2nd registration page, it redirects them back to the first. I am okay with the exit() on the drupal_goto(), as there is no need to proceed, as the user skipped or is attempting to bypass the first registration page.

global $user;

 $uid = $form_state['values']['uid'];

 if ($uid != $user->uid && !user_access('moderate omni_eve_api users')) {
 drupal_goto('<front>');
 }

These checks are not necessary, because the parent menu item is protected by user_access, which means permissions will be checked in validation and submit functions whose path is the same. is the same.

I want to only allow certain people with the role listed above, to be able to access the API information. In the world of EVE :), it is sensitive information that can change the outcome of the EVE world. There may be a better way to handle it, but so far I have not come across a better way.

I have gone and made the changes as suggested in the above posts, as well as I am confident that it is ready for a beta release. Would love to get more input if anyone is able and familiar with EVE :D

Added "PAReview: review bonus" tag.

Added "Reviews of other projects"

Having trouble with hook_hook_info can't get it to work.

/**
* Implements hook_hook_info().
*/
function omni_eve_api_hook_info() {
  $hooks = array();

  $hooks['teamspeak_format_name'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_connection'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_group_create'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_group_get_id'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_get_groups'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_list'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_info'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_by_name'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_delete'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_get_groups'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_get_groups'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_remove_group'] = array(
    'group' => 'ts3',
  );

  $hooks['teamspeak_user_add_group'] = array(
    'group' => 'ts3',
  );

  $hooks['jabber_test_connection'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_connect'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_disconnect'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_format_name'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_format_display_name'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_group_add'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_url_query'] = array(
    'group' => 'jabber',
  );

  $hooks['jabber_url_send_query'] = array(
    'group' => 'jabber',
  );

  $hooks['get_xml'] = array(
    'group' => 'api',
  );

  $hooks['get_character_info'] = array(
    'group' => 'api',
  );

  $hooks['get_character_api'] = array(
    'group' => 'api',
  );

  $hooks['get_character_sheet'] = array(
    'group' => 'api',
  );

  $hooks['get_alliance_ticker'] = array(
    'group' => 'api',
  );

  $hooks['get_corporation_info'] = array(
    'group' => 'api',
  );

  $hooks['get_character_name'] = array(
    'group' => 'api',
  );

  $hooks['verify_blue'] = array(
    'group' => 'api',
  );

  $hooks['list_valid_characters'] = array(
    'group' => 'api',
  );

  $hooks['characters_exist'] = array(
    'group' => 'api',
  );

  $hooks['api_error_msg'] = array(
    'group' => 'api',
  );

  $hooks['list_api_simple'] = array(
    'group' => 'api',
  );

  $hooks['cronapi'] = array(
    'group' => 'cron',
  );

  $hooks['general_cron'] = array(
    'group' => 'cron',
  );

  $hooks['users_cron'] = array(
    'group' => 'cron',
  );

  $hooks['teamspeak_user_cron'] = array(
    'group' => 'cron',
  );

  $hooks['jabber_user_cron'] = array(
    'group' => 'cron',
  );

  $hooks['teamspeak_name_cron'] = array(
    'group' => 'cron',
  );

  $hooks['cron_queue_info'] = array(
    'group' => 'cron',
  );

  $hooks['cron_teamspeak_user'] = array(
    'group' => 'cron',
  );

  $hooks['cron_teamspeak_name'] = array(
    'group' => 'cron',
  );

  $hooks['cron_teamspeak_role'] = array(
    'group' => 'cron',
  );

  $hooks['cron_jabber_user'] = array(
    'group' => 'cron',
  );

  $hooks['cron_jabber_role'] = array(
    'group' => 'cron',
  );

  $hooks['cron_queue_users'] = array(
    'group' => 'cron',
  );

  $hooks['cron_get_standings'] = array(
    'group' => 'cron',
  );

  $hooks['cron_get_alliance_corporations'] = array(
    'group' => 'cron',
  );

  $hooks['cron_get_access_mask'] = array(
    'group' => 'cron',
  );

  return $hooks;
}

Removed my module_load_include for the above and getting errors. It is not loading the include file as I expected with the hook.

I went a different way for hook_hook_info. Still using module_load_include, but now the include files are only loaded when needed, and not on every page load.

1. OeaException instead of OmniEveApiException is not ideal, but fine with me.

Just wanted to keep it short, I use the same tag in my database scheme "oea".

2. omni_eve_api_hook_info(): why do you need that hook? Please add a comment. And why do you register a "ts3" file extension, that is not the name of your module? What is the purpose of this long list of hook names, you never invoke them? Looks like there are a lot queue names in there, did you mean hook_queue_info()? You don't need to register every function you define with hook_hook_info().

I implemented hook_hook_info() due to this comment https://drupal.org/comment/reply/2051379/7747017#comment-7692353 I have since learned it does not work that way, I have just forgotten to remove the hook_hook_info()

3. omni_eve_api_curl_http_request(): why do you need cURL and cannot use drupal_http_request()? If you depend on cURL you need something like https://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/...
4. omni_eve_api_curl_http_request(): looks like a lot of copied code here, why do you need to mess with drupal_generate_test_ua() and Simpletest?

I will be adding a requirement for curl, completely slipped my mind, and yes I prefer curl :)

5. omni_eve_api_role_list(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.

I will be converting most queries to db_select(); I liked how the dynamic query looked/worked and did not know about the performance problem.

6. omni_eve_api_enter_api_form_submit(): empty function, so it can be removed?

Yes, when I originally made the registration I made a validate and submit test function as I did not know what I would need. I will remove it as I don't need it :)

7. omni_eve_api_user_list_api_form(): looks a bit suspicious security wise, the parts in $list_characters are not sanitized? Where do you make sure that those user provided fields are sanitized?

I think you are mistaken, those fields are not user input, they are pulled from the database and displayed. The only option the user can input is by selecting the table row for deletion, which only sends a single (int) via an input array(). Although I did just add a check to sanitize the input to verify its an (int).

Fixed changes above, I will be converting all my db_select to db_query soon.

I have gone through and double/triple checked, there should be no more unsanitized output to the user.

Thanks for all the help guys :)

omni_eve_api_clean_input(): what is this? Why don't you use check_plain()?

I output the message to Teamspeak, it does not have the same abilities that Drupal does.

"variable_set('omni_eve_api_ts3_cron_reg_message', omni_eve_api_clean_input((string) $form_state['input']['reg_message']));": "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://drupal.org/node/28984

I will change it to clean the input on transfer to teamspeak.

$string is a bad variable name, better describe what is in it like $vcode or similar.

Done.

omni_eve_api_user_list_api_form(): usage of check_plain() here looks good, although you don't need it for DB number fields, they can never contain a malicious string, right?

True, but I just wanted to be sure, I didn't know what would be needed to pass.

"check_url('http://www.teamspeak.com/?page=downloads')": why do you call check_url() on a static string, no user provided text is involved? Also elsewhere.

Same as above, didn't know if it would be needed to pass :)

"(string) $form_state['values']['host']": the cast to string is not necessary, submit form data will always be a string?

Doesn't hurt, and its consistent.

omni_eve_api_admin_jabber_form_submit(): just use variable_set('omni_eve_api_jabber_enable', (bool) $form_state['values']['enable']); instead of the if?

Nice catch :)