[D7] Diversity Enricher

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxsti-innsbruck1991696git

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.

Hi

/**
* Function get_current_node
* 
* This function retrieves the currently shown node id
* 
* @return nid    Current node id
*/
function get_current_node() {

  if ($node = menu_get_object()) {
    $nid = $node->nid;
  }

  return $node;

}

Variable $nid not use now. Nee to delete it.

/**
* Function get_drupal_owlim_mapping_by_uri
* 
* This functions maps the article uri to the drupal node id
* 
* @param owlim_article_uri  Article uri
* @return node_id    Article node id
*/
function get_drupal_owlim_mapping_by_uri($owlim_article_uri) {
  $query = db_select('diversity_enricher_table', 'de');
  $query->condition('de.' . 'articleowlimuri', $owlim_article_uri, '=')
    ->fields('de', array('nodeid'))
    ->range(0, 1);
  $result = $query->execute()->fetchAll(PDO::FETCH_ASSOC);
  if (count($result) > 0)
    return $result[0]['nodeid'];
  else return NULL;
}

Better use $result = $query->execute()->fetchColumn();. Current expression returns a single value.

Thanks KOsipenko, we will fix that asap.

Your repo only has the master branch. You will need to create a 7.x-1.x branch and remove the master branch.

git checkout master
git checkout -b 7.x-1.x

More info here.

Some other points:
- Add the configuration page path in the .info file.
configure = admin/config/people/diversity_enricher_admin

- I think you could use module_load_include() (or drupal_get_path) to include the files instead of using __FILE__ magic const and defining the __BASE_PATH const. At least consider prefixing the const name with the module name.

- diversity_enricher_adminForm does not follow the function naming convention (and I agree it's too minor and probably a pareview will show it.)

- In menu router items, if you set access callback to TRUE, user always has access to the page. It seems like you need to omit this (thus, defaulting to user_access) because you have added access arguments to the item already.

- do_post_request -- Consider using drupal_http_request for this.

- If you feel like you need to autoload classes, just add them in the module's .info file.
files[] = sesame/phpSesame.php
No need to manually include files in the hard way.

- In menu router items that you want to json_encode, print, and exit(), consider changing the hook_menu implementation's 'delivery callback' function to a single function that does the json_encode part. See 'ajax_deliver' delivery callback from 'system/ajax' menu router item.

- You will also need to rename some function names with your module name prefix to avoid any conflicts.

Good job, and sorry if above suggestions sound any bad. I didn't test the module yet but trying to be helpful to other project reviewers.

Thanks Ayesh! We already got through some of your comments (1 to 5). We will keep you updated on the rest.

KOsipenko, we implemented your suggestions! Thanks again!

Ayesh, thanks again! We now have fixed your comments 1-6 of [#4]. Missing are the following:

- If you feel like you need to autoload classes, just add them in the module's .info file.
files[] = sesame/phpSesame.php
No need to manually include files in the hard way.

- In menu router items that you want to json_encode, print, and exit(), consider changing the hook_menu implementation's 'delivery callback' function to a single function that does the json_encode part. See 'ajax_deliver' delivery callback from 'system/ajax' menu router item.

- You will also need to rename some function names with your module name prefix to avoid any conflicts.

we fixed all issues.

Hello, pretty interesting seeing that what thing does. Seems to "work," here are a few things I noticed that you may want to clean up.

Coding Standards

The Ventral.org report shows so many things I don't know where to start. Please take a look at https://drupal.org/coding-standards, maybe look around for how to configure your IDE to format your code. Be sure to add sufficent comments.

Libraries

You'd probably be better suited to decouple the the rdf-api, semsol, and sesame libraries from the actual module. This would let other modules leverage the code, and make your module lighter. The code comprising of HEAD is currently 15MB.

Working with Field Data

It's not advised to access field data by plucking it directly out of the node object like this: $node->body['und'][0]['value'] Use field_get_items() instead.

Rendering Entities (Nodes)

You also may want to avoid writing into the field on node render, it's a good way to get yourself into trouble. It's not uncommon to use something other than the default text renderer. A better solution would be to provide "extra fields" which show up on the node render, without messing with the output of the field. See hook_field_extra_fields().

Parsing Entities (Nodes)

Kind of related to the rendering, you may want to re-think how you're gathering the node data for parsing. Right now you're just grabbing the value of the body field and there are a few issues with that. 1) Not all nodes have body fields, and 2) I believe the text at that point has not been run through the input filter. A better solution may be to fully render the node, then send that data for parsing. Take a look at the core search module to see how it does it.

