PersonalTube

Please review the details on the Applying for permission to create full projects page. The application process requires that you provide a link to the code via a sandbox repository hosted here on Drupal.org ... we can not review code downloaded from a 3rd party website.

Nope, this issue will be sufficient.

Currently, the backlog in the project application queue is in the neighborhood of 6 weeks. We are diligently working to clear the queue, but it may take a few weeks before someone gets to your application.

In the meantime, I'd suggest visiting the new Tips to ensure a smooth review page, and performing a self-review to ensure that your module meets all of the expectations listed on that page. This will help ensure that, when someone does get around to reviewing your application, they won't flag any issues which will further delay the acceptance of your application.

Thanks in advance for your patience!

I did a quick Coder review to check the Coding Standards, see below.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/personaltube/personaltube.module:
 +2: [normal] Commits to the Git repository do not require the CVS $Id$ keyword in each file.
 +5: [minor] indent secondary line of comment one space 
 +6: [minor] indent secondary line of comment one space 
 +7: [minor] indent secondary line of comment one space 
 +8: [minor] indent secondary line of comment one space 
 +9: [minor] indent secondary line of comment one space 
 +10: [minor] indent secondary line of comment one space 
 +16: [normal] Use an indent of 2 spaces, with no tabs
 +17: [normal] Use an indent of 2 spaces, with no tabs
 +18: [normal] Use an indent of 2 spaces, with no tabs
 +19: [normal] Use an indent of 2 spaces, with no tabs
 +29: [normal] Use an indent of 2 spaces, with no tabs
 +30: [normal] Use an indent of 2 spaces, with no tabs
 +39: [normal] Use an indent of 2 spaces, with no tabs
 +40: [normal] Use an indent of 2 spaces, with no tabs
 +41: [normal] Use an indent of 2 spaces, with no tabs
 +42: [normal] Use an indent of 2 spaces, with no tabs
 +51: [normal] Use an indent of 2 spaces, with no tabs
 +53: [normal] Use an indent of 2 spaces, with no tabs
 +54: [normal] Menu item titles and descriptions should NOT be enclosed within t().
 +54: [normal] Use an indent of 2 spaces, with no tabs
 +56: [normal] Use an indent of 2 spaces, with no tabs
 +57: [normal] Use an indent of 2 spaces, with no tabs
 +58: [normal] Use an indent of 2 spaces, with no tabs
 +86: [normal] Use an indent of 2 spaces, with no tabs
 +87: [normal] Use an indent of 2 spaces, with no tabs
 +88: [normal] Use an indent of 2 spaces, with no tabs
 +88: [normal] Control statements should have one space between the control keyword and opening parenthesis
 +88: [normal] use a space between the closing parenthesis and the open bracket
 +89: [normal] Use an indent of 2 spaces, with no tabs
 +90: [normal] Use an indent of 2 spaces, with no tabs
 +91: [normal] Use an indent of 2 spaces, with no tabs
 +92: [normal] Use an indent of 2 spaces, with no tabs
 +93: [normal] Use an indent of 2 spaces, with no tabs
 +94: [normal] Use an indent of 2 spaces, with no tabs
 +95: [normal] Use an indent of 2 spaces, with no tabs
 +96: [normal] Use an indent of 2 spaces, with no tabs
 +97: [normal] Use an indent of 2 spaces, with no tabs
 +98: [normal] Use an indent of 2 spaces, with no tabs
 +99: [normal] Use an indent of 2 spaces, with no tabs
 +100: [normal] Use an indent of 2 spaces, with no tabs
 +101: [normal] Use an indent of 2 spaces, with no tabs
 +102: [normal] Use an indent of 2 spaces, with no tabs
 +103: [normal] Use an indent of 2 spaces, with no tabs
 +104: [normal] Use an indent of 2 spaces, with no tabs
 +105: [normal] Use an indent of 2 spaces, with no tabs
 +106: [normal] Use an indent of 2 spaces, with no tabs

Status Messages:
 Coder found 1 projects, 1 files, 41 normal warnings, 6 minor warnings, 0 warnings were flagged to be ignored

There are some minor coder issues, but nothing seriously

Can you rename the variable 'configuration_string' it's a bit to generic, 'personaltube_configuration_string' would be better.

Can you also validate the input on the admin screen, and make sure $array_split = explode('#@#', $configString); returns enough elements

Review of the 7.x branch:

• ReadMe.txt should be README.txt
• Remove all old CVS $Id tags from all files, not needed anymore
• Remove "version" from your info file, this added by drupal.org packaging automatically
• no need to include license references in the info file, a GPLv2 LICENSE.txt is added by drupal.org packaging automatically. Also in the other files.
• "array( 'title' => 'Access PersonalTube', 'description' => 'Perform maintenance tasks for PersonalTube', ) );": All user facing text must run through t() for translation. There should be no space before ")".
• personaltube_menu(): indentation errors, use 2 spaces per level. "array( 'title' => 'personaltube'," this array should be one indentation level deeper.
• personaltube_block_view(): indentation errors, function bodies should be indented 2 spaces. Please re-visit the coding standards: http://drupal.org/node/318#functdecl

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/personaltube.module:
   +4: [minor] indent secondary line of comment one space 
   +5: [minor] indent secondary line of comment one space 
   +6: [minor] indent secondary line of comment one space 
   +7: [minor] indent secondary line of comment one space 
   +8: [minor] indent secondary line of comment one space 
   +9: [minor] indent secondary line of comment one space 
  
  Status Messages:
   Coder found 1 projects, 1 files, 6 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  personaltube.module:26:  return array('access personaltube' => array( 'title' => 't(Access PersonalTube)', 'description' => 't(Perform maintenance tasks for PersonalTube)', ) );
  personaltube.module:33:  $blocks['personaltube'] = array( 'info' => t('PersonalTube'), );
  personaltube.module:41:  return array( 'admin/config/personaltube' =>
  personaltube.module:42:    array( 'title' => 'personaltube',
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• "return array('access personaltube' => array( 'title' => 't(Access PersonalTube)', 'description' => 't(Perform maintenance tasks for PersonalTube)', ) );": you need tu use t() as function outside of the string.
• "access personaltube" that permission is not checked anywere, so it is without effect.
• personaltube_block_view(): The markup in there is vulnerable to XSS attacks. Make sure that you sanitize the variable contents when embedding it. See http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...
• "'#description' => 'Enter the Drupal Configuration String from your PersonalTube account',": All user facing text must run through t() for translation. Please check all your strings in your module.

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  personaltube.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• personaltube_perm(): this function is empty now?
• "$block_content .= check_url('http://app.personaltube.com/script/personaltube.js');": you don't need to sanitize this, as this is not user provided input. Also elsewhere.
• "$block['content'] = filter_xss($block_content, array('div', 'script'));": so the filter_xss() call here is useless because the script tag is allowed anyway?
• "$block_content .= '
  

  ';": if you want to add javascript to a page you should use drupal_add_js(). Also in the other cases.

• "$block_content .= 'var PersonalTube_Organization_Name = "' . $array_split[0] . '";';": $array_split is user provided input, so you need to do sanitization here. Please read http://drupal.org/node/28984
• "'access arguments' => array('administer media personaltube'),": you need to define that permission in hook_perm().
• "'#default_value' => check_plain(variable_get('personaltube_configuration_string', '')),": no need to use check_plain() here, the Form API sanitizes #default_value for you.

Hi,

either you didn't properly check in the update or there was some misunderstanding regarding unix style line endings. When I download your latest snapshot, the module file is still in "Windows" line endings mode (automatted review shows the same result). Please check again and make sure you have configured your editor properly.

Additions:

• Implementation of drupal_add_js() looks consistent.
• I would recommend renaming $array_split into something more verbose, like $config_details. This is no must-do, but would ease readability for others and even yourself some day ;))

I think this is not far from "ready to go".

Hi,

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/personaltube.module:
   +6: [minor] There should be no trailing spaces
   +59: [minor] There should be no trailing spaces
   +75: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./personaltube.module ./personaltube.info ./README.txt
  

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.

Manual review:

• I consider the line endings as minor and non-blocking. You should nevertheless remove them for perfection's sake ;)
• Linebreak at file ending is a false positive as I find them in the latest 7.x snapshot.
• The comments section in the .info file is not needed. Also I doubt that the (c) notice complies with the GPL that your module will be released under. ;)
• Same for the copyright notes in .module file.

BUT! Sorry to not have seen that before - some issues remain in the other branches:

Master branch: You should clean it up so there remains none but the readme file that instructs anyone to check with one of the major release branches.

Tags: AFAIK, the are not used unless you really create releases in a full project; IMO they should also be removed.

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  personaltube.module
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./personaltube.module ./personaltube.info ./README.txt
  

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.

Manual review 6.x-1.x:

• Comment section in the info file and copyright notices: See 7.x branch remarks.
• You should fix all issues mentioned for the 7.x branch (e.g. drupal_add_js) accordingly.

Sorry again for not looking at that sooner. But I think these are all minor things that you will have fixed soon so I personally do not see much left on the way :)

HTH, dave

No, the linebreaks at the end of a file (one single new line) are not a false positive. They are defined in the coding standards:

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file.

Hi klausi, I did not mean that the requirement is wrong. I never would ;)

But there is *some* strange issue with that point. Just download a snapshot from the repo viewer in the cases where I've marked "false positive (IMO)". When I check control samples, there definitely *are* \n at the file's end. Strange enough, this is not always the case; sometimes the complaint is correct. Of course I cannot judge this for sure, neither the causes if there are any.

Perhaps we can get together aside from here to verify the reason? I installed the script exactly by your instruction page. Of course it may be that there remains some misconfig on my side. Whatever. Just drop me a line/pm? :) Cheers!

Remember to update your issue status after posting changes. :)

Hi,

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./personaltube.info:   ASCII English text, with CRLF line terminators
  personaltube.info
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./personaltube.module ./personaltube.info ./README.txt
  

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.

So still finding the CLRF issue in the 6.x-1.x branch, forgot to commit? Also IMO you should clean the existing tags in the repo.

HTH, dave

Hi,

aside the line endings, no more issues found in the automated review. Except for the line endings issue that I consider inconsistent (also, newlines exist).

Looking manually:

Uninstall routine:
You set up Drupal variables, but do not clean them up on uninstall. That should be fixed in an install file.

Security:
At first I was uncertain whether the implementation of the config string would leave some backdoor for XSS, but I didn't find a way to bypass check_plain() as it is used here. So this looks ok.

Coding standards:
There are @author and @copyright infos in the @file docblocks. AFAIK, they should be removed, but I leave this up to someone else to clarify.

Other (just thinking out loud, please may also someone knowing better object to this):
The way you add JS. I am aware that D6, other than D7, has no "official" syntax for adding external scripts. You are aware as well, your code shows that. For D6, drupal_html_head() could be a better choice (see e.g. http://drupal.org/node/91250). But please take this just as a note from me, I really cannot judge it as a reviewer yet.

Edit: Found no other remains.

Ok, looks good for me now.

• Found no security issues
• No coder issues except (IMO) false positives on missing newlines
• Clean comment blocks.
• No 3rd party code included.

Only remain, please may someone re-check this:

Copyright notice in all files - compatible with GPL? If not, please re-set to "needs work" and make a statement, I couldn't find documents on this.

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...al-7/sites/all/modules/pareview_temp/test_candidate/personaltube.module
  --------------------------------------------------------------------------------
  FOUND 5 ERROR(S) AND 2 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
    38 | WARNING | Last item of an inline array must not followed by a comma
    78 | ERROR   | Variable "configString" is camel caps format. do not use mixed
       |         | case (camelCase), use lower case and _
    79 | ERROR   | Variable "configString" is camel caps format. do not use mixed
       |         | case (camelCase), use lower case and _
    80 | ERROR   | Variable "configString" is camel caps format. do not use mixed
       |         | case (camelCase), use lower case and _
   104 | WARNING | Line exceeds 80 characters; contains 81 characters
   107 | ERROR   | Variable "configString" is camel caps format. do not use mixed
       |         | case (camelCase), use lower case and _
   108 | ERROR   | Variable "configString" is camel caps format. do not use mixed
       |         | case (camelCase), use lower case and _
  --------------------------------------------------------------------------------
  

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.

manual review:

• personaltube_perm() in 6.x-1.x: this hook has to return a simple array of permission strings only in Drupal 6.
• remove the copyright information from the file headers, you can give credit in README.txt

Otherwise I also think this is nearly ready.

Hi, did you just forget to remove the copyright notice in some files except readme or is there a deeper background?

Just to make sure, are you aware that everything you publish here will be explicitly subject to GPL?

Hi,

Coding standards

Now looks clean, no more warnings in drupalcs.

hook_perm() (D6 branch), ref. #29

You really only provide bare english strings, one per permission, all embedded in a flat array. Please do check again with the D6 api docs.

This should be all IMO.

Hi, this is really easy (see here): Just return array('funny permission identifier'); if you have only the "funny" permission.

hth,
dave

Found no more issues; drupalcs/coder are clean and anything else has already been discussed and solved.

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