Screen9

What is the purpose of plugins/s9wysiwyg/s9wysiwyg.inc and plugins/s9wysiwyg/s9wysiwyg.css? In fact, the whole s9wysiwyg folder. The .js is referenced in screen9_editor.module, which references the .png, but nothing else.

There is an insane amount of HTML being printed out hardcoded. Can you explain exactly why that is done, and the usual Drupal methodology isn't being used?

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• ./js/editor_plugin.js: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

  			// Add a node change handler, selects the button in the UI when a image is selected
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./screen9.module:412:    $medias = screen9_call('search', array($text, array('title'), $fields, $filters, 'posted', 5, $offset));  // sortby, count, start
  

• ./screen9_cck.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function theme_screen9_cck_formatter_downloadlink($element = NULL) {
  --
  
  function theme_screen9_cck_formatter_viewlink($element = NULL) {
  --
  
  function theme_screen9_cck_formatter_viewimage($element = NULL) {
  --
  
  function theme_screen9_cck_formatter_downloadimage($element = NULL) {
  ... many others ...
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./screen9_editor.module:8:function screen9_editor_wysiwyg_plugin($editor, $version=0) {
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  media_screen9.module:2:// $Id$
  screen9_cck.module:2:// $Id$
  screen9.install:2:// $Id$
  screen9.module:2:// $Id$
  

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

manual review:
remove all debug statements and commented out code lines.

Review of the 6.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/screen9.module:
   +14: [minor] There should be no trailing spaces
   +61: [minor] There should be no trailing spaces
   +65: [minor] There should be no trailing spaces
   +69: [minor] There should be no trailing spaces
   +132: [minor] Comment should be read "Implements hook_foo()."
   +184: [minor] Comment should be read "Implements hook_foo()."
   +217: [minor] Comment should be read "Implements hook_foo()."
   +318: [minor] Comment should be read "Implements hook_foo()."
   +398: [minor] There should be no trailing spaces
   +497: [minor] There should be no trailing spaces
   +498: [minor] There should be no trailing spaces
   +673: [minor] There should be no trailing spaces
   +676: [minor] There should be no trailing spaces
   +705: [minor] There should be no trailing spaces
   +774: [minor] Comment should be read "Implements hook_foo()."
   +801: [minor] There should be no trailing spaces
   +804: [minor] There should be no trailing spaces
   +818: [minor] There should be no trailing spaces
   +821: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 19 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.
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  js/editor_plugin.js:64:                //var rUrl = '/screen9/getv?vid='+mediaID+'&playerWidth='+playerWidth+'&playerHeight='+playerHeight+'&aspectRatio='+aspectRatio;
  screen9.module:99:    //drupal_set_message(t("The Screen9 api call '%function' failed.", array('%function' => $function)), 'error');
  screen9_cck.module:135:          //dsm($item['permaid']);
  screen9_cck.module:137:          //dsm($media);
  

• ./screen9.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      //drupal_set_message(t("The Screen9 api call '%function' failed.", array('%function' => $function)), 'error');
  

• ./js/editor_plugin.js: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

                  //var rUrl = '/screen9/getv?vid='+mediaID+'&playerWidth='+playerWidth+'&playerHeight='+playerHeight+'&aspectRatio='+aspectRatio;
  			// Add a node change handler, selects the button in the UI when a image is selected
  

• ./includes/providers/emvideo/screen9.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function emvideo_screen9_info() {
  function emvideo_screen9_extract($video_code = '') {
  function emvideo_screen9_embedded_link($video_code) {
  function emvideo_screen9_video($video_code, $width, $height, $field, $item, $node, $autoplay, $options = array()) {
  function emvideo_screen9_preview($video_code, $width, $height, $field, $item, $node, $autoplay, $options = array()) {
  function emvideo_screen9_thumbnail($field, $item, $formatter, $node, $width, $height) {
  function emvideo_screen9_content_generate() {
  function emvideo_screen9_data_version() {
  function emvideo_screen9_data($field, $item) {
  function emvideo_screen9_duration($item) {
  

• ./media_screen9.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function media_screen9_emfield_providers($module, $provider = NULL) {
  function media_screen9_form_alter(&$form, &$form_state, $form_id) {
  

• ./screen9.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
  

  672- *  The form data of the video.
  800- *  Start delimiter.
  

• ./screen9_cck.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
  

  85- *  The form data of the permaid.
  

• ./screen9_editor.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
  

  233- *  String containing the image data.
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  media_screen9.module:2:// $Id$
  

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

  ./screen9_editor.install ./screen9.install ./media_screen9.info ./templates/select_page_popup.tpl.php ./templates/edit_page_popup.tpl.php ./templates/upload_page.tpl.php ./templates/upload_page_popup.tpl.php ./templates/details_page_popup.tpl.php ./INSTALL.txt ./screen9_editor.module ./includes/providers/emvideo/screen9.inc ./screen9.info ./screen9_cck.info ./js/editor_plugin.js ./screen9_editor.info
  

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.

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-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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:

• editor_plugin.js: indentation errors, always use 2 spaces per level, same as in PHP files. Also in screen9.js.

@screen9 has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

You need to do some work on the coding standards issues, after you have taking care of that stuff, this looks RTBC for me.

Any updates ?
Setting needs work, as there's still something left to do..

You need to set the status to "needs review" if you want to get a review.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Warning: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered. Please create your own account to post on drupal.org.

didn't mean to change the status.

@screen9,

Did you rewrite your module?

I still get plenty of errors on : http://ventral.org/pareview/httpgitdrupalorgsandboxscreen91184484git
with several critical.

Is your module alive?

We run coder and it seems that all errors are removed. We cleaned master too. We checked that all our changes are visible. It is ready for review again please.

@screen9,

Code_review is not enough accurate. Your code must pass the following PAReview test. You may also install PAReview locally for convenience testing.

I still get plenty of errors on : http://ventral.org/pareview/httpgitdrupalorgsandboxscreen91184484git
with several critical errors.

For example in sites/all/modules/screen9/templates/upload_page_popup.tpl.php:
+22: [critical] Use the Form API to build forms to help prevent against CSRF attacks.

sites/all/modules/pareview_temp/./test_candidate/templates/upload_page.tpl.php:
+12: [critical] Use the Form API to build forms to help prevent against CSRF attacks.

sites/all/modules/pareview_temp/./test_candidate/templates/select_page_popup.tpl.php:
+28: [critical] Use the Form API to build forms to help prevent against CSRF attacks.

You shouldn't have PHP code or forms in your templates tpl.php files.

To build forms, use form_api thru hook_form, in your screen9.module file.

See hook_form API and forms_api_reference.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)