Node Collection (Previously known as Entity Collection (Super Node))

welcome,

Please rename your readme to "README.txt"

Great, you've already cleaned up all styling stuff reported by automated reviews :)

Putting the module in the category/package called "Maya" doesn't make much sense for me ?

You can also get a review bonus and we will come back to your application sooner.

regards

The name entity_collection is already taken (http://drupal.org/project/entity_collection)

Forgot to change status. I guess you need to find a new name :/

Changes a sentence so that it makes more sense.

One issue found: There are still files other than README.txt in the master branch.
Please remove all files except README.txt from master branch.
Rename `readme.txt` into `README.txt`.

Code is verified with Drupal Code Sniffer. All is ok.

Installed and tested on fresh Drupal installation. Works fine.

Looks fine. ( please, also remove empty API/ folder from master branch)

Yes, it's ok.

Added Reviews of other projects section to the issue summery

Sorry, I got to remove that tag as per

You can use automated tools like pareview.sh for review but you must always do an additional manual review.

From #1410826: [META] Review bonus

Please do 3 manual reviews for getting a review bonus

We all really appreciate that :)

me will do it

Minor stuff that catched my eye

1. usually permission machine names are separated with whitspace not underscore (administer_node_collections -> administer_node_collections)
2. require_once "$module_path/admin/node_collection_node_forms.admin.inc"; doing that drupal_get_dir stuff is not necessary (and looks ugly) require_once 'admin/node_collection_node_forms.admin.inc'; should just work the same
3. Implements hook_perm().update to hook_permission
4. The base table for node_collection_parent.This is not very helpful, describing what exactly the table is good for is a more common practice

5.  * Implements hook_form().
    * Admin settings form for the node collections

   Please start function descriptions with a short intro (like "Implements .."); leave a blank comment line and end comments with a dot

    * Implements hook_form().
    *
    * Admin settings form for the node collections.

6. Maybe admin/structure or admin/config/content is a better place for your module config?
7. What about using a MENU_LOCAL_ACTION for your add relationship link ?
8. Please use the "empty" option of tables to define a message to show instead of an header-only table (looks broken)
9. you should really think about using shorter paths (node_collection_settings/edit_delete_node_collection_relationships could be node-collection/list)
10. If there's a new menu for every

Security issue:
Your not filtering the relation name correctly, putting <script>alert('xss');</script> into it will cause the site to execute that js (= XSS).
Make sure to filter all your output correctly. There are several functions for that, in this case check_plain() should do the job.
Please inform your self about that. (Also have a look at the presentation about security vulnerabilities at denver)

Also the module did not work out for me really good

• After I created several child node there was still only the overview link in the menu
• also the automatic parent/children path generation didn't work out for me

Did I overlook something?

regards

.

Ahh, okay, I see (that link is hard to find, that's probably another good point for using a MENU_LOCAL_ACTION ;) )

Manual review,

are you sure all this includes are requried globally?

// Add the required files.
require_once 'admin/node_collection.admin.inc';
require_once 'admin/node_collection_node_forms.admin.inc';
require_once 'api/node_collection.api.inc';
require_once 'theme/node_collection.theme.inc';

forexample, I mean why not to put conditional load for admin/node_collection.admin.inc in node_collection_menu()

Hi sankatha,

Pretty solid module, I can't find any immediate changes I would suggest. Except:

• In your hook_menu(): you should define your NODE_COLLECTION_SETTINGS_PATH. Currently the settings for your module appear in admin/config/content/ by adding a title, description they will group under your module name.

That's it, guess this is almost done ;)

Ok, I missed part 6 of his comment ^^ Then I feel this is done ;)

manual review:

1. "dependencies[] = pathauto": really? why would you require that?
2. node_collection_schema(): copy and paste error for the child nid "'description' => 'Node collection parent ID.',".
3. node_collection.module: you are requiring a bunch a files globally here, which will be included on every single page request. Ideally you should only expose hook implementations and API functions that might be used by other modules globally.
4. Your module file is quite small. While not an official standard it is recommended to have the important hook implementations directly in your module file, so that I can find them faster if I try to figure out where your module hooks into Drupal.
5. node_collection_admin_settings_form(): doc block is wrong, this is not a hook. See http://drupal.org/node/1354#forms
6. Similar for node_collection_admin_settings_form_submit().
7. "throw new RuntimeException(...": hm, better define your own exception class to distinguish them from real PHP runtime errors?
8. "db_insert('node_collection_parent')": better use drupal_write_record() which also validates against the DB schema. Same for you db_update() calls.
9. node_collection_api_get_parent_node_instance(): do not use node_load() in a loop, this will be slow. Use node_load_multiple() instead.
10. "$child_nid->ec_child_nid": the prefix "ec" still stems from the old module name and does not make sense anymore. You should probably replace it with "nc" to avoid confusion.
11. node_collection_api_is_child_instance(): you don't need the foreach() and you should limit the query with range() if you are interested in only one record. And you can use fetchField() or similar.

Otherwise I think this is nearly ready. Good work so far! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hello @sankatha, your module rocks :)

My simple review:

• Clean your README.txt file in master branch, please see http://drupal.org/node/1127732.

You're welcome @sankatha. I'm changing the status.

Sorry for the delay, but doing reviews with #1410826: [META] Review bonus is currently the only way to make this process faster.

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

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

  FILE: ...ll/modules/pareview_temp/test_candidate/admin/node_collection.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   118 | ERROR | Line indented incorrectly; expected 4 spaces, found 5
  --------------------------------------------------------------------------------
  

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. "php = 5.3.x": why do you need PHP 5.3? Do you use any features that are not available in 5.2?
2. node_collection_schema(): you should specify the foreign keys that refer to the node_type and node tables.
3. "require_once 'admin/node_collection_node_forms.admin.helper.inc';": better use something like require_once dirname(__FILE__) . '/mymodule.inc'; to avoid possible issues with relative paths. Also elsewhere.
4. "@return void": if there is no return value just omit the @return tag. See http://drupal.org/node/1354#functions
5. "t("Edit : $child_node->title")": do not directly embed variables into translatable strings, use placeholders instead. Also elsewhere.
6. node_collection_api_is_parent_type(): doc block: "Return TRUE or FALSE" that is obvious from the fact that the type is boolean. You should describe in what case FALSE is returned, same for TRUE.
7. node_collection.api.inc: all files that define classes should be listed in the info file for autoloading.
8. node_collection_api_get_grand_parents(): why do you catch the exception if you just re-throw it? Also in node_collection_api_delete_associated_child_instance().

Although you should definitively fix those issues they are not application blockers to me, so I guess this is RTBC

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.