Entityconnect

Hey!
I ran a review using http://ventral.org/pareview, with following result:

FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/entityconnect.module
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
66 | ERROR | Whitespace found at end of line
76 | ERROR | Whitespace found at end of line
85 | ERROR | Whitespace found at end of line
95 | ERROR | Whitespace found at end of line
106 | ERROR | Whitespace found at end of line
109 | ERROR | Whitespace found at end of line
110 | ERROR | Multi-line function declaration not indented correctly; expected
| | 0 spaces but found 8
127 | ERROR | Whitespace found at end of line
137 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: ...ll/modules/pareview_temp/test_candidate/includes/entityconnect.form.inc
--------------------------------------------------------------------------------
FOUND 29 ERROR(S) AND 2 WARNING(S) AFFECTING 28 LINE(S)
--------------------------------------------------------------------------------
12 | ERROR | Arguments with default values must be at the end of the
| | argument list
16 | ERROR | Whitespace found at end of line
60 | ERROR | Whitespace found at end of line
83 | ERROR | Whitespace found at end of line
104 | ERROR | An operator statement must be followed by a single space
108 | ERROR | Whitespace found at end of line
117 | ERROR | No space before comment text; expected "// if()" but found
| | "//if()"
136 | ERROR | Whitespace found at end of line
149 | ERROR | Whitespace found at end of line
150 | ERROR | There must be a single space before an operator statement
150 | ERROR | Whitespace found at end of line
154 | ERROR | Whitespace found at end of line
155 | ERROR | Inline comments must start with a capital letter
157 | WARNING | Line exceeds 80 characters; contains 81 characters
157 | ERROR | There must be no blank line following an inline comment
158 | ERROR | Whitespace found at end of line
160 | ERROR | Whitespace found at end of line
163 | ERROR | Whitespace found at end of line
166 | ERROR | Whitespace found at end of line
167 | ERROR | Inline comments must start with a capital letter
169 | WARNING | Line exceeds 80 characters; contains 81 characters
169 | ERROR | There must be no blank line following an inline comment
170 | ERROR | Whitespace found at end of line
172 | ERROR | Whitespace found at end of line
174 | ERROR | There must be no blank line following an inline comment
175 | ERROR | Whitespace found at end of line
182 | ERROR | Whitespace found at end of line
195 | ERROR | You must use "/**" style comments for a function comment
218 | ERROR | You must use "/**" style comments for a function comment
222 | ERROR | Whitespace found at end of line
235 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...ll/modules/pareview_temp/test_candidate/includes/entityconnect.menu.inc
--------------------------------------------------------------------------------
FOUND 16 ERROR(S) AND 2 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
20 | ERROR | Whitespace found at end of line
22 | WARNING | Line exceeds 80 characters; contains 97 characters
33 | ERROR | Whitespace found at end of line
37 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
41 | ERROR | Whitespace found at end of line
42 | ERROR | An operator statement must be followed by a single space
42 | ERROR | There must be a single space before an operator statement
42 | ERROR | Whitespace found at end of line
49 | ERROR | There must be no blank line following an inline comment
51 | WARNING | Line exceeds 80 characters; contains 81 characters
51 | ERROR | Whitespace found at end of line
51 | ERROR | Inline comments must start with a capital letter
56 | ERROR | Whitespace found at end of line
63 | ERROR | Whitespace found at end of line
64 | ERROR | Whitespace found at end of line
67 | ERROR | Whitespace found at end of line
72 | ERROR | Whitespace found at end of line
80 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

FILE: ...es/all/modules/pareview_temp/test_candidate/theme/css/entityconnect.css
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Expected 1 space before opening brace of class definition; 0
| | found
6 | ERROR | There must be no space before a colon in a style definition
7 | ERROR | Expected 1 space after colon in style definition; 0 found
9 | ERROR | Expected 1 space after colon in style definition; 0 found
17 | ERROR | Expected 1 space before opening brace of class definition; 0
| | found
21 | ERROR | Expected 1 space after colon in style definition; 0 found
22 | ERROR | There must be no space before a colon in a style definition
28 | ERROR | Expected 1 space after colon in style definition; 0 found
--------------------------------------------------------------------------------

FILE: ...ites/all/modules/pareview_temp/test_candidate/theme/js/entityconnect.js
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
14 | ERROR | Expected 0 spaces before closing bracket; 1 found
27 | ERROR | Whitespace found at end of line
28 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

You may want to consider configuring your development IDE to follow Drupal's coding standards as best as possible.

I'm a little confused about which branch to review, you got an 2.x but the 1.x is more recent, did you unintentionally created the 2.x one or does it contain yet-not-stable code?

as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Automatic Review:

Please visit below page, i will suggest you to remove the following warning and errors:-
http://ventral.org/pareview/httpgitdrupalorgsandboxjygastaud1445640git

Manual Review:

1. Hook_menu : try to define array of variable and then return it, and also you have not mention the 'access argument' which will make your project urls more secure.
2. Please remove the extra code which you don't needed and you have commented it out.
3. In entityconnect.menu.inc > function entityconnect_edit($cache_id) { you are returning return $cache_id; but before this you are calling drupal_goto() which will not allow next statement to call it.
4. Remove the those cases which you are not implementing.
5. You should writing secure code, coding standards.

Hi jygastaud,

please keep discussions in the issue,

So, I have a small question about bonus review. I made reviews for 5 or 6 different projects. I add access to the review bonus tag when I add a new comment. However I am not some I am really able to decided myself that my project should be promote.
I am asking beacause I saw at least 2 projects promoted by their owner to review bonus project.

If you've done 3 or more manual reviews you can simply add the review bonus tag your self.

But, I had a quick look at your reviews an I got a few problems with them that you can improve for future reviews:

• Don't copy the whole automated reviews into the issue. They make the issue look clumsy and can be quite demotivating as the applicant "hits the wall of errors". It's much better when you attach a file or a direct link. Also don't forget that most of them are suggestions.
• The main requirement for getting a review bonus is that you're in fact doing a manual review of the code, unfortunately most of the manual reviews you've done look like you've repeated some findings of automated tools.

Don't get me wrong, I'm very happy that you've decided to help out in the queue!
But it would be great if you'd spend a little more time and effort on your reviews :)

It's absolutely okay if you first point the applicant to automated reviews at the beginning (I do it the same way) but as mentioned above, just don't paste in the whole review. ;)

Reviews of other projects

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: ...es/all/modules/pareview_temp/test_candidate/theme/css/entityconnect.css
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    6 | ERROR | Multiple CSS properties should be listed in alphabetical order
    7 | ERROR | Multiple CSS properties should be listed in alphabetical order
   20 | ERROR | Multiple CSS properties should be listed in alphabetical order
   22 | ERROR | Multiple CSS properties should be listed in alphabetical order
  --------------------------------------------------------------------------------
  

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:

• entityconnect_menu(): all your menu items are unprotected, meaning any anonymous user can access the paths. Please read the documentation of hook_menu() again and fix 'access callback'.
• entityconnect_check_access(): doc block is confusing, as this function is never called currently.
• entityconnect_form_alter(): why do you access $_REQUEST here? Why can't you use $form_state or $form?
• entityconnect_cache_set(): do not repeat the function name at the beginning of the doc block. Begin with a verb in the form "Does such and such" http://drupal.org/node/1354#functions . Also elsewhere.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Great job getting PAReview to show absolutely no errors. That's a tough job, and I'm impressed. Plus your wysiwyg_exclude? Doubly impressed.

Applying Drupal's branching strategy with our development workflows is rather tricky, ain't it? Alright, so the 1.x, 2.x, 3.x kind of stuff is for api changes. If a module that extended your module would totally break after you pushed some code, you need to move up an increment. If you have not made an official release, feel free to make as many api changes and rewrites as you want without increasing a major version. Although, oftentimes people will increment a major version if they're doing a significant rewrite. Others will need to do a major version increment even if they are on dev just because they are so popular and have been on dev way too ridiculously long... pathauto

In your case, it looks like you wanted to create some experimental changes without messing up your current stable branch. Awesome, that is exactly how you should do it, and that is why Git is awesome. But, you shouldn't be using the 7.x-x.x branch naming scheme for those sorts of experimental branches. How you name them could be entirely personal preference. It could be the issue number plus description, like "1456272-refactor_forms", or something more explicit like "7.x-1.x-refactor_forms" which clearly demonstrates what it is a branch from.

Great README.

Your .gitignore seems to be personal to you. Not a good idea, you should probably take that out. That could be tough for your workflow I know. If you let me know your use case I may be able to give you some tips to work around it. Not a blocker, but you'll want to remove it.

Your .info file surrounds some values with quote, and others with nothing. It'd be best to be consistent. Surrounding with quotes is best. Minor, not a blocker.

You use arg() a bunch, and we're trying to reduce that usage. Minor, not a blocker. Just something to think about.

Appropriate usage of hooking into Drupal's jQuery behaviors.

You tackled a rather significant project and demonstrated a solid understanding of Drupal community standards, FormAPI, entities (duh), and how to progressively enhance through module development (++). Just what we want in a contributor, so I for one would like to welcome you with open arms.

manual review:

• "'#language' => 'und',": use the constant LANGUAGE_NONE.
• "$item['href'] = 'admin/people/create/' . $cache_id . '';": why do you need to append the empty string at the end?

But that are just minor issues, so ...

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

Congratulations, jygastaud! You are now a "vetted" git user (with a good understanding of git tricks!) and can promote experimental sandboxes to full projects.

Thank you all for solid reviews, looks like there is more work before a full release but perhaps it is in alpha. Please review the recommendations above, at the least, before releasing.