[D7] Trove

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

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! Thanks for your contribution!

I've performed a code review for your project. It looks clean to me and the README.txt is well written.

I did not test whether or not the module is working as expected since I would have to register to get an API key. This might be a problem for more reviewers. Can't we create one account with an API key that can be used by all reviewers?

As it comes to documentation you might want to consider the following:

• Add a short description of what Trove is to the project page. You can still link to the website of the NLA for more information.
• It is not clear to me whether I can use the Trove API module without enabling the Trove Views Query module and if so how. Maybe you can add some text about that too.
• My guess it that the main module (Trove) is intended as an API that can be used by others to create modules that interact with the Trove service and that the Trove Views Query is such a module. You might consider to write some documentation about how others can use the API in their own modules.

I have a few suggestions for you:

• The TroveAPI class does not conform to coding or documentation standards. Among other things, it has undocumented parameters and variables, and non-camelcased function names
• Consider splitting your TroveAPI class into multiple files and loading using a proper class autoloader like XAutoload using PSR-4
• Consider defining your TroveAPI class using an Interface. Strictly speaking, your @return value for the factory function is incorrect without one.
• Consider moving the API component to a standalone library (e.g. on github) and load via composer
• If you retain the current structure, change the file name. module.api.inc is the standard naming convention for Drupal hook documentation files
• Remove the .gitignore file. You can always ignore these files globally in your own development environment

I'm happy to help with any of those refactoring tasks, if you wish.

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.

Automated Review

http://pareview.sh/ -- has coder finding . Please work on those.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Stands valid here.

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.

Coding style & Drupal API usage
looks Good.

Those pareview issues alone are surely not application blockers, anything else that you found or should this be RTBC instead?

@klausi : Haven't yet tested the module by downloading ...Had a walk through code check . Look good . Would surely want to do a Dev test before RTBC.

Thanks,
SwapS

Automated Review

http://pareview.sh/ -- has coder finding . Please work on those.
Please make sure that characters not exceeds the above 80 characters

Manual Review
In Trove.module
it is good to use dynamic queries instead of static quiries
db_query("SELECT nuc, name
FROM {trove_contributors}")
trove.install
Please make sure that characters not exceeds the above 80 characters
README.txt
Please make sure that characters not exceeds the above 80 characters

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Please do the following changes

FILE: /trove.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
57 | WARNING | Line exceeds 80 characters; contains 121 characters
83 | WARNING | Line exceeds 80 characters; contains 130 characters
147 | WARNING | Line exceeds 80 characters; contains 130 characters
166 | WARNING | Line exceeds 80 characters; contains 130 characters
----------------------------------------------------------------------

FILE: .../trove/TroveApiNewspaperTitle.php
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
4 | WARNING | Line exceeds 80 characters; contains 85 characters
---------------------------------------------------------------------------

FILE: /README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
56 | WARNING | Line exceeds 80 characters; contains 85 characters
68 | WARNING | Line exceeds 80 characters; contains 120 characters
----------------------------------------------------------------------

As the poster has said many times, the line length reports can be ignored due to a long URL provided for additional help. Please read proceeding comments when giving your reviews.

This would be a very useful module if we can get it through the review process. It seems to have languished in the queue a little. What else is needed for RTBC at this point?

@bren001 - are we likely to need a new API key for testing? Or will the temp key you posted almost a year ago still work?

and in the meantime, this module is available on github

https://github.com/bren2000/trove

and in the meantime, this module is available on github

https://github.com/bren2000/trove

I've tested the module with the provided API key and it looks to work as advertised :-)

Tested:

• Publication title search
• Keyword search
• Faceted keyword search
• Change result encoding between xml/json

So I see no reason why this shouldn't be a full project.

Assigining to myself for next review that may be tonight

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

FILE: ...MAMP/htdocs/www/d7/sites/all/modules/contrib/pareview_temp/README.txt
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 10 WARNINGS AFFECTING 10 LINES
---------------------------------------------------------------------------
 48 | WARNING | [x] Unused use statement
 49 | WARNING | [x] Unused use statement
 56 | WARNING | [ ] Line exceeds 80 characters; contains 85 characters
 58 | WARNING | [x] Unused use statement
 59 | WARNING | [x] Unused use statement
 68 | WARNING | [ ] Line exceeds 80 characters; contains 120 characters
 70 | WARNING | [x] Unused use statement
 71 | WARNING | [x] Unused use statement
 82 | WARNING | [x] Unused use statement
 83 | WARNING | [x] Unused use statement
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...www/d7/sites/all/modules/contrib/pareview_temp/src/trove/TroveApi.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 26 | ERROR | [x] Inline comments must end in full-stops, exclamation
    |       |     marks, colons, or question marks
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...ll/modules/contrib/pareview_temp/src/trove/TroveApiNewspaperTitle.php
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 4 | WARNING | Line exceeds 80 characters; contains 85 characters
---------------------------------------------------------------------------


FILE: ...htdocs/www/d7/sites/all/modules/contrib/pareview_temp/trove.admin.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------
 7 | WARNING | [x] Unused use statement
 8 | WARNING | [x] Unused use statement
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...MP/htdocs/www/d7/sites/all/modules/contrib/pareview_temp/trove.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
---------------------------------------------------------------------------
   8 | WARNING | [x] Unused use statement
   9 | WARNING | [x] Unused use statement
  57 | WARNING | [ ] Line exceeds 80 characters; contains 121 characters
  83 | WARNING | [ ] Line exceeds 80 characters; contains 130 characters
 147 | WARNING | [ ] Line exceeds 80 characters; contains 130 characters
 166 | WARNING | [ ] Line exceeds 80 characters; contains 130 characters
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Time: 758ms; Memory: 5Mb

http://pareview.sh/pareview/httpgitdrupalorgsandboxbren0012139829git-7x-1x

Manual Review :

1. Remove some unused statements from trove.module
2. trove_admin_settings_validate : Add more user error set message
3. Add a hook_help

it would be really helpful to add a example submodule which provides the examples/usecases of the api usage.yeah there are couple of good examples in Readme file.

Rest looks good to me.Please take a review bonus before next review.

Assiging to klausi to give it a look when he gets time

Edit : You should fix above issues first and get another review bonus by manual reviews of 3 modules that would help speed up the process.

Edited the comment.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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. trove_get_zones(): did you mean to use drupal_map_assoc() here?
2. trove_cron(): my cron runs every 5 minutes, so this will be executed very often. Is that necessary? I think you should define an interval when items should be refreshed.
3. protected function troveSetError(): never pass already translated string s to watchdog(). It means it will get translated twice, which is bad.

Otherwise looks good to me.

Thanks for your contribution, Brendon!

I updated your account so you can 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 stay 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.

\o/

Yay!!

Welcome aboard @bren001