[D7] Scribble

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxs_leu2126163git

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.

Something besides pareview.sh issues.

• Your project page needs description.
• Remove master branch http://drupal.org/empty-git-master.
• Implement variable_del to hook_uninstall.
• Why do you use DIRECTORY_SEPARATOR???
• scribble.module - line 334 use abstraction layer instead of query.

@s_leu, good work!

But:

• Remove from scribble.js "console.log('BLABLA');" ;)
• It's recommended to move all theme functions with HTML to .theme.inc or .tpl.php. May be you can use theme_html_tag() in your theme_scribble_brush_options()?
• For directory creating there's Drupal function file_prepare_directory().

Hi s_leu,
This module contain drupal_mkdir($path, 0777); many times in the function scribble_prepare_folders($scribble_id) at line 451.
I think this is security issues to create directory with permission 0777 on the server any where, if i am right? Will it work with maximum 0775 permission?

Thanks,
Yash

Hi s_leu,
1- Your configure link is wrong in info file configure = admin/config/user-interface/scribble It should be admin/config/media/scribble

2- The images that are coming in slideshow are quite heavy a 500x500 is of size 758 kb it can be made of low size possibly also in jpg for more compression.

3-Background color of canvas not working in my side. Although i have given it in settings form.

The project page should list entity as a dependency (and maybe libraries api). And there is what seems to be a security related issue. The rest of my feedback is more in the optional or question category. This project is great example the Drupal API and code standards and very close to RTBC.

scribble-brush-options.tpl:

foreach ($variables['brushes'] as $brush_id => $label):
  if ($first) {
    $checked = 'checked="checked"';
    $first = FALSE;
  }
  else {
    $checked = '';
  }

The control statement should use the ":" alternative syntax instead of curly braces in template files. Alternatively use https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

scribble-slideshow.tpl:
In the same vein, rather than an almost empty scribble-slideshow.tpl, consider #prefix/#suffix render array functionality:
https://drupal.org/node/930760

function scribble_requirements($phase) {
  $requirements = array();
  // Ensure translations don't break during installation.
  $t = get_t();

  if (in_array($phase, array('install', 'runtime')) && !file_exists(libraries_get_path('jqscribble') . '/jquery.jqscribble.js')) {
  

Is this a hidden dependency on libraries module? I'd think you would get an error if libraries wasn't installed on a missing function definition of libraries_get_path.

function scribble_install() {
  // This module renders the field values in it's own theme function, fields do
  // not to be displayed.

Nit: comment typo. fields do not (need) to be displayed.

function scribble_uninstall() {
  field_delete_field('scribble_title');
  field_delete_field('scribble_image');
  field_delete_field('scribble_brushes');
  field_delete_field('scribble_image_snapshots');
  field_delete_field('scribble_background_color');
...

  // @todo
  // field_delete_instance();

Instead of field_delete_field, if you only call delete instances then the last field base will automatically get deleted. However if someone uses an existing field on another content type, then this approach will break them. Not strictly important here because the field would probably break regardless, but something to consider.

js:

        $draw_canvas.data("jqScribble").save(function (imageData) {
          if(confirm(Drupal.t('You\'re about to save ur changes. Is that cool with you?')) && !$draw_canvas.data('jqScribble').blank) {
           

Miss spelling in the confirm.

function scribble_page_title($scribble, $title_part_key = NULL) {
  $items = field_get_items('scribble', $scribble, 'scribble_title');
  $return = $items[0]['value'];
  $title_parts = scribble_get_menu_title_parts();
  if ($title_part_key && array_key_exists($title_part_key, $title_parts)) {
    $return .= ' | ' . $title_parts[$title_part_key];
  }
  return $return;
}

Missing any sanitization on the title. That's a bit of a security issue.

function scribble_prepare_folders
Might want to make the destination for scribles more flexible so they can be stored in a private filesystem. Or maybe you don't. I think having them stored in private could get complicated and is more of an enhancement.

function scribble_remove_image_form($form, &$form_state, $scribble, $fid) {
  ...
  $question = t('Are you sure to remove image !image from scribble !scribble?', array(
    '!image' => $file->filename,
    '!scribble' => $scribble->label,
  ));

Typo in confirm message. Are you sure you (want) to remove.

Nothing is a blocker. However some minor feedback below. Consider using drush and coder. You can even set it up with a git pre-commit hook using drush coder-review --git.

.module:
127 | WARNING | A comma should follow the last multiline array item.
952 | ERROR | Missing parameter type at position 1
954 | ERROR | Missing parameter type at position 2

Getting review bonus would help speed up the process and make sure it gets on the review admins radar.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

And please add the links to your reviews to the issue summary as requested on the review bonus page.

Security question:

1. In scribble_save_ajax() are there any security concerns about converting POST values to an image and eventually writing it to disk?

Non-blocking issues:

• If you use the Libraries API, you don't have to require the sites/all/libraries path in scribble_requirements()
• In your validate functions like scribble_settings_validate_jpeg_quality(), becareful using is_numeric, this will be TRUE for floats, octals, hex, &c. ctype_digit() may be better here.
• Also if you set the form element to '#required' => TRUE then you don't need the empty checks in the validate functions
• Do you need the empty ScribbleMetadataController class?

Assigning to klausi for the security question, otherwise this is ready to go from me!

As far as I can see scribble_save_ajax() is no a security risk, since it just saves uploaded data to an image file. That is basically the same as uploading a file over a file field widget.

I'm more concerned about the scribble ID which also directly comes from $_POST and is never validated. What characters are allowed in a scribble ID? That should be checked before using it in a filename. I'm not sure how that could be exploited exactly, but you can come up with some ideas like '../../../../somewhere' and try to build an attack from that. Can you validate the scribble ID before doing the file processing?

Thanks for those updates! Normally we ask that you don't RTBC your own issues, but the changes were small and this application has been around too long already. I'm not sure how to move credit from one git account to another, maybe this will help? http://stackoverflow.com/questions/3042437/change-commit-author-at-one-s....

Thanks for your contribution, s_leu!

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.