[D7] ORCID

I'm commenting in on this issue as well to confirm @partyka and I talked and figured out a game plan for an ideal path. I reviewed the original code that @partyka wrote and being it was the direction I wanted my api code to head towards, I took on the not-as-good namespace for my module.

This module will consist of 2 parts: a core api that other modules can use to push or pull information from ORCID. And the secondary will be the full entity integration (and also likely the user field integration that I did in the orcid_integration module that @partyka linked to). While the orcid_integration module is published, it will be slowly stripped of its set of functionality (so the 1.x branch will expand slightly and we will later create and publish a 2.x branch which utilizes the core api to bring functionality that might not belong in the core suite. Once this occurs, the orcid_integration description will be updated to be clearer about how the module integrates with @partyka's orcid module.

We have both been cleaning up the module together via PAReview and while we will continue cleaning, the module is in a very good state. I'm marking it as RTBC because my involvement has mostly been w/ PAReview. The core idea and structure is very well done.

Automated Review

FILE: ...drupal-7-pareview/pareview_temp/views/orcid_author_views_controller.inc
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
12 | ERROR | Missing short description in doc comment
16 | ERROR | Public method name "OrcidAuthorViewsController::views_data" is
| | not in lowerCamel format, it must not contain underscores
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/includes/orcid.admin.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 4 WARNINGS AFFECTING 5 LINES
--------------------------------------------------------------------------------
14 | ERROR | Public method name "OrcidUIController::hook_menu" is not in
| | lowerCamel format, it must not contain underscores
54 | WARNING | Line exceeds 80 characters; contains 83 characters
55 | WARNING | Line exceeds 80 characters; contains 82 characters
87 | WARNING | Line exceeds 80 characters; contains 151 characters
88 | WARNING | Line exceeds 80 characters; contains 247 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/orcid_author.tpl.php
--------------------------------------------------------------------------------
FOUND 9 ERRORS AFFECTING 8 LINES
--------------------------------------------------------------------------------
1 | ERROR | Missing file doc comment
3 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
4 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
5 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
6 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
7 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
7 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
8 | ERROR | Variable "OrcidAuthor" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
9 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/orcid.views_default.inc.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
19 | WARNING | Variable $view is undefined.
20 | WARNING | Variable $view is undefined.
20 | WARNING | Variable $view is undefined.
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/orcid.install
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
29 | WARNING | Line exceeds 80 characters; contains 85 characters
--------------------------------------------------------------------------------

FILE: .../drupal-7-pareview/pareview_temp/orcid_api/includes/orcid_connector.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
16 | WARNING | Class name must be prefixed with the project name "OrcidApi"
54 | WARNING | Variable $publicWww is undefined.
54 | ERROR | Variable "publicWww" is camel caps format. do not use mixed
| | case (camelCase), use lower case and _
--------------------------------------------------------------------------------

FILE: .../drupal-7-pareview/pareview_temp/orcid_api/includes/orcid_utilities.inc
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
9 | WARNING | Class name must be prefixed with the project name "OrcidApi"
15 | WARNING | Line exceeds 80 characters; contains 93 characters
--------------------------------------------------------------------------------

Manual Review

• Duplication
• Yes, but this module will become the primary integration in favor of https://drupal.org/project/orcid_integration.
• Multiple applications
• No
• README.txt
• Follows the guidelines for in-project documentation. However, install and configuration instructions could be enhanced.
• Master Branch
• Does not follow the guidelines for master branch.
• 3rd party code
• Follows the guidelines for 3rd party code.
• Individual user account
• Follows the guidelines for individual user accounts.
• Code too short/too simple
• Follows the guidelines for project length and complexity.
• Licensing
• Yes
• Security
• No security issues identified.
• Coding style & Drupal API usage

  Code review here using this format:

  OrcidUIController
    public function hook_menu() {
      $items = parent::hook_menu();
      $items[$this->path]['title'] = t('Orcid Authors');
      $items[$this->path]['description'] = t('Manage Orcid Authors.');
      $items[$this->path]['access callback'] = TRUE;
      $items[$this->path]['type'] = MENU_LOCAL_TASK;
      return $items;
    }
  

  (*) I don't think that t() is needed for title and description.

  (*) Hidden dependency on curl. See https://drupal.org/project/curl for an example of a hook_requirements.

  $orcid_author = entity_create("OrcidAuthor",
      array(
        "orcid_id" => check_plain($form_state['values']['orcid_id']),
        "first_name" => "",
        "last_name" => "",
        "published_name" => "",
        "email" => "",
      )
    );

  Don't checkplain on data storage.

  orcid_api.module
  There should be some type of @file doc comment.

  authors.view
  All php files should end in .inc, or .php.

  The starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

Continued manual review.

• function orcid_views_default_views() {
    $views = array();
  
    $files = file_scan_directory(drupal_get_path('module', 'orcid') . '/views', '.view.inc');
    foreach ($files as $file_path => $file) {
      require $file_path;
      if (isset($view)) {
        $views[$view->name] = $view;
      }
    }
    return $views;

  module_load_include could be used here. or at least require_once.

• There's a lot of commented out code, especially in admin.inc

• function orcid_author_form_submit(&$form, &$form_state) {
    try {
  ...
    }
    catch (Exception $e) {

  Don't catch exception, that's too broad. Catch the appropriate exception instead.

• (*)
  

  class OrcidAuthorViewsController extends EntityDefaultViewsController {
    public function views_data() {
  ...
      $data['orcid_author']['other_names'] = array(
        'title' => "Other Names",
        'help' => "Other Names used by this Orcid Author.",
        'relationship' => array(
          'base' => 'orcid_author_other_names',
          'base field' => 'orcid_id',
          'relationship field' => 'orcid_id',
          'handler' => 'views_handler_relationship',
          'click sortable' => 'false',
        ),
      );

  This uses inconsistent quotes. Probably should use single quotes. And I'm fairly sure this should also use t(), therefore marking as needs work. Applies to the rest of the function.

The starred item (*) is a fairly big issue and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

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

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

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/orcid_api/includes/orcid_connector.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   53 | WARNING | Variable $publicWWW is undefined.
  --------------------------------------------------------------------------------
  

• Codespell has found some spelling errors in your code.
  

  ./includes/orcid.inc:96: supressing  ==> suppressing
  

• 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.

manual review:

1. project page: the Entity API module has not dependencies, so that sentence should be corrected.
2. orcid_views_default_views(): this function is duplicated. You should rename the file orcid.views_default.inc.php to orcid.views_default.inc and remove the hook from the module file, that should work.
3. orcid_author_load(): why do you need the extra SELECT query first? entity_load() will also return FALSE if nothing could be found?
4. orcid_delete_author(): why is the array_pop() necessary? This function desperately needs @param documentation, why is $reseacher an array? See https://www.drupal.org/coding-standards/docs#param
5. orcid_theme(): doc block is wrong, this is a hook implementation and should be documented as such, see https://www.drupal.org/coding-standards/docs#hookimpl
6. orcid_author_property_set_authors(): doc block is wrong, copy and paste error?
7. orcid_author_property_set_urls(): check_plain() is wrong here. "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://www.drupal.org/node/28984 . Also elsewhere.
8. "if (in_array("administrator", $user->roles)) {": you should always avoid to check for user roles, always use permissions instead with user_access() instead.
9. orcid_author.tpl.php: all user facing text must run through t() for translation.
10. orcid_author.tpl.php: this looks vulnerable to XSS exploits. First name and last name seem to be user provided input, so they should be sanitized before printing (for example in a preprocess hook). I assume this is currenlty not a security problem because you check_plain() stuff on saving.
11. When submitting an invalid ORCID number: Notice: Undefined property: OrcidAuthor::$other_names in orcid_author_form_submit() (line 69 of /home/sace1b98b9c22dd5/www/sites/default/modules/2268549/includes/orcid.pages.inc). Warning: Invalid argument supplied for foreach() in orcid_author_property_set_authors() (line 241 of /home/sace1b98b9c22dd5/www/sites/default/modules/2268549/orcid.module). Notice: Undefined property: OrcidAuthor::$urls in orcid_author_form_submit() (line 70 of /home/sace1b98b9c22dd5/www/sites/default/modules/2268549/includes/orcid.pages.inc). Warning: Invalid argument supplied for foreach() in orcid_author_property_set_urls() (line 256 of /home/sace1b98b9c22dd5/www/sites/default/modules/2268549/orcid.module). Please enable PHP notices in your development environment.
12. The cancel link on the delete author confirmation form is broken.
13. The update tab on an author shows the same thing as the delete form and I cannot update the author ID?
14. admin/structure/orcid/author is broken and does not show anything, I think you forgot the page callback in hook_menu()?
15. orcid_author_update_confirm_submit(): why do you need to clear ALL caches here (very expensive)? Please add a comment.
16. OrcidUIController:hook_menu(): this is vulnerable to access bypass, because the menu item has the access callback set to TRUE. This should have some permission like "access orcid authors" or something new? And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
17. When I try to order by Given name at admin/content/orcid-authors: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'orcid_author.given_name' in 'order clause': SELECT orcid_author.entity_id AS entity_id, :entity_type AS entity_type, NULL AS revision_id, :bundle AS bundle FROM {orcid_author} orcid_author WHERE (orcid_author.orcid_id LIKE :db_condition_placeholder_0 ESCAPE '\\') ORDER BY orcid_author.given_name ASC LIMIT 50 OFFSET 0;
18. The "add author" link on the overview page is also broken.
19. OrcidUIController::overviewForm(): this is vulnerable to SQL injection. The sort order is not automatically sanitized by the DB API layer in D7, see #829464: orderby() should verify direction and escape fields. I could not directly exploit this because the code upper cases the text so I could not match my lower case tables, but for anyone that has case sensitivity somehow ignored in MySQL this is a severe problem. You should check for the values ASC and DESC, the only ones allowed here.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. Make sure to thoroughly test your module before you set this back to "needs review". A project should be ready for prime time when we do this review.

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.

@partyka had updated the module. Setting to needs review.

The check_plain() usage on saving is definitely a blocker, you need to know when to use check_plain() and when not. Let us know once this is ready for review again!

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.

I'm working on ORCID module for Drupal 8 which allows users sign in/up with OAuth2 API now https://github.com/shenzhuxi/orcid.
Is it possible to make ORCID a full project? I would like to be the maintainer and move the codes from GitHub back to here.

This project application is closed. Since the original applicant did not respond you can fork the project and submit your own project application.