[D7] Drawingfield

I get the following errors: http://pareview.sh/pareview/httpgitdrupalorgsandboxrashid7862340611git

Basically you need to:
-Set default branch
-Prefix all your functions with your module name
-Check why opcache.so cannot be opened
-Fix some issues related with coding standards

There seems to be dependency on literallycanvas API. Please add hook_requirements to install / module file so that site builders get notification if something is required for this module.

https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...

Hi @rashid_786
First Thanks for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.Please follow the best practices identified here. http://pareview.sh/pareview/httpgitdrupalorgsandboxrashid7862340611git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: The third party library is under the Open source(BSD) license .Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. there is a file field.inc inside the includes folder.Please rename it to drawingfield.field.inc so that the file name should be as per the module name.
2. there is a file system.inc inside the includes folder.Please rename it to drawingfield.system.inc Also would you please let me know can't we write these hooks inside the .module file.if yes then do so else would you please let me know why you have kept in the seperate file.Similar is for the field.inc
3. Use the Jquery update module for the jquery 1.8 and add its as dependency in your module info file.

4. foreach (module_list() as $module) {
     if (file_exists($file = dirname(__FILE__) . "/includes/{$module}.inc")) {
       require_once $file;
     }
   }

   Would you please let me know what is the need of requiring all modules inc files ? if you only want to include your module inc file then simply inlude it using include_once or require_once .

Thanks Again!

This review uses the Project Application Review Template.

This sounds very neat!

> description = Drawing Field allows you to collect HTML5 canvas based drawing in cck and in your custom modules via fapi

CCK doesn't exist in D7, also, it's best not to use a developer acronym like 'fapi' in UI text.

> * Install, update, and uninstall functions for the field_example module.

Not quite true ;)

> * Implements hook_field_requirements().

This is hook_requirements() surely!

 * Defines the database schema of the field, using the format used by the
 * Schema API.
 *
 *
 * All implementations of hook_field_schema() must be in the module's
 * .install file.
 *
 * @see http://drupal.org/node/146939
 * @see schemaapi
 * @see hook_field_schema()
 * @ingroup field_example

Hook implementations shouldn't repeat the docs from the hook definition.

    'data' => array('type' => 'text', 'not null' => FALSE),

This array in hook_schema() isn't laid out the same as the others.

> define('DRAWINGFIELD_IMAGE_PATH', variable_get('file_public_path', NULL) . '/drawing/');

There's a stray space at the start of this line. Also, do you really need to define a constant for this, when it's a value you only use once? And in fact, what about allowing the user to set the image path, same as the way core file and image fields do?

/**
 * Get list of modules and inlclude elements as required.
 */
foreach (module_list() as $module) {
  if (file_exists($file = dirname(__FILE__) . "/includes/{$module}.inc")) {
    require_once $file;
  }
}

This is a fairly expensive operation to do, given it's every single page load!! You only have two .inc files, so just load them explicitly by filename.

    case 'admin/help#drawingfield':
      return '<p>' . t("DrawingField allows you to create HTML5 canvas based

Wonky indentation.

> * Implementation of helper function which converts base64 data into image.

'Implementation' refers to hooks and callbacks. Is this one of those? If so, it needs to name the thing it's implementing. If it's just a helper function for this module, it needs param docs.

}
/**
 * Implements hook_field_formatter_view().

A number of functions are missing blank lines between them.

 <div class="clearfix"></div> 

What's this for? The clearfix class goes on the container of things you want to clearfix, not as a clearer after them.

> Literally Canvas v0.3.0 depends on jQuery (tested on 1.8.2) can be found at http://code.jquery.com/jquery-1.8.2.min.js and add it inside literallycanvas directory.

Don't do this! Add a dependency on http://drupal.org/project/jquery_update instead.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Your README needs to say which jQuery version to set in JQUpdate.

The widget doesn't work for me. I see a blue rectangle (which overflows and covers up things below). Also, it's breaking Admin Menu.

> * Implements hook_form_type_hook_value().

I can't find anything about this hook. Which module does it belong to?

> * Implements hook_after_build().
> * Implements hook_preprocess().

Not hooks, but a Form API callbacks. If in doubt, check the docs on api.d.org.

/**
 * Implements hook_elements().
 */
function drawingfield_element_info() {

Code and comment disagree.

  drupal_add_js("{$jquery_update}/jquery/1.8/jquery.min.js",
               array(
                 'scope' => 'header',
                 'group' => JS_THEME,
                 'every_page' => TRUE,
                 'weight' => 10,
               ));

You don't need to do this any more now you have JQuery Update working for you! This might be what's breaking Admin Menu.

> Implements theme function

'Implements' refers to hooks, not theme function.

> and react.js at http://cdnjs.cloudflare.com/ajax/libs/react/0.10.0/react-with-addons.js.

This is missing from the README and from hook_requirements(). It should also be a URL to a page rather than directly to the script.

Also, I still can't get this to work I'm afraid.

> react.js URL already mentioned in readme and hook_requirements

In hook_requirements I only see the check for literally_canvas. In the README you need a URL to a download page rather than to the script, if possible.

I get these JS errors which I presume prevent the widget from working:

TypeError: $ is undefined literallycanvas.min.js:1
ReferenceError: LC is not defined drawingfield.js:10

Removing bonus tag, you did not make 3 manual reviews of other applications. You only mentioned the automated review results and other obvious things. Please read the Review bonus program page carefully and do a manual line-by-line walkthrough other projects.

Assigning to myself for my next full review.

Automated Review

Review of the 7.x-1.x branch (commit ae82c3c):

• ./includes/drawingfield.system.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
  

  function form_type_drawingfield_value($element, $edit, $form_state) {
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

FILE: /home/matt/PAR/pareview_temp/drawingfield.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 20 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: /home/matt/PAR/pareview_temp/includes/drawingfield.system.inc
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 43 | WARNING | Format should be "* Implements hook_foo().", "* Implements
    |         | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements
    |         | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "* Implements
    |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".
--------------------------------------------------------------------------------

Time: 415ms; Memory: 7.25Mb

Manual Review

Individual user account

[Yes: Follows / No: Does not follow] the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) height, width, and possibly background(?) are going through directly to output without plaining. See https://www.drupal.org/node/28984 This is not currently exploitable because of the length limits but this should be handled properly. Test your input fields with escapable strings to see what happens.

Coding style & Drupal API usage

[List of indentified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
The README should outline the exact path to the two JS libraries once they have been installed. This will cut down on problems and issues.

(+) In drawingfield_requirements(), don't split the translated string across lines. This makes translation harder. Also elsewhere.

Did you run into an instance where you have to explicitly load the libraries module in drawingfield_requirements?

Personally, I would only do the library check at runtime and not hard fail during installation.

drawingfield_base64_to_image() needs a proper docblock with @param and @return

(+) You are using two different libraries, so these should really be two different library definitions in two different places.

(+) Your field options should be validated. Also, consider that people may want to use rgb() syntax for the background color or percentages for the height/width, and also big values for the canvas size.

(+) In drawingfield_after_build(), #attached is preferred over drupal_add_library and drupal_add_js and drupal_add_css.

Your JS needs to use the context and settings that get passed to the behavior.

(+) .on was added to jQuery 1.7. but Drupal 7 ships with 1.4.4. You need to figure out how to handle this. Look into jQuery update.

(+) localStorage doesn't work in old browsers. You need to figure out how you want to handle this so the JS doesn't error.

(+) I got JS errors in a stock Drupal 7 install.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

I have not seen the changes done by you in the code.
A little thing

* Canvas tag is a HTML5 which is being used to draw paint like drawing and older browser does not support most of HTML5 tags. Below are browsers version supporting canvas tag.
[IE10+, Firefox4.0+, Chrome latest, Opera9.5 and safari4.0+]

Please specify this on your project page as well as project application issue summary.

cool :)

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

https://www.drupal.org/project/canvas_field

This sounds like a feature that should live in the existing canvas_field project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the canvas_field issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

@rashid_786,
Would you also specify the same difference on the issue summary page as well as project page similarly like this project https://www.drupal.org/project/jquery_carousel
Also change the status after doing the same.

> Drawingfield come with large number of tools much more like paint in windows.

Those could be added to Canvass Field maybe?

> it is still in unstable mode.

That's not a reason not to combine the two projects.

Thanks! @rashid_786
Fix the suggestions suggested by pareview :

Manual Review :
drawingfield.module : Its better to use module_load_include to load inc files instead of require_once dirname(__FILE__) . "/includes/drawingfield.field.inc";
require_once dirname(__FILE__) . "/includes/drawingfield.system.inc";

Rest seems good to me.

> * Implements form_type_hook_value().

That's not a hook.

Thanks! That's fine then.
Setting to RTBC :)

/**
 * Implements form_type_hook_value().
 */
function form_type_drawingfield_value($element, $edit, $form_state) {

@rashid_786, @joachim is correct here in #43 (and the dude has over 3000 commits to contrib modules...).

That is not a hook. Hooks are called by module_invoke() or module_invoke_all().

That is the value callback for your element. It gets called by _form_builder_handle_input_element() to build #value for the form element itself, which then gets assigned to $form_state['values'].

See the docblocks in includes/form.inc for how to properly document this function.

Review of the 7.x-1.x branch (commit 5519f94):

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

  FILE: /home/klausi/pareview_temp/drawingfield.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   20 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. drawingfield.module: the require_once statements should not be indented, right?
2. "drupal_alter('drawingfield_theme', $items);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
3. "@unlink($values['image_path']);": do not mask errors with the "@" operator as that makes it hard to trace and debug errors. And you should use drupal_unlink() instead.
4. drawingfield_after_build(): why can't you use #attached for the JS settings, that should work as well?
5. drawingfield.field.inc: do not duplicate all the documentation from the field hooks. Developers can always lookup that on the hook definition in *.api.php files.
6. drawingfield_base64_to_image(): why do you hard-code "file_default_scheme() . '://drawingfield'" here? I think that should be configurable per field where the resulting picture will be stored, same as with file fields in drupal core.
7. drawingfield_base64_to_image(): maybe you want to use file_save_data() or file_unmanaged_save_data() instead of doing your own rand() and file_put_contents() logic?

But that are not application blockers, so ...

Thanks for your contribution, rashid_786!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

> @rashid_786, @joachim is correct here in #43 (and the dude has over 3000 commits to contrib modules...).

It does look like this is one I didn't know about! I've had a read of https://www.drupal.org/node/169815 and it's all rather mysterious. Filed an issue to improve the documentation of this: #2391605: document (and rename?) form_type_hook_value() callback.