Hope that helps you out. Let me know if you have any questions.

Thanks kerasai! I fixed the first two points. The libraries have to be downloaded separately now as explained in the README.txt.

The other points will be fixed in the next days. I will keep you updated!

Thanks again!

Thanks again for your comments kerasai. I looked more closely into your comments about "Rendering Entities (Nodes)" and "Parsing Entities (Nodes)".

We are using the hook_node_view which is the step before rendering the content. There are hooks which allow you to make changes after rendering (hook_node_view_alter) but we believe that this would mess things up even more.

As for the input filtering we now use the safe_value (rather than the blank value) attribute of the body field, which prevents any security issues. This module focuses on articles which commonly have a body field.

Thanks again for your feedback. Although we didn't take all changes into account it urged us to rethink the design and we spotted some flaws (e.g. not using the safe_value attribute).

If you have further comments, just let us know! Thanks again!

PS: Here is a good article about node rendering http://www.computerminds.co.uk/articles/rendering-drupal-7-fields-right-way

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).

Can you confirm that the sti-innsbruck and athalhammer accounts are single users? Please change your account information and enter your realname. You respond with "we" most of the time.

Also, which user is requesting "git vetted user" status? sti-innsbruck created the application, but andreas thalhammer seems to be doing the work.

If you prefer, we can also promote this sandbox to a full project without granting you "git vetted user" status.

----
Top Shelf Modules - Crafted, Curated, Contributed.

double-post.

Hi kscheirer,

I'm sorry about the confusion. The application development was initially driven by Simon Hangl (sti-innsbruck account) but currently, the project is fully maintained by me. Could you please point me to the option to move the project from one user to another one?

Regards,
Andreas

Nope, that's good enough, thanks!

Project page

Please take a moment to make your project page follow tips for a great project page.

PAReview: 3rd party code

• js/jquery.jstree.js
• js/jquery.simplemodal.1.4.3.min.js
• js/_lib/jquery.cookie.js
• js/_lib/jquery.hotkeys.js
• js/_lib/jquery.js (especially don't do this one, Drupal comes with jQuery. jquery_update is the module to require if you need a newer version.

All appear to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

License

Please remove licensing information from all files. Drupal will add the appropriate version automatically during packaging so your repository should not include it. All code hosted on drupal.org must be under a GPLv2 license. If you have additional code you don't want covered under that, make it available somewhere else (github?) and use the Libraries API to include it.

I get a 500 error when viewing http://drupalcode.org/sandbox/sti-innsbruck/1991696.git/blob/refs/heads/..., which is mighty odd. Filed an issue with the webmasters here: https://drupal.org/node/2093509.

Code Review:

• Your constants must be namespaced to the module, so RDFAPI_INCLUDE_DIR should be DIVERSITY_ENRICHER_RDFAPI_INCLUDE_DIR. The other one is ok, although you don't need the leading __.
• When implementing a hook, the function doc should read "Implements hook_something()." See the last point in https://drupal.org/node/1354#drupal.
• We only allow spaces, no tabs, see https://drupal.org/coding-standards#indenting.
• Bad line endings and all functions must be namespaced to your module reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxsti-innsbruck1991696git.
• Take a look at https://drupal.org/coding-standards, the code style is off all over the place.
• You can use a file parameter in hook_menu that will load up external files when requested. This lets you split out your functions into other files. Currently Drupal will load and parse the entire .module file on every page request, which is rather large.
• Generally in drupal we use variable_get/set to retrieve and store settings, instead of creating a database row. You can also provide defaults in this manner, instead of insering them during diversity_enricher_install().
• diversity_enricher_create_new_node() seems strange - how do you know that the article content type exists on the site?
• Why do you need a new global $_diversity_enricher_uri?
• All your requirements and dependencies should be documented in a hook_requirements() in your .install file.
• Try not to build html, Drupal provdes theme functions for this purpose. For example, in diversity_enricher_construct_tree() you can use theme('item_list').

That's all so far, I'll be happy to review more once the code style is cleaned up.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Dear kscheirer,

Thank you for your detailed comments. I will explain subsequently how we addressed your comments:

• Project page
  solved
• PAReview: 3rd party code
  solved
• License
  solved
• Code Review
  Solved most of the mentioned issues.

  Two points are only partially solved:

  • The naming of "RDFAPI_INCLUDE_DIR" cannot be changed without also altering the library that we use.
  • The module is designed to work with the article content type. As article is one of the two predefined content types that come with any Drupal 7 installation, we assume that it is existent. However, the import functionality also worked after we deleted the article content type.

Thanks again!
Andreas

Fixed issues from pareview.sh

FILE: /var/www/drupal-7-pareview/pareview_temp/css/basic.css
--------------------------------------------------------------------------------
FOUND 65 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/css/basic_ie.css
--------------------------------------------------------------------------------

FOUND 12 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/css/divEnr.css
--------------------------------------------------------------------------------
FOUND 43 ERROR(S) AFFECTING 21 LINE(S)
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/css/enricher.css
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
FILE: /var/www/drupal-7-pareview/pareview_temp/css/tagcloud.css
--------------------------------------------------------------------------------
FOUND 29 ERROR(S) AFFECTING 8 LINE(S)

Ok on the include path. And I guess it's ok that you assume the Article content type exists - can you document that somewhere? Or check for it in hook_requirements?

You can also provide a drush.make file that automates the dependency downloads and puts them in the right directory for your users.

None of the CSS files may be licensed either, everything hosted on drupal.org can only be GPLv2 or better. Also, we do have CSS coding standards available here: https://drupal.org/node/1886770.

There are significant style issues reported at pareview.sh, as noted above. Some are spurious, but can you try to reduce that number? They mostly seem to concern your doc-block formatting, for which we have docs available here: https://drupal.org/coding-standards/docs#functions and https://drupal.org/node/1918356#generic-function. The full issue report is available here: http://pareview.sh/pareview/httpgitdrupalorgsandboxsti-innsbruck1991696git.

Otherwise this looks ready to go!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Dear kscheirer,

Thanks again for your comments! We added the article content type "article" to the hook_requirements and fixed almost all of the pareview.sh issues. We also removed the licensing from the css (in fact, this was still a remnant of an imported library).

As for the drush.make. The https://drupal.org/project/drush_make module is no longer maintained, nor available for Drupal 7. Is there an up-to-date version?

Thanks!

Thanks for those updates, much improved!

Not sure how I missed this, but all variables must be namespaced to your module (meaning start with diversity_enricher_*). For ex

• variable_set('sesame_uri', '');
• variable_set('repository_name', '');
• variable_set('ranking_service_uri', 'http://ranking.render-project.eu/rank');
• variable_set('repository_useranking', 1);
• variable_set('enrycher_service_uri', 'http://enrycher.ijs.si/render/en/run-newssenti-newsarticle.rdf');

You can simplify your article requirements check by using more specific sql like "SELECT type FROM {node_type} WHERE type='article'" and then just see if you get a result or not.

You can also remove the .gitignore file. As for drush.make, you don't need another module, you just create a .make file. You can use this makefile maker : http://drushmake.com/. Or create your makefile manually, docs are here http://drush.ws/docs/make.txt.

The variable names are a release blocker, but I'm promoting this anyway on the assumption that you'll fix that before a final review happens.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks kscheirer!

I just fixed these two issues:

Not sure how I missed this, but all variables must be namespaced to your module (meaning start with diversity_enricher_*). For ex

variable_set('sesame_uri', '');
variable_set('repository_name', '');
variable_set('ranking_service_uri', 'http://ranking.render-project.eu/rank');
variable_set('repository_useranking', 1);
variable_set('enrycher_service_uri', 'http://enrycher.ijs.si/render/en/run-newssenti-newsarticle.rdf');

You can simplify your article requirements check by using more specific sql like "SELECT type FROM {node_type} WHERE type='article'" and then just see if you get a result or not.

Will take care of the make file tomorrow!

Thanks again!

kscheirer,

We are not yet able to promote the project (Edit -> Promote). Is there anything missing from our side?

Thanks!

Yes, this application has 1 more step left. It still needs to be reviewed a final time, and marked "fixed."

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved). Only manual reviews count, just using http://pareview.sh is not enough.

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, athalhammer!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.