OData Server

@r2integrated,

There is a sum of Drupal coding standards errors found using ventral.org you can view them with this link.

Things I recommend fixing first (by priority):

1. test_candidate/classes/ODataServer.php: line 64, "Using eval() or drupal_eval() in your module's code could have a security risk if the PHP input provided to the function contains malicious code."
2. classes/entities_template.inc: The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
3. It seems that your not using short function descriptions. These comments can be very helpful later on when you need to debug something and don't remember what every single function do. see Doxygen and comment formatting conventions

please rectify the issues stated in automation testing - http://ventral.org/pareview/httpgitdrupalorgsandboxr2integrated1561302git

fixing all coding standard issues is NOT mandatory - DON'T change to needs work without explanation/major issues

Good clear introduction and summary of what the module does and how. It's surprising how many applications leave us guessing from the beginning :-)

/**
  * Make sure SDK is installed properly, and if not, inform the user additional
  * actions are required.
  */
function odata_server_enable() {

.. This can and should be done using the hook_requirements() instead. When used, this allows the enable process to prevent installation (if that's what you are trying to do), and also helps by providing status reports as well, that confirm when things are good, even if you move servers etc later.
Good on you for checking though!

Thanks for using the libraries API, it's a good way to be doing this.

I can't fully evaluate all the functionality of the code by tracing it in my head and also understanding the API you are interacting with, but it looks well structured, very well documented and very consistent.

It's not clear to me where, how, or if, access controls are checked. Does this respect field privacy, unpublished content and restriction by role? This is the first security concern in the service as described.

Looking at things like DrupalQueryProvider.php, I can tell you've taken a very robust approach, and clearly know what you are doing there. Perhaps a summary of the extent to which this service respects normal Drupal access controls would be helpful. Is there any expected authentication to read from this service? I see no API key code or anything except

'access arguments' => array('access content'),

in that respect. Maybe I just can't see where to look for that.
If "security" is totally out-of-scope, please make that very clear. We can't have modules that inadvertently make private information public when enabled.

...

Small niggles :

I get the feeling there are cleaner ways of doing this though: ( odata_server_settings() )

     require_once drupal_get_path('module', 'odata_server') . '/classes/ODataServer/Utils/ClassAutoLoader.php';
     ClassAutoLoader::register(array(
         __DIR__ . '/../classes',
         DRUPAL_ROOT . '/../' . $library['library path'] . '/library')
     );

This (and things like it) should be passed through t() as a matter of course. Preferably without markup.

$form['permissions']['info'] = array(
  '#markup' => '<p>Pick which bundles, properties, fields, and references should be exposed through the OData server.</p>',
);

  '#markup' => '<p>' . t('Pick which bundles, properties, fields, and references should be exposed through the OData server.' . '</p>'),

I'd be interested in really trying this whole out, but it looks like a large job to spin up. Is there a way to test it, or a demo you could recommend as an example of how this odata works, or can you describe how you are using it currently?

Code-review-wise:
It's just important If you can let us know what's happening (or not) with security and/or maybe provide *some* sort of authentication layer (Drupal roles probably won't be appropriate for REST, so maybe an API key or IP lock)

Review of the 7.x-1.x branch:

• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./classes/entities_template.inc
  

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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:

1. odata_server_enable(): that should be hook_requirements().
2. odata_server_libraries_info(): example.com?
3. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. Or use your own permission.
4. " module_load_include('inc', 'odata_server', 'inc/odata_server_odata_field_info');": Also, no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';. Also elsewhere.

Yeah, I guess I can see why the library func would not be available before the module is activated.
A reasonably common pattern is for a .install hook to module_load_include() its own module file when needed (during the first .install, the .module isn't loaded yet) and then usually that has any requirements check function needed there.
BUT ... yeah, it's not going to work well on a library (under libraries API) that the module provides itself. It's a puzzler.
Will work fine if doing hook_requirements(runtime) I'd imagine, but install-time... may have to do a work-around of some type. I can't think of a short answer.
Maybe they will have an idea or example in the libraries issue queue.

Sounds like a runtime requirements check is the most practical at this time. Thanks for looking in to it.

It's good you are considering the security aspect. Personally, I think that the built-in access controls we expect to work - "published" and "role can view content" MUST be respected by default. Any module that was found in the repository that failed to do so would have a security issue raised against it immediately. So a note on the README is not enough. The project page must loudly announce that installing this module would make previously private data public. Sorry, but it is a big deal, so make a clear disclaimer - and in the interim until this can be applied, mark the current release as "requires bad_judgement" Thats what bad_judgement is for - to mark a module as an insecure thing that you probably should not put on a real live site as-is

There are a number of access check APIs for the core entities - node_access() etc which are the ones to use. When using Drupal like a data service without authentication, the default will be what is available to the anon user. Don't check the published yourself, but use these if possible. Access permissions rapidly can be modified by other processes, so this API func is the one to trust. I don't know if any support for other new entities is available.

Do you need help with EntityFieldQuery?

Manual Review:
# README.txt: looks good
# odata_server.info: Why a blank line on line#6.
There are not a blockers.

# But in the file odata_server.admin.inc

function odata_server_settings($form, &$form_state) {

}

At various places in this file, you have used variable_get() to asssign #default_value to form elements, so these variables would be stored in the database, but these variables have to be deleted when the module is uninstalled, so the implementation of hook_uninstall() is required in the odata_server.install file which would use variable_del() to delete these variables.

/**
 * Implements hook_uninstall().
 */
function odata_server_uninstall() {
  $vars = db_query("SELECT name FROM {variable} WHERE name LIKE 'odataserver- permissions-%'")->fetchCol();
  foreach ($vars as $var) {
    variable_del($var);
  }
}

This would delete all the variables created.

More review..need some time to understand the code a bit more..

Please make sure to change the status in that case, as reviewers usually pick up the issues which have "need review" status..