A Drupal 6 module:

The Droogle Module integrates Google Docs with Drupal. It provides a list of a configured google user's documents and allows uploading docs to that account. It also exposes much of the google docs api to Drupal users -- you can create collections, move documents into collections, etc. Droogle also works with Organic Groups and each Organic Group can hook up to a unique user account to manage it's group documents. The module's creation is sponsored by Babson College and is to be used to better manage documents for groups, classes, etc. Integrating documents with google means better access to documents on ipads, iphones, etc, since there are many apps for these devices to sync your google docs account(s). Droogle is not to be confused with BBoogle, which is Blackboards integration with Google Docs -- but Droogle works much the same way and serves the same purpose.

We are looking for ideas on how to improve the module to make it as useful as possible in eduction and to the general community. We are open to having co-maintainers, preferably with another college or university.

http://drupal.org/sandbox/barnettech/1327544 (project sandbox page)
http://drupal.org/project/1327544/git-instructions (link to git instructions for the project)
http://drupalcode.org/sandbox/barnettech/1327544.git (link to the project in git)

There is another project that integrates with google but it didn't really work, we tried it, and it didn't work in Organic Groups, and didn't expose a lot of the functions to create users, work with google docs folders, etc. This module is much more plug and play and covers more of the google api. There is also a theme function to wrap google document lists.

Here is a screenshot: http://www.barnettech.com/content/i-will-be-releasing-droogle-module-sho...

REVIEWS I HAVE DONE:
1.) http://drupal.org/node/1379794#comment-5495068
2.) I did a number of reviews in this thread: http://drupal.org/node/1316852#comment-5348664
3.) this thread as well I did a few: http://drupal.org/node/1166144#comment-5347174
4.) http://drupal.org/node/1236914#comment-5517848
5.) http://drupal.org/node/1354558#comment-5517898
6.) http://drupal.org/node/1336946#comment-5518252
7.) http://drupal.org/node/1336946#comment-5519404
8.) http://drupal.org/node/1336946#comment-5523648
9.) http://drupal.org/node/1336946#comment-5524046
10.) http://drupal.org/node/1336946#comment-5524806
11.) http://drupal.org/node/1236914#comment-5557230
12.) http://drupal.org/node/1379794#comment-5557258

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Status: Needs review » Needs work

Zend: appears 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.

Barnettech’s picture

Thanks. I figured the Zend code was open source as well. Does that not matter?

Barnettech’s picture

Ok I've removed the Zend directory and updated the README file on where folks can download the latest Google Gdata api. The readme gives exact instructions on how and where to put the Zend directory.

Let me know what else needs doing.

Thanks,

James Barnett

Senior Drupal Developer / Architect
Babson College
jbarnett@babson.edu

klausi’s picture

Status: Needs work » Needs review

you need to change the status if you want a review.

doitDave’s picture

Status: Needs review » Needs work
FileSize
132.04 KB

I have attached automated review results for your convenience. Did you already check with http://drupal.org/node/1011698 - especially the recommended lectures such as coding standards?

Manual review:

Barnettech’s picture

I'm amazed that drupal.org rejects hard work and sharing of code for minor errors like:

"Line indented incorrectly; expected at least 2 spaces, found 1"

I don't have time to look at such errors for a week or 2 at least. I actually had run it through the coder module already actually and found there were no major errors.

Do folks really treat every minor thing for submission here? I will comply if needed but it will greatly slow down my ability to share. I will put the code on a branch

though, I agree that needs doing ... thank you. Let me know if the "Line indented incorrectly; expected at least 2 spaces, found 1" kinds of things will hold up this

getting accepted.

Barnettech’s picture

Status: Needs work » Needs review

I've made a tag for this, so this should be all set. I've also confirmed that most other major drupal modules that are released as full projects have these messages such as: "Line indented incorrectly; expected at least 2 spaces, found 1"

I did run it through the coder module and all major problems were resolved.

In CTOOLS for example in the drupal 6.x version I'm seeing :

math-expr.inc
Line -1: @file block missing (Drupal Docs)
Line 88: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
    var $suppress_errors = false;
Line 89: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
    var $last_error = null;
Line 91: Arrays should be formatted with a space separating each element and assignment operator
    var $v = array('e'=>2.71,'pi'=>3.14); // variables (and constants)
Line 91: missing space after comma
    var $v = array('e'=>2.71,'pi'=>3.14); // variables (and constants)
jthorson’s picture

Status: Needs work » Needs review

barnettech,

I'm amazed that drupal.org rejects hard work and sharing of code for minor errors

Admittedly, the automated tools which are being used to review applications can be a bit nit-picky ... however, these tools are also revolutionizing the project review process, helping us provide feedback within hours or days of a new application relative to the weeks that it often took before their introduction. These tools flag both 'blockers' and 'suggestions' ... we don't expect every application to be perfect, but we do strongly recommend conforming to the established Drupal coding standards, to maintain code/formatting consistency within the contrib space.

In time, we hope to fully automate the initial review of new project applications using similar automated review tools ... in the meantime, there are some key items hidden within the coding style warnings which will need to be addressed before your application can be approved. Addressing the coding style items while tackling the big-ticket items will help both you and potential reviewers more easily filter out those big ticket items from within the review script output on future follow-ups.

I cannot put the Zend code on drupal.org although Zend provides their code opensource, I don't want to figure out if their license jives with that required on Drupal.org, and I couldn't get Droogle approved on Drupal.org wihtout doing this

