[D7] m2Serve Connector

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

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.

Looks good except few issues with variable names, class names and function names. That should be fixed. I guess 7.x-1.x is your branch, the bot scanned the master with some old code.

URL: http://pareview.sh/pareview/httpgitdrupalorgsandboxprateekjain2112189git...

Also you can delete maser branch, once you set the default branch as 7.x-1.x

Manual Review:

• $views = views_get_all_views(); in m2serve_services_views_list.resource.inc line no 16. -- This assumes that views module is installed and enabled. The module's info file doesn't mention views as a dependency. Your module may work without views, but you should Ideally check for views module before using it APIs, or let the user know that he needs to install views in order to use the full capability of the module views_get_all_views. See: https://api.drupal.org/api/drupal/includes!module.inc/function/module_ex...

Major is only for issues that haven't been reviewed in 2 weeks, see https://drupal.org/node/539608#application-priorities.

Apart from the function naming issues outlined in the automated report, I found a couple of other things you can consider:

• The switch-case block at m2serve_connector.module:47 can be refactored to use m2serve_connector_get_request_type. I know they are not exactly same, but consider it if these two blocks are semantically the same.
• The test case appears to be a unit test but it inherits from DrupalWebTestCase. Consider extending DrupalUnitTestCase instead.
• There seems to be a few instances of commented code. It is not clear why they were removed. It would be a good idea if you can mark why you have commented code and in what event might someone want to uncomment it.
• There are multiple spelling and grammatical mistakes in README.txt. It is still readable but it could use some cleaning up.

Accepted for #1 and #2. As far as #2 is concerned, I just feel unit test cases are more appropriate, but if these are the only tests that can be unit tested in near future, then fine.

For #3, I mean code that has been commented out, not comments for code. I saw this in the test file (which seems to be for future use) and a variable $view_display_count in m2serve_services_views_list.resource.inc and another such line in m2serve_connector_resource.inc.

For #4: README.txt is now understandable but there are a few minor punctuation and grammatical mistakes. For example:

• ... build native, hybrid and mobile web apps very rapid for Drupal solutions.
• Other places where lack of punctuation makes reading a little bit difficult.
• No periods after sentences in bullet points.
• Installation section should also describe how to install the module (just link to the page on d.o). This is just for completeness.

Lastly, I still see the issues in the automated report are not fixed. Particularly, functions not prefixed with module names. I know it is unlikely that there would be a clash on that name but it is still for the better. Further, there are some unused variables, which can be ignored but $results is undefined, but used in m2serve_connector_resource.inc:151. I think this should be fixed.

Also, similar to what is mentioned by @swarad07 in #3, the m2serve_connector.info also needs to list views as dependency. views_get_all_views is being used in m2serve_connector.admin.inc. I know you have an indirect dependency on views but here, you are directly using a views function and hence, you should explicitly list it as a dependency.

I think we are good to go. It seems that the function name prefix error was a false positive.

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.

I'm also upping the priority to major since it's been more than 2 weeks.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Dear klausi,

I am the co-maintainer/developer of the module. Hoping this helps to speed up the process, I am listing some of my manual reviews as well,

https://drupal.org/comment/8262523#comment-8262523
https://drupal.org/node/2130235#comment-8262543

Please let us know anything else we can do to make it a full project.

Cheers,
Swara

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

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

  
  FILE: /home/klausi/pareview_temp/m2serve_connector.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   206 | ERROR | Invalid @return data type, expected object but found stdClass
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/m2serve_connector.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   613 | WARNING | Are you accessing field values here? Then you should use
       |         | LANGUAGE_NONE instead of 'und'
   616 | WARNING | Are you accessing field values here? Then you should use
       |         | LANGUAGE_NONE instead of 'und'
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/m2serve_connector.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   194 | WARNING | Unused variable $key.
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/resources/m2serve_connector_resource.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    33 | WARNING | Unused variable $key.
   175 | WARNING | Unused variable $arr.
  --------------------------------------------------------------------------------
  

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. module name: why the long "m2serve_connector" and not simply "m2serve"?
2. _m2serve_connector_resource_access(): looks broken, what about the empty user_access() calls?
3. m2serve_connector.test: do not use t() for assertion messages, that only creates overhead for translators.

But that are not application blockers, so ...

Thanks for your contribution, prateekjain!

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.