DrawingField provides a field type to create HTML5 based drawing.

What this module Provides:

  • Rich drawing tools
  • Exporting drawings to PNG
  • Saving and loading JSON
  • Constant-size or infinite canvases

It achieves this using the literallycanvas API plugin from:
https://www.versioneye.com/javascript/literallycanvas:literallycanvas/0.4.0
literallycanvas API depends on react.js library, go to below URL to download:
http://cdnjs.cloudflare.com/ajax/libs/react/0.10.0/react-with-addons.js

Browser support: Firefox 3+, Safari1.4+, Chrome, IE10+, Opera 9.5+ support the HTML5 canvas tag.

Requirements:
Libraries API
Jquery Update

Installation:
• Download literallycanvas API extract contents to libraries/literallycanvas
• Download react.js library and rename as react.js, add to
libraries/reactjs.
• Install and enable Libraries API module
• Install and enable Jquery Update module and configure it with jquery1.7+ version.
• Install and enable DrawingField module

Usage:
• Create a content type, entity form with a drawingfield widget.
• Customize the canvas background color, width and height of the drawingfield instance to your needs
• Start creating drawings

Go to readme.txt to get more details regarding installation.

Similar Modules

* canvas_field

How is it different from other canvas base field type:
* Provides a field type to create canvas based drawing for web and touch devices and drawing can be edited once created.
* Drawing can be rotated left to right and top to bottom.
* Text or contents can be provided on the drawing.
* It has option like undo/redo/clear to perform on the drawing to get effective image.
* We can change pencil line size at various level from thinner to thick.
* Line/rectangle/Circle can be provided on the canvas with various background color.

Module Link:
https://www.drupal.org/sandbox/rashid_786/2340611

Link to git Repository:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rashid_786/2340611.git drawingfield
cd drawingfield

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxrashid7862340611git

Manual reviews of other modules:

Files: 

Comments

guardiola86’s picture

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

guardiola86’s picture

Status: Needs review » Needs work
gaurav.pahuja’s picture

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...

rashid_786’s picture

@guardiola86,

Thank you for your valuable reviews. I have fixed the following issues:
-Set default branch Done
-Prefix all your functions with your module name
function form_type_drawingfield_value($element, $edit, $form_state) {
Above function is a hook and module name is required in between in this hook, please go through http://api.drupalhelp.net/api/multiselect/multiselect.module/7, https://www.drupal.org/node/169815 to know more about the same.
-Check why opcache.so cannot be opened Done
-Fix some issues related with coding standards Done

Thanks,
Rashid

rashid_786’s picture

@gaurav,

Thanks for your valuable review. As suggested by you, i have implemented hook_requirements in .install file and mentioned details about required libraries.

Thanks,
Rashid

rashid_786’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Issue summary: View changes

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.

naveenvalecha’s picture

Status: Needs review » Needs work
joachim’s picture

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.

rashid_786’s picture

Issue summary: View changes
rashid_786’s picture

@naveen,

Thanks for your detailed review, as per your suggestion i have renamed inc templates as drawingfield.field.inc and drawingfield.system.inc and included only these file in module file.

Thanks,
Rashid

rashid_786’s picture

@joachim,

Thanks for your very detailed review of the module, I have done all the changes as suggested and pointed by you.

Thanks,
Rashid

rashid_786’s picture

Status: Needs work » Needs review
PA robot’s picture

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.

joachim’s picture

Status: Needs review » Needs work

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.

rashid_786’s picture

Issue summary: View changes
rashid_786’s picture

@jaochim,

Thanks for your valuable reviews, I have updated literallycanvas library with lastest version which have no dependency on Jquery.js. Hence i have removed the dependency of jquery_update module.

> * Implements hook_form_type_hook_value() has replaced with Implements form_type_hook_value, please go through http://api.drupalhelp.net/api/multiselect/multiselect.module/7, https://www.drupal.org/node/169815 to know more about the same.

updated below with appropriate comments.
> * Implements hook_after_build().
> * Implements hook_preprocess().

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

below are the steps to install and configure module:
* download literallycanvas library at https://www.versioneye.com/javascript/literallycanvas:literallycanvas/0.4.0 and react.js at http://cdnjs.cloudflare.com/ajax/libs/react/0.10.0/react-with-addons.js. Add these to
your sites/all/libraries directory as sites/all/libraries/literallycanvas and sites/all/libraries/literallycanvas/js/react.js respectively.

* Install and enable Libraries API module
* Install and enable DrawingField module
* Create a content type, entity form or custom form with a drawingfield
* Customize the canvas background color, width, height and directory path of the
drawingfield instance to your needs
* Start creating drawings

Thanks,
Rashid

rashid_786’s picture

Status: Needs work » Needs review
joachim’s picture

> 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.

rashid_786’s picture

react.js URL already mentioned in readme and hook_requirements(). It only provides script directly, will have to copy script and save as react.js and include into librarycanvas/js/ directory.

Kindly let me know what kind of issues you are facing because it's working fine for me.

Thanks,
Rashid

rashid_786’s picture

need review

joachim’s picture

> 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
rashid_786’s picture

I have put the check for react.js in hook_requirements. There is a page http://cdnjs.com/libraries/react where a lots of versions listed on the page but it would be difficult for user to download the file because it provides the direct scripts here also. I have updated readme file with more information to understand user easily.

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

above error may occur due to unavailability of react.js.

Please download fresh code of this module and then install.

Thanks,
Rashid

pingwin4eg’s picture

Issue summary: View changes
Issue tags: -PAReview: review bonus

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.

rashid_786’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
rashid_786’s picture

@pingwin4eg

Manual reviews of 3 other applications are done.

Thanks,
Rashid

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning to myself for my next full review.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Needs work
Issue tags: +PAReview: security

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.

rashid_786’s picture

Issue summary: View changes
rashid_786’s picture

Hi mpdonadio,

Thanks for detailed reviews and suggestions . Here are my points in italic.

Automated Review

>./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) {

> * form_type_hook_value is a hook not a function it can't be prefixed with module name as module and is being used in middle of hook name. Please go through http://api.drupalhelp.net/api/multiselect/multiselect.module/7, https://www.drupal.org/node/169815 to know more about the same.

Manual Review

Secure Code
* Check plain has been implemented for height,width and background color.

Coding style & Drupal API usage

(+) In drawingfield_requirements(), don't split the translated string across lines. This makes translation harder. Also elsewhere.
* Split lines have been reduced.
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.
* As suggested Check has been implemented only for runtime.

drawingfield_base64_to_image() needs a proper docblock with @param and @return
* @param and @return docblock is done.

(+) You are using two different libraries, so these should really be two different library definitions in two different places.
* As per suggestion, two different library definitions have been created 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.
* field options have been validated. Background color will hexa code and color name, height will have in px and width will have in px, would like go with rgb and percentage in next version release of the module.

(+) In drawingfield_after_build(), #attached is preferred over drupal_add_library and drupal_add_js and drupal_add_css.
* As suggested, #attached is implemented instead of 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.
* As suggested, dependency of jQuery update module has been made on the module.

(+) localStorage doesn't work in old browsers. You need to figure out how you want to handle this so the JS doesn't error.
* 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+]

(+) I got JS errors in a stock Drupal 7 install.
* Kindly test on the expected browser and make sure all required library is included in expected directory.

rashid_786’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

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.

rashid_786’s picture

Hi Naveen,

Browser support information mentioned already in project page and application issue summary with title "Browser Support".

Could please let me know what are the changes missing in the code?

Thanks,
Rashid

naveenvalecha’s picture

cool :)

klausi’s picture

Status: Needs review » Postponed (maintainer needs more info)

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’s picture

FileSize
26.89 KB
11.08 KB

Hi Klausi,

Thanks for your reviews, But Drawingfield module is completely different from canvas field except both use canvas html tag. Here are the comparison:

As you can see in attached images both have different tools and different libraries. Drawingfield come with large number of tools much more like paint in windows.
Features in Drawingfiled:
* Pencil
* Eraser
* Line/rectangle/Circle
* Text option on the image
* image rotation
* Undo/redo/clear
* Different mode of color e.g. Background/foreground/color with circle/rectangle
* We can change pencil line size at various level from thinner to thick.

Where as Canvas field come up with very limited feature that can not fulfill user expectations for drawing point of view and it is still in unstable mode. So according to me to drawingfield cant be integrated with canvas field.
So i request you to make it as need reviews and provide your valuable feedback on this module.

Thanks,
Rashid

naveenvalecha’s picture

@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.

joachim’s picture

> 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.

rashid_786’s picture

@naveen,
Thanks for suggestion, i will mention difference details on summary and project details page.

@joachim,
Both modules are using different libraries with different features and are conflicting with each other by merging them. In such scenario would be difficult to combine.

Thanks,
Rashid

rashid_786’s picture

Issue summary: View changes
rashid_786’s picture

Status: Postponed (maintainer needs more info) » Needs review

@Naveen,

Suggested changes on project summary and issue summary page are done.

Thanks,
Rashid

naveenvalecha’s picture

Thanks! @rashid_786
Fix the suggestions suggested by pareview :

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

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

    <br>
    function form_type_drawingfield_value($element, $edit, $form_state) {<br>
    
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

    <p></p>
    <p>FILE: ...ar/www/drupal-7-pareview/pareview_temp/includes/drawingfield.system.inc<br>
    --------------------------------------------------------------------------------<br>
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE<br>
    --------------------------------------------------------------------------------<br>
     43 | WARNING | Format should be "* Implements hook_foo().", "* Implements<br>
        |         | hook_foo_BAR_ID_bar() for xyz_bar().",, "* Implements<br>
        |         | hook_foo_BAR_ID_bar() for xyz-bar.html.twig.", or "* Implements<br>
        |         | hook_foo_BAR_ID_bar() for xyz-bar.tpl.php.".<br>
    --------------------------------------------------------------------------------</p>
    

    Time: 139ms; Memory: 7Mb<br>
    
  • 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.

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

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.

joachim’s picture

> * Implements form_type_hook_value().

That's not a hook.

rashid_786’s picture

FileSize
25.39 KB

Hi Naveen,
Thanks for your reviews,

Automated Review
many discussion has happened on this hook in above threads. Kindly go through below URL to know more about this hook and also attached image for reference.

https://www.drupal.org/node/169815
http://api.drupalhelp.net/api/multiselect/multiselect.module/7

Manual Review
can not use this function (module_load_include) in a global context since it requires Drupal to be fully bootstrapped, hence it is recommended to use require_once to include the global file.
Kindly go through https://api.drupal.org/api/drupal/includes!module.inc/function/module_load_include/7 to know more on the same.

Thanks,
Rashid

rashid_786’s picture

Hi jaochim

Kindly go through below URL to know more about this hook and would welcome any suggestion.

https://www.drupal.org/node/169815
http://api.drupalhelp.net/api/multiselect/multiselect.module/7

Thanks,
Rashid

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

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

mpdonadio’s picture

/**
 * 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.

klausi’s picture

Status: Reviewed & tested by the community » Fixed

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:

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’s picture

Hi klausi,

Thanks for your valuable reviews, i have fixed points 1,3,4,5 and take rest of the points as an enhancement.

Thanks,
Rashid

rashid_786’s picture

Thanks a lot to all the reviewers for contributing their valuable reviews to improve the code quality and making this module a success :).

Thanks,
Rashid

joachim’s picture

> @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.

rashid_786’s picture

Okay cool, however i have updated my doc comments for this callback.

Thanks,
Rashid

Status: Fixed » Closed (fixed)

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