There are multiple reasons behind not including 3rd party libraries. The licensing compatibility (and legal implications) is only one of these. (Incidently, anything released on drupal.org is automatically licensed under GPLv2 ... so unless the 3rd party library is also GPLv2, it's a non-starter.)

Another reason is to avoid conflicts when multiple modules attempt to include the same 3rd party library within their code. This can lead to name-spacing conflicts between functions, as well as issues where two modules try to include two different versions/releases of the same library. To avoid these site-killing issues, we recommend that all 3rd party libraries be placed in sites/all/libraries, and leverage the Library API for inclusion.

From Docs.php:

* Zend Framework
*
* LICENSE
*
* This source file is subject to the new BSD license that is bundled
* with this package in the file LICENSE.txt.

If it's subject to BSD, it can't be hosted on drupal.org.

Other items:

Menu item titles and descriptions should NOT be enclosed within t().
Because the code already runs these through t() internally, the t() call in your code redundant.

There should be no trailing spaces
Extra whitespace triggers error messages when others create patches for your module, affecting readability (and reviewability) of the resulting patches.

The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
Self-explanatory.

Function name "getClientLoginHttpClient" doesn't include package part.
Even if it is only example code, your example should properly prefix function names so that folks duplicating your example code can avoid function namespace collisions. Other files in the module also use generic names such as 'uploadDocument' ... prefix these with your module name to ensure a it's unique across an entire site.

Remove all old CVS $Id tags, they are not needed anymore.
This is a leftover from CVS, and are redundant now that we've moved to Git.

Please ensure that these have been addressed and uploaded to the sandbox repository before setting back to 'needs review'. And to avoid another thousand-line automated review output, I'd *strongly suggest* adjusting your indentation to the Drupal standard of two spaces (no tabs).

jthorson’s picture

Status: Needs review » Needs work
Barnettech’s picture

Status: Needs review » Needs work

Thanks for your comments. I just updated my code to fix lots of the items listed above, how can I run an automated report as you would run it so I can avoid changing this to needs review before I catch any further problems?

I ran it through coder and besides some camel case errors I can't avoid because I'm calling a 3rd party library I think I've fixed most errors. I'm not sure if I just hadn't updated git with the latest but I didn't find most of the errors listed above. The ones I did find I went and fixed though.

What can I run to make sure I've gotten everything?

Thanks,

James

Barnettech’s picture

Status: Needs work » Needs review

I've gotten rid of all problems that I know of. Please excuse that this is my first module submission and am doing the best I can to satisfy the requirements. If there is a way for me to run your automated tool to check for problems let me know. I ran things through coder and besides some camel casing which I cannot avoid since I'm calling google's api (which uses camel casing) I believe I've fixed all the problems as requested. I do appreciate you folks guiding me through this process and I'm eager to succeed here. I actually have quite a few other modules I've written here at Babson which I'd like to contribute, which I think others would greatly benefit from. Many of them integrate very nicely with Acquia Commons for any who are interested. We are really looking to galvanize the Drupal education space.

thanks,

James Barnett
Architect Babson College
jbarnett@babson.edu

patrickd’s picture

Status: Needs review » Needs work

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

  • README.txt is missing, see the guidelines for in-project documentation.
  • The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
    ./appsapis.inc
    ./google_tests.php
    
  • ./appsapis.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
    function printCollection2($user=NULL, $password=NULL, $collection_id = '0B4mZRjHDl6MvZTA3NzBiMDctOTQ1NS00MGM5LWE2ZTUtNWYwZWYzMmJhMWI3') {  //happy_folder2 collection
    function xml_attribute($object, $attribute) {
    
  • ./droogle.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
    function microtime_float() {
    
  • ./appsapis.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
    53- */
    
  • ./droogle.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
    14- */
    
  • Remove all old CVS $Id tags, they are not needed anymore.
    droogle.info:1:; $Id$
    
  • All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
    ./appsapis.inc
    

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Barnettech’s picture

Ok now we're in business. I was able to run PAReview myself: All errors have been treated. The google_tests.php file is a command line test script hence the closing ?> php tag, so that is not in fact an error. As far as the README.txt file not having lines exceeding 80 characters I'd like to put my foot down and say it makes my README file less readable and seems ridiculous. Put please let me know if this serves some greater purpose. I saw doItDaves post about how 50% of folks find a lot of these rules as an impediment -- I'm trying to learn from this process and not pass judgement just yet. But just to write a readme file I had to write a shell script to find which lines of my README.txt exceeded 80 characters:

Here's the script

cat README.txt | while read line
do

count=$(echo $line | wc -c)
echo $line $count

done

I'm trying not to have an opinion yet but this seems ridiculous especially since I can barely finish 1 or 2 well polished sentences without going over 80 characters.

Review of the 6.x-2.x branch:

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

I'm hoping to get DROOGLE promoted today! Let me know what else I can do. I ideally would like to help with the Drupal community though my free time is tough to come by these days, so please take my comments as having the correct spirit of promoting progress, success and cooperation.

Thanks,
Best,

James Barnett
Architect Babson College
jbarnett@babson.edu

Barnettech’s picture

Status: Needs work » Needs review

See my comment above and the pareview results with my comments. thanks for the review.

jthorson’s picture

Status: Needs review » Needs work

barnettech,

The 80 character limit is a recommendation to help afford readability and prevent needing to scroll across long lines for folks who are installing your module via an ssh window or the command-line, as many server admins do not install the GUI interface.

For an example of how the readability is affected by long lines, check out the current version of your README.txt file (which is not line-wrapping at 80 characters) at http://drupalcode.org/sandbox/barnettech/1327544.git/blob/refs/heads/6.x...

Other comments:

  1. variable_get() calls should typically include a suitable default value argument for when that variable is not set. As an example, for your 'droogle_title_text' field, I would typically pass a default value of '' along as the second function argument instead of the default NULL, which can help prevent 'undeclared variable' PHP warnings from creeping up in your code. (In fact, it appears that the D6 version of variable_get doesn't even explicitly set NULL if you do not pass a value.)
  2. It appears that your settings form submit routine simply sets a bunch of variables ... I'd suggest investigating returning system_settings_form($form) on your declaration, which would let Drupal take care of the variables for you.
  3. Recommended coding standards: Be consistent with your spacing around { and } characters. The coding standards at http://drupal.org/coding-standards#controlstruct recommend a space before between the control statement and the opening {, and the closing } should be on a line by it self (even if followed by an 'else'). The Drupal standards also call for indentation of two spaces per line, where droogle.module appears to use a single space in sections of the code. While these may seem like minor items, we do want to encourage contrib modules to abide by the standards, providing a consistent look and feel across all contrib module code for both site builders and developers/patch writers.
  4. Your theme functions directly concatenate html and string variables, where those variables come from external sources. I would strongly recommend that these variables be wrapped in sanitization functions such as check_plain() or filter_xss(), rather than implicitly trusting that content provided to you from an external site is not malicious. (The current approach could leave you vulnerable to XSS attacks.)
Barnettech’s picture

Status: Needs work » Needs review

Thank you for your code review.

I've fixed 1, 2, and 4. I ran my module through the coder module and it's not showing any errors for your #3 above. I was very impressed with your review and loved the comments regarding variable_get and system_settings_form($form), and I greatly appreciate the help in having a 2nd set of eyes make sure my code is not vulnerable. These items have been fixed. So we're left with #3. I've already jumped through 100s of hoops for the coder module and fixed up my syntax to comply with drupal.org The coder module already had me fix my spacing regarding "{}" and "()" characters. The coder module is now satisfied with all of my syntax, spacing, etc. There are a few camel casings left because I'm calling google's api and they use camel casing differently. google_tests.php is a command line test so has a closign ?> php tag.

I believe the right thing to recommend is ... if you want different spacing being submitted you should put a patch in for the coder module.

I believe I've satisfied all requirements now?

I'm learning quite a bit from this process, and hope to finally get my project approved this round.

Cheers,

James

jthorson’s picture

Status: Needs review » Needs work

Coding Standards

Use an indent of 2 spaces, with no tabs

I'm glad your module is passing the Coder reviews ... unfortunately, the standard we review against is http://drupal.org/coding-standards and not the Coder module. The first line of the standard on that page reads "Use an indent of 2 spaces, with no tabs." As this is not reflected in your code, the only logical conclusion a I can come to as a reviewer is that noone has yet directed you to the Coding Standards page, which is *strongly recommended* reading for all contributors. Please have a look through the content on that page, and ensure that your code aligns with the established Drupal standards.

I realize that not all published modules are fully compliant ... the standards change and evolve over time, and contributors subsequent modules are not subject to any peer review. That said, one of the primary goals of this review process is to ensure that new contributors are aware of (and willing to abide by) these standards ... the indenting and control structure spacing included in your module suggests that you have not yet been introduced to them.

Repository Structure and Branch Naming

Please review http://drupal.org/node/1015226 for information on tag naming. Your branches are fine, but contrib module tags should follow the 6.x-1.0-beta1 convention, as opposed to 6.x-beta1 (which is the pattern for Drupal core). Also, I'm not certain why you have seperate 6.x-1.x and 6.x-2.x branches at this point; but I suspect it's not necessary. A 2.x branch would be used to indicate a rewrite or a break in backwards compatibility with previous releases, or in some cases the addition of major new feature ... though this final case can still be handled within a point release (when going from 6.x-1.1 to 6.x-1.2) assuming it's still backwards compatible with previous versions.

Finally, I noticed that you have recent commits against both the 6.x-1.x and 6.x-2.x branches, but not all of your commits were applied to both branches of code. Again, I'm not sure if this is intentional or not, but it may result in some of your fixes only being applied against one code stream; and a reviewer who is looking at the other code stream is likely to flag the exact same issue all over again. Generally speaking, my advice for contributors is to stick with a single branch throughout the review process, just to make sure that all of your fixes get included in the final code when it becomes time to promote the project and create an official release ... in your case, I suspect that some of these have been missed (in which case, I'd consider performing a 'git merge' to bring the branches back together again and ensure that all of the fixes are included).

Barnettech’s picture

Status: Needs work » Needs review

Is there any automated tool I can use to highlight my spacing / syntax infractions so I don't miss any? I will clean up the repository as well of course. I am switching from svn to git ... Hence i was a bit clumsy but the latest branch is up to date.

Barnettech’s picture

On the page you referred me to http://drupal.org/node/1015226 it says

Also note:

the Coder module provides an automated way of checking for code standards compliance

So don't say I didn't read on how to work with drupal standards.

Perhaps it is because this is a Drupal 6 module jthorson and you are applying Drupal 7 standards which may have changed?

This is the Drupal 6 version of Droogle. When I submit the module for Drupal 7 I promise to use the Drupal 7 coder module for code standards compliance.

The page you referred me to as I say above confirms I have complied with all code standards for submitting a DRUPAL 6 module.

jthorson’s picture

Status: Needs review » Needs work

I'd be surprised if Coder isn't flagging the spacing and control operator spacing items - make sure you've got it configured to run on 'minor'. For a really critical review, you could also add the Coder Tough Love plugin to Coder - I've yet to see a project that can pass that 100% ... if you can pass the CTL review, you can pass anything. ;)

Other tool options include the PAReview.sh script or Drupal Code Sniffer ... though I don't know for certain whether Code Sniffer flags the indentation or not.

jthorson’s picture

Status: Needs review » Needs work

Just so anyone reading isn't confused, you were referring to the coding standards link, but posted the branch naming link ... the link in the last comment should have been http://drupal.org/coding-standards

So don't say I didn't read on how to work with drupal standards

I didn't say that ... I said that the spacing and control structures within your code suggest that you aren't aware of the standards. The alternative is that you *are* aware of the standards, and blatantly choose to disregard them, in which case any time I spend reviewing your application is wasted time. For your sake, I chose the first interpretation.

The page you referred me to as I say above confirms I have complied with all code standards for submitting a DRUPAL 6 module.

I don't see where that page confirms anything, but your code repository confirms that you have not.

LINE 3: Missing @file declaration

Documenting files
Each file should start with a comment describing what the file does. For example:

/**
 * @file
 * The theme system, which controls the output of Drupal.
 *
 * The theme system allows for nearly all output of the Drupal system to be
 * customized by user themes.
 */

Note that a blank line should follow a @file documentation block.

The line immediately following the @file directive is a summary that will be shown in the list of all files in the generated documentation. (Like other summaries, it should be one sentence in one line of up to 80 characters.) If the line begins with a verb, that verb should be in present tense, e.g., "Handles file uploads." Further description may follow after a blank PHPDoc line.

LINE 12. Incorrect Function Documentation Docblock structure

Notes on function documentation syntax
First line: description

The first line of the block should contain a brief description of what the function does, limited to 80 characters, and beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such").

A longer description with usage notes should follow after a blank line, if more explanation is needed.

After the long description, each parameter should be listed with a @param directive, with a description indented by two extra spaces on the following line or lines. If there are no parameters, omit the @param section entirely. Do not include any blank lines in the @param section.

After all the parameters, a @return directive should be used to document the return value if there is one. There should be a blank line between the @param section and @return directive.

If there is no return value, omit the @return directive entirely

Documenting hook implementations
If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

/**
 * Implements hook_help().
 */
function blog_help($section) {
  // ...
}

LINE 18: Improper Indentation

Indenting and Whitespace
Use an indent of 2 spaces, with no tabs. Lines should have no trailing whitespace at the end.

LINE 18: Extraneous permission
Why declare a 'use google apps' permission that you never use?

LINE 30: Improper use of t().

$output = t(<<<END
<p>Droogle does for Drupal what BBoogle does for Blackboard. We integrate Drupal with google apps and other Google applications.</p>
END

Translators don't necessarily understand code structures such as <<<, especially when normal quotes would suffice.

LINE 38: Missing function declaration docblock
Explanation as per that for line 12, above.

LINE 40: Redundant Code

 $droogle_title_text = variable_get('droogle_title_text', '');
 if (!$droogle_title_text) {
  $droogle_title_text = "DROOGLE: A list of your google docs";
}

should be

  $droogle_title_text = variable_get('droogle_title_text', 'DROOGLE: A list of your google docs.');

LINE 66: Wrong default argument for variable_get()
$allowed = variable_get('droogle_upload', '');
If you're expecting $allowed to be an array (using array_values($allowed) later on), your default value should be an array.

Conclusion

Note ... I'm not even 100 lines into your module. Trust me when I tell you that Coder will *not* catch everything ... it is simply a tool which will help you catch some of the more common issues that are easy to identify through automation.

Also, please keep in mind that those performing reviews are volunteers, giving up their own free time to try and help you through this review process. Getting frustrated, suggesting we don't know what we're talking about, or trying to push toward premature approval of your application is not productive; nor is it conducive to encouraging the reviewer to perform a follow-up review.

Second Opinions & Complaints
If you have an issue with a review or one of the coding standards you're being asked to abide by, then feel free to come to the project applications group page at http://groups.drupal.org/code-review and request a second opinion or file a complaint there.

Barnettech’s picture

I do appreciate your volunteer efforts, I am volunteering to submit this code and all my code in hopes it greatly improves the Drupal education space. The only thing I'm rushing to do is to get the project cleared so I can work on more exciting features and new modules to benefit my employer and others in the community. Now that we are down to spacing and line breaks in doxygen conversation I'm happy that is the only thing left to figure out. I mean no disrespect, I just want to submit this code. I'll seek council from other friends in the boston drupal meetup on how to proceed. I do sincerely thank you and others in this thread for helping in making droogle a better product.

James

Barnettech’s picture

I am working on my DOXYGEN documentation which does need work and is the only thing I know needs doing prior to release.

Barnettech’s picture

Status: Needs work » Needs review

DOXYGEN documentation was added for all files and functions. I cleaned up the naming of my tags. Thank you all for your help in reviewing my first module. As we run toward progress we try not to forget the important details, less we leave our shoes untied and trip so to speak. I do sincerely applaud your efforts.

I reviewed my module and compared the errors in the coding module to other core contributors that I greatly respect -- Moshe Weitzman, MerlinOfChaos, both of them have contributed enormously to drupal. The droogle module now complies to coding standards more than the Migrate module (Moshe) and Chaos Tools module (Merlin).

I am aiming to get an A here, and I certainly think these 2 are respected members of the community -- the coder module is throwing fewer errors to check with droogle than their modules. I am contributing this module because it is good to give back to the community, but I only have so much time to give. These are certainly not billable hours. I'm going to ask a colleague to review my code for me. My colleagues have a great desire to make sure our code is secure and top notch, and I strive to have our team contribute more to the review process, and to contribute as much as we can to the drupal community. I did not have them do it previously because I wanted to just get a feel for the process around here.

I hope you will respect their decision to either set this as "RTBC" or to set this module to "needs review".

I am requesting to have this 3rd party look at this for a final review.

Again I really do appreciate everyone who's contributed another set of eyes to insure the quality of this code.

jthorson’s picture

Anyone is welcome to review an application, and anyone other than the applicant themself is welcome to set it to RTBC. The only request is that your reviewer familiarize themselves with the contents of the Review full project applications documentation, and all referenced standards and security practice pages. This will help ensure that your RTBC status doesn't get bumped back down to 'needs work' under final review by a Git administrator.

On another note, you have not addressed at least five of the items flagged in my last comment, only one of which is a 'spacing or line break' issue. Please ensure that you have fixed in code (or addressed in comments) each of the flagged items before re-setting the application to 'needs review'.

As per your comments regarding other contributors and their modules ... I can appreciate that it seems like a double standard; but comparing your module to other modules which also do not abide by the standards does not change the fact that the standards exist, and are the measure used in this application process. The fact is that a successful application means you could publish other modules without any sort of peer review or abiding by ANY coding standards (not that you should!), which is one reason that we place as much focus as we do on ensuring that you are aware and willing to abide by the standards within *this* application process. Frankly, if you aren't willing to do it here, there is nothing to prevent *your* future modules becoming the one someone tries to use as an example to unsuccessfully justify their own non-compliant code.

Barnettech’s picture

Status: Needs work » Needs review

So I just put the last of the fixes in that you requested jthorson. Again I am learning a lot from this process and actually kind of enjoy it, but always feel pressed for time to move on to my next task. So you are saying this is a one time process and once I have the right to publish modules I wont have this type of review (for better or worse) again? Is this how others are publishing with far more errors than this module has?

If you will be patient with me, I'm willing. What else do I need to fix / learn to submit modules? I'm only frustrated because I like to get a lot done, which usually leads to success, but sometimes it is worth while to look at each detail, I do appreciate that. And it was / is frustrating that others I respect are submitting modules with far more compliance issues.

But I will see this through with an open mind eager to learn and succeed with this process, now and for future modules. What do I need to fix next?

Barnettech’s picture

Just so you know I'm fairly pathetic here refreshing this page waiting for the next thing to learn / fix. Let me know what else is waiting for me to learn / fix -- the module is working great, but I'm eager to gain acceptance by this group and learn your processes.

jthorson’s picture

I don't have time to perform another review tonight ... though I will point out that the default value for variable_get() on your $allowed command should be array() ... I'm not sure if array_values(NULL) throws a PHP warning or not, but since the variable is expected to be an array, an empty array makes a better default value than NULL.

Anecdotally speaking, until very recently, most reviews would go anywhere from a few days to four weeks between follow-ups ... I only mention this to put things in perspective, and as encouragement for you to stop clicking 'refresh'. ;)

Barnettech’s picture

Ok I'll stop hitting refresh then :) boy that was a long few hours ... just kidding. Have a good night. Thanks for the perspective... I had heard you really need to have a co-worker do the review, or someone from a local drupal meetup, but I feel like we're getting there. After I learn the process I can teach my team.

Cheers,

James

jthorson’s picture

I had heard you really need to have a co-worker do the review, or someone from a local drupal meetup

Up until about a couple of months ago, the 'needs review' queue was in the neighborhood of 400 applications long ... so if you wanted a quick approval, that was largely the case. The addition of the automated review scripts, along with some relentless reviewing by a select few helped clear the entire queue ... and with luck, we'll never let it get back to that state again. :)

Barnettech’s picture

Kudos to you all, your efforts have helped with drupals growth and it's become very fun to work with, it's become so easy to impress clients / employers

Barnettech’s picture

Ok the line now reads

 $allowed = variable_get('droogle_upload', array());

Anything else that needs to be fixed?

jthorson’s picture

Status: Needs review » Needs work

So you are saying this is a one time process and once I have the right to publish modules I wont have this type of review (for better or worse) again?

I had answered this question, but then lost my form submission ... and just now realized that I didn't re-answer when I re-typed my reply ... but your statement is correct that this is a one-time process, which is a bit part of the reason that we're so sticky about the seemingly 'little things'.

Anything else that needs to be fixed?

1. Question: Is there any particular reason why, after all this, your module still only uses a single space for each indent, instead of the recommended two?

2. Docblocks are much better, but there are still some inconsistencies. Use 'Implements' instead of 'Implement', ensure your capitalization is consistent with the exampls provided in the standard, remove blank lines above and below an 'Implements hook_x().' declaration (and make sure you include blank lines where the standard asks for them), and add a period at the end of each declaration and description. Ensuring these patterns are followed allows your module to be parsed by Drupal's Doxygen parser, so that help pages for your code can be auto-generated on sites such as api.drupal.org or drupalcontrib.org.

3. Overall spacing is inconsistent ... there are a few spots where you have two empty blank lines between the code and a closing '}', which affects the readability of the code.

4. For readability, inline code comments (lines using '//') should have a space between the '//' and the comment/code.

5. $content = 'got to uploadDocumemt'; This looks like a 'debugging' leftover to me ... all debugging related code should be stripped out of the module before applying.

Really ... I can only stress that you need to make sure your code abides by all of the standards listed at the coding standards page and all pages it links to before setting things back to 'needs review' ... The goal here is to help you determine what items need to be checked before promoting a module to 'full project' status, not for us to provide you a line-by-line summary.

Barnettech’s picture

Question: Is there any particular reason why, after all this, your module still only uses a single space for each indent, instead of the recommended two?

My response is as I've said, I did an audit of many other Drupal modules, many of the authors being leaders in the community and none of them even passed the coder module's standards. So I figured that eventually someone would flip this project to "RTBC". This module does pass the coder module test and Pareview.sh test btw (if anyone else is reading this). I also don't know you at all, don't know your standing in the community and did not know if you were in fact wrong, and the coder module was right ... that my module already complied with coding standards due to it's having passed the coder module automated test.

I will be going through my code today with a fine tooth comb looking to comply with the coding standards page. Again thank you for you time and energy in helping me get this module approved. I know there is a sore lack of folks helping with core bugs, and the module approval process, and I'm not looking to disrespect your helpfulness, I'm just new to the process and doing the best that I can to learn, to be helpful, and to contribute. Those are my only intentions -- and I am sure those are your intentions as well. So again .... thank you despite my many questions. I'm definitely the guy who questions "authority" but that is also how you learn rather than blindly following folks off a cliff.

Cheers,

James

Barnettech’s picture

Status: Needs work » Needs review

I went through the code and addressed all the items requested above in comment #33. I also ran the coder module through looking for minor errors and fixed all of those.

I fixed up the Droxygen especially looking for spacing, punctuation errors, etc.
I fixed it so the spacing for all lines in general used the 2 space indentation.
I put a space after comments (lines starting with //)
I looked at all the lines with a "}" character and it seems ok now.
I read through all of the coding standards page: http://drupal.org/coding-standards#indenting

Barnettech’s picture

Just posted this comment about the review process: http://groups.drupal.org/node/195303#comment-645748 It's all very interesting to see how this works :)

Barnettech’s picture

Been doing my own code review trying to carefully compare the coding standards page to my code. I found some variables today which did not conform and I changed them to lower case separated by underscores versus the camel casing I was using which conformed to Google's coding standard. I see no other things that do not conform to drupal standard.

Now in minor mode on the coder module, there are no errors whatsoever (although some camel casing remains but that is the only way to call the google api -- so must stay, and coder incorrectly says I'm missing some check plain statements which is not the case actually).

Barnettech’s picture

Just started helping with the review process myself of another soul in the review process: http://drupal.org/node/1166144#comment-5347266

Barnettech’s picture

http://drupal.org/node/1166144#comment-5347326 I started helping with the que with what I've learned thus far

Barnettech’s picture

Just commited to my branch after running my code through coder tough love.

Droogle code now passes coder tough love and minor errors checks in coder module, and I manually searched for infractions to drupal.org coding standards

I'm hoping if someone has the time to review this again, there wont be any infractions they could find. Fingers are crossed. I'll continue to help others in their review process as I wait.

James

jthorson’s picture

If you keep posting to your own thread like this, it's going to look to reviewers like the thread is busy, which indicates that someone else is already actively discussing the application with you ... at which point they simply skip over and move on to the next applicant.

Also ... it's considered rude to send unsolicited emails to your reviewers ... if anything, it's going to ensure that they do *not* choose your module the next time they have a few minutes to spare for another review. I've suggested before that you simply need to have a bit of patience, and accept that this process takes time ... and count yourself fortunate that it's not taking four weeks between followups, like it would have had you applied back in March.

klausi’s picture

Status: Needs review » Needs work
FileSize
44.77 KB

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-2.x branch:

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

  • droogle.css: indentation errors, always use 2 spaces per level. See http://drupal.org/node/302199
  • "'#options' => $role_names,": you need to sanitize the role names to prevent XSS vulnerabilities.
  • "watchdog("DROOGLE", "Error: Unable to process for the group nid " . $current_group->nid . " " . $e->getMessage());": do not concatenate the message like this use placeholders for translatability.
  • "drupal_set_message("Error: Unable to authenticate. Please check your credentials.\n" . check_plain($e->getMessage()));": all user facing text should run through t() for translation, and you should use placeholders here as well. Please check all your strings.
Barnettech’s picture

I cleaned up the master branch, just README.txt is there now. Also I moved everything to 6.x-1.x since there was no need for 6.x-2.x yet, since there is no major release.

Another person reviewed my module at the Boston Drupal Meetup and I fixed some items in response to that review. Google passwords are encrypted in the database, the Droogle OG settings on the settings page show conditionally only if OG is even installed.

Klausi: I fixed up the drupal_set_message to use the t( function appropriately, thank you.

Klausi: As I've posted google_tests.php is a file to run at the command line as described in the README.txt file and so needs the closing ?> php tag

Klausi: At the Drupal meetup in Boston we were cleaning up my repository and it seems to me the repository was a bit messed up after that session, I've now fixed it up and the sandbox should now be showing the most up to date code in 6.x-1.x Sorry for this confusion.

Klausi: I've run coder in minor mode (using the DRUPAL 6) module and there are absolutely no errors showing. I previously ran pareview on my code and there were no errors. Now I run it and there are many, but many are applying Drupal 7 and Drupal 8 coding standards and don't apply to a Drupal 6 module and in fact I'm confused if a Drupal 6 module should comply with Drupal 6 standards or Drupal 7 standards -- my guess is it depends on the reviewer, so I could end up in an endless loop fixing things to Drupal 6 standards for one reviewer and Drupal 7 standards for another? Here is one such suggestion from pareview which is a change in Drupal 7 coding standards: 44 | ERROR | Missing parameter type at position 1 In Drupal 7 you would do: @param bool $flagged but in drupal 6 you were required to do: @param $flagged

Most of the errors in pareview are spacing errors and coder says there are no errors in this area. Because coder in minor mode is geared to Drupal 6 and pareview is setup to review Drupal 7 and Drupal 8 modules I am leaving the rest of my code unchanged, since coder and coder tough love in Drupal 6 say there are absolutely no further problems in minor mode. I also see you ran drupal code sniffer but that is also a Drupal 7 tool (http://drupal.org/project/drupalcs). When I upgrade this module to Drupal 7 I will be sure to use these new tools.

Let me know what else I need to do, and I will willingly listen and comply (even if your thoughts conflict with my thoughts written here). I will be submitting a Drupal 7 version of this module within the next 6 months or so btw. Right now we are using Acquia's distribution "Drupal Commons" (https://network.acquia.com/downloads/drupal-commons) and this distribution is still on Drupal 6. My group at Babson will be assisting Acquia in upgrading this distribution to Drupal 7 shortly and I'll upgrade Droogle at that time to Drupal 7 as well.

Thank you for your review.

Thanks,

Barnettech

klausi’s picture

Status: Needs work » Needs review

From https://drupal.org/coding-standards

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version.

Your module is new code, so you should follow the new standards.

Barnettech’s picture

Status: Needs review » Needs work

Ok, no problem.

Barnettech’s picture

Status: Needs work » Needs review

I just spent a few hours cleaning this up. There are no longer any errors on Pareview. Let me know what else needs doing. thanks!

klausi’s picture

Status: Needs review » Needs work
FileSize
955 bytes

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

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:

  • droogle_hello(): you are calling an organic_group function unconditionally here, so you must depend on og in your info file.
  • git commit messages: "oy" doesn't tell me much, please provide useful commit messages, see http://drupal.org//node/52287
  • "* A boolean and if set to true the cache is cleared and": This comment breaks at 56 characters, but you should make use of the full 80 character limit as long as a word does not exceed that limit. See http://drupal.org/node/1354#general "They should be as long as possible within the 80-character limit." Please check all your comments.
  • droogle_settings_form_submit(): This is not a hook, see http://drupal.org/node/1354#forms
  • droogle_settings_form_submit(): why do you call system_settings_form() here?
  • droogle_settings_form_submit(): $decrypted variables are never used in this function.
  • droogle_hello(): $path is undefined. Please enable PHP notices in your development environment to catch such undefined variables errors.
  • "$content .= 'The group has no google docs presently';": all user facing text should run through t() for tranlation. Please check all your strings.
  • Have considered to move the og functionality out into a droogle_og submodule?
  • "Implements hook block": comment should be "Implements hook_block()."
  • theme_droogle_list_docs(): inline comments in a function body should use the "//" comment style, "/* .. */" is discouraged. See http://drupal.org/node/1354#inline
  • "t('File %file was uploaded to google docs', array('%file' => check_plain($form_state['values']['file']->filename)))": no need to use check_plain() here, the "%" placeholder already runs check_plain() for you. And you should avoid double escaping.
  • droogle_print_collection2(): are you sure that you need cURL? why can't you use drupal_http_request()?
  • "check_plain($the_file_url)": $the_file_url is a URL, so you should use check_url().

As you might know I started #1410826: [META] Review bonus, so feel free to list any reviews in the issue summary that you have done and add the tag, after you fixed all issues in your own module.

Barnettech’s picture

droogle_settings_form_submit(): why do you call system_settings_form() here?

Comment above (http://drupal.org/node/1328366#comment-5308244) encouraged me to use this function, so I did. I actually did not know about it and finds it saves a few lines of code here. Then I want to do some special handling of passwords, so still use function droogle_settings_form_submit($form, &$form_state) for this.

Have considered to move the og functionality out into a droogle_og submodule?

Yes, and I chose not to, the separate module would only be about 20 lines of code with hook_menu and then a callback which would just call the api function. Thanks for the suggestion.

droogle_print_collection2(): are you sure that you need cURL? why can't you use drupal_http_request()?

A lot of these API functions were made to be as generic as possible for use from the command line without the need to bootstrap drupal.

pareview now reports no errors, all items mentioned above have been fixed. Thank you for your review Klausi. I am in the middle of reviewing another applicants module but it is fairly involved and is taking a while: http://drupal.org/node/1379794, although I already ran pareview on his project and did a quick manual review of the code (I did leave one comment for him you'll see). I'll install the project and will then do another manual review for him. Regardless if my module gets approved I will do that review of course. I am looking to get more involved in the community.

Barnettech’s picture

Issue tags: +PAreview: review bonus

Adding PAReview: review bonus tag, I've done quite a few rounds of reviews

klausi’s picture

Don't forget to set the status to "needs review" if you have completed your improvements.

Barnettech’s picture

Status: Needs work » Needs review

I have completed the improvements thank you, when I put in the tag it defaulted back to needs work, thanks Klausi

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus
  • droogle.css: indentation errors, always use 2 psaces per level.
  • droogle.css: this file contains URLs that are not existent on every site. Remove them and do CSS overrides in your site's theme.
  • "t('<p>Droogle does for Drupal what BBoogle does for Blackboard. We integrate Drupal with google apps and other Google applications.</p>')": The p tags should be outside of the t() function call.
  • "'#options' => $role_names,": you need to sanitize the role names before printing them to avoid XSS exploits via malicious role names. I know, D6 core also has this vulnerability, but you should do it the safe way nevertheless. See also #316136: New role name not filtered into admin/user/permissions
  • "watchdog("DROOGLE", "Error: Unable to process for the group nid " . $current_group->nid . " " . $e->getMessage());": do not concatenate variables into the translatable message like this, use placeholders instead.
  • "$allowed = variable_get('droogle_upload', array());": you should not save allowed roles like this. Create a new permission "droogle upload" and let the permission system handle which role is allowed to use it. Then just use user_access() to check if a user is allowed to upload stuff.
  • "$key = 'droogle_bl@h';": this should be a constant defined at the top of your module.
  • "'#value' => 'Submit',": all user facing text should run through t() for translation, please check all your strings.
  • "form_set_error($fieldname, 'Error uploading file.');": same here, please use t().
  • "$form['#attributes']['enctype'] = "multipart/form-data";": why do you need that?
  • droogle_settings_form_submit(): it is enough if you invoke system_settings_form() in the form building function, you don't need it in your custom submit handler. Just remove the line here.

I'm removing the review bonus tag, you can do some more reviews and assign it back to you then if you want.

Barnettech’s picture

Status: Needs work » Needs review

All items in #52 above have been fixed.

"$form['#attributes']['enctype'] = "multipart/form-data";": why do you need that?

the answer is yes, no files can be uploaded to google docs without this being set.

Barnettech’s picture

Issue summary: View changes

detailed some of the reviews I've done while waiting for my project to be reviewed.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

Barnettech’s picture

Issue tags: +PAreview: review bonus

Just finished doing 3 reviews, so adding the PAReview: review bonus tag.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus
FileSize
1.9 KB

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

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:

  • droogle.css: the url path to the images is hardcoded. This will not work if i have this module in another folder like sites/example.com/modules or profiles/recruiter/modules for example. You will have to find another way to add the pictures.
  • "@theerror", array('@thenid' => $current_group->nid, '@the_error'": spelling mistake in the placeholder name.
  • droogle_upload_form() and "multipart/form-data": but the form does not upload to Google directly, the file is submitted to Drupal first. What am I missing?
  • Also: "// Set the form encoding type." is an example for a bad comment. You should not document what you are doing (for simple code lines like this), you should document why you are doing it.
  • "form_set_error($fieldname, 'Error uploading file.');" once again: all user facing text should run through t() for translation. Once again: please check all your strings in all your code. Everytime I'll encounter an untranslated string in you module now I will bump this back to "needs work" immediately ;-)
klausi’s picture

Issue summary: View changes

Updated issue summary.

Barnettech’s picture

Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Thank you Klausi, all has been fixed. I've actually been doing reviews for the last 2 days, and got a bit obsessed with it, so added some of the links above and am flipping it back to have the PAReview: review bonus tag. I wonder if you guys could run a git post commit hook to run pareview and auto post to the thread if there are errors? It would save you a lot of time having to run it manually. You probably are already thinking along those lines.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

themebrain’s picture

Status: Needs review » Needs work

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

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.

Source: http://ventral.org/pareview - PAReview.sh online service

Manual review:
- The sequence strings in translate file must be matched with the string in t function, so here be careful with break line and multiple spaces. The same in other places. For example:
drupal_set_message(t('the Droogle og password was not entered, every time this form is saved all passwords must be re-entered, if not
using Droogles og feature please ignore this message'));
- Please removed the debuging code before submit such as:
// print_r($feed);
- A lot of inline comments using /* */, it seem different from the comment formatting conventions http://drupal.org/node/1354
- Some variables don't match with the naming convention http://drupal.org/coding-standards#naming, for example:
$Entry = '';
$Url = "https://docs.google.com//feeds/default/private/full/folder%3A" . $collection_id . "/contents";

Barnettech’s picture

Status: Needs work » Needs review

I fixed the all items mentioned in comment #57 although in this comment:

The sequence strings in translate file must be matched with the string in t function

I looked at your example and the code works and as well conforms to the drupal.org style guide

Let me know what else needs fixing and what else I may have missed and I will diligently fix it. Thank you.

themebrain’s picture

Status: Needs review » Reviewed & tested by the community

It looks good now

klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus
  • "droogle_add_permission($document_id = '1mKjGghPEU2hfcoU82gPPXT2G0wkBPrIQtPch01-X3BA', $user = 'drupal_development@babson.edu')": I think those default values are not useful to anyone but you. You should avoid hard coding of very specific settings that do not make sense for other users.
  • appsapis.inc: is this code that should be useable outside of Drupal? Then it should not contain any Drupal functions (e.g. drupal_set_message(), theme()). Or is it a full part of the Drupal module? Then all user facing text (error messages!) should run through t() for translation.
  • ""source" => "your application name",": should't you use your application name, i.e. "droogle" here?
  • "echo $result;": do not print content directly like this if you return it anyway. Same for "echo $ex->getMessage();" Also in other places.
  • theme_droogle_list_docs(): use the l() function to generate link markup.
Barnettech’s picture

Status: Needs work » Needs review

Everything was addressed in comment #60 above. Thank you for the review.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

Barnettech’s picture

Issue summary: View changes

Updated issue summary.

Barnettech’s picture

Issue tags: +PAreview: review bonus

Adding Pareview bonus tag back after having done some more reviews.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

manual review:

  • "l(t(check_plain($file->title))...": no need to use check_plain() here, the l() function will do that for you.

But that it just a minor issue, otherwise I think this is RTBC. Thank you for your patience and your endurance during this process! Thanks also for helping out in other applications.

klausi’s picture

Issue tags: -PAreview: review bonus

Removing review bonus tag.

zzolo’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, @barnettech! 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.

Barnettech’s picture

Thanks for all who helped with my review. I'm excited to take this step toward getting more active in this great community.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.