[D7] Croptool

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.

Quick review:

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.

It seems like you´ve a misnamed branch/tag created in your repository, please have a look at the release naming conventions.

Please remove the LICENSE.txt file. Drupal.org will add the appropriate version automatically during packaging so your repository should not include it.

jquery.imgareaselect.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you´re not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

There is a git tag that has the same name as the branch 7.x-1.x. Make sure to remove this tag to avoid confusion.

Major status is only for applications with a status of needs review that have been awaiting response from a reviewer for 2+ weeks https://drupal.org/node/539608.

There are many errors in code http://pareview.sh/pareview/httpgitdrupalorgsandboxvishalshah92211213git.

Manual Review

croptool.field.admin.inc:11 - Custom function should be documented according to https://drupal.org/coding-standards/docs#drupal
croptool.field.admin.inc:12 - It is safer to make a copy of global variables ($base_url = $GLOBALS['base_url']) to avoid accidentally overwriting or changing the global value.
croptool.field.admin.inc:46,698,705,712,785,841 - Use placeholders for variables passed to db_query(). See https://drupal.org/node/310072
croptool.field.admin.inc:599 - Use $form_state['redirect'] instead of drupal_goto()
croptool.field.admin.inc: - Use placeholders for variables passed to db_query(). See https://drupal.org/node/310072
croptool.module:5 - @file doc. This is not the installation file.
croptool.module:115,116,117 - don't include files from hook_menu. These should be defined in your .info file. See https://drupal.org/node/542202#files

Is it possible to get an explanation of how this differs from other image crop/focus modules that are already in circulation?

Is this functionality that should be considered for addition to an existing module?

See: Comparison of image cropping and resizing modules.

As noted previously: Major priority is only for projects that have been waiting for 2 weeks or more for feedback.

Coder module is reporting few issue. You can fix those.
https://drupal.org/project/coder

Another note:-
you are using
variable_get('file_croptool_path')

it was defined in install file :
variable_set('file_croptool_path', "sites/default/files");

what if there are multiple sites under sites directory.? I think you should use file directory path here.
may be:-

$files_dir = file_stream_wrapper_get_instance_by_uri('public://')->getDirectoryPath();

or similar.

Thanks
-Inder Singh
http://indersingh.com

Guessing last update was supposed to go from "needs work" to "needs review" to get people looking at it again.

It is a nice module.
But not clear with the action on when we click "Crop & Save" button.
Attaching screenshot for the same.

My review:

Concept

First of all many thanks for your contribution. I’m not sure if this is the place to debate the concept and the purpose of your module.Truth is that I do have some doubts about its purpose and how it’s different from modules such as manualcrop. Maybe you could elaborate in the project description. I'm sorry to say, and this is just my humble opinion, but the reasons you mention in #14 don't really point out any advantage over modules like manual crop, with which you can also have multiple crops for one image.

You could argue that if you want to display the same image but with different crops, you should still only have only one image field. You can use image styles to create different versions of that image, and you can use hook_field_extra_fields, or Display Suite, to show the same image several times in the node display using different image styles. Cropping it seems to me is image style stuff. But maybe I’m missing something essential.

Issues

• You say libraries is required, yet in your hook_requirements you have an if statement checking for libraries, making it look like it would work without it too. If I enable the module with drush, it should automatically enable libraries as a dependency. Before it can do that, the hook_requierements hook throws a REQUIREMENT_ERROR. So I can’t enable the module through drush without enabling libraries first.
  I think you should allow the module to be enabled , even though the img select plugin isn’t present. Throw a warning on the status page.
  Enabling the module through the ui does work properly.
• error: When enabling the module and generating the first crop, the module throws an error:
  Warning: imagepng(/Applications/MAMP/htdocs/insha/sites/default/files/cropimages/drupal_0_200_200.png): failed to open stream: No such file or directory in croptool_generate_images() (line 419 of /Applications/MAMP/htdocs/insha/sites/all/modules/croptool/croptool.module).
  This is because, like it says, the folder is relative to the root of the drupal installation. By default this folder is not writable. It should point by default to the sites/default/files/[folder-name] folder.
• The use of some words are somewhat confusing: what does TAB mean. If it is like a drupal primary tab, UI element on top of a page, i can’t find where it should be.
  field_p and field_c. What does it stand for. _p = path and _c for a json string of settings.
• You allow attaching the crop field to nodes and taxonomies. Why not allow all entities using hook_entity_presave?
• You can set the order of the crops, using your own jquery plugin. Why not use the native drupal table Drag and drop?
  https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...
• The admin pages have a lot of custom ajax callbacks, and don't process the data through regular _submit callbacks. It would be easier and more readable to use Drupals native AJAX api, but probably it makes even more sense not to use ajax at all, but just normal _submit callbacks for processing the form.
• code: Your javascript file doesn’t have any comment blocks to describe the different functions.
• code: The javascript isn’t wrapped in javascript closures, which is best practice.
  see https://drupal.org/node/171213
• code : Use of arg() is not recommended in croptool.field.admin.inc:22 Would be better to use an argument in hook_menu: admin/config/content/croptool/%
• code: Avoid inline html and inline CSS. croptool.field-admin.inc:99
• code: debug code on croptool.field-admin.inc:136
• code: Comment @file in croptool.install doesn’t make sense. Seems to come from some documentation of the hook_schema.
• code: In croptool_menu you have croptool_settings as a page callback. This function calls a form. Better would be to put drupal_get_form directly in hook_menu and use ‘croptool_settings_form’ as a page argument.
• The amount of DB queries in croptool.field_admin.inc makes it hard to judge security issues and found out how it actually works. I imagine it is a bit too much to ask to change this for the purpose of this module, but next time i would recommend to make the crop_field_details table into an entity with the entity API, so you can use proper CRUD functionality. That would be easier to read and safer.

Conclusion:

Having a quick glance at the code, I feel like you need a lot of code for what you're trying to achieve, and are not leveraging the power of Drupal. With all the DB queries, ajax callbacks and such, it's hard to assess the security of your module. I think it would be a good idea to look into the drupal examples modules and see how they do stuff like ajax and normal administration pages, see if you can shorten your code and make it more readable. But true enough, apart from the one bug of the image generation folder and enabling by drush, I haven't found any other bugs, so it is working!

First, very good review by nuez - there are a lot of work to do here, just to follow up on that review.

In addition:

There is already exists a lot of modules for cropping images for Drupal, and this one, to be honest, does not seem to be very well executed. It looks bloated, and does not leverage on Drupal.

Module duplication and fragmentation is a huge problem on Drupal.org and we prefer collaboration over competition. Unless you can argue your module is unique and significantly better than existing modules, and that cannot be fitted into (for instance) Manual Crop, it may be best suited as a sandbox.

To see if there is a way to get your work resolved within the framework of one of the already existing cropping tools, please open an issue in the most relevant issue queue for one of these tools 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".

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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