[D7] Perfecto

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Change git url to full git clone command

Useful module, but I found some bugs.

1. Module is incompatible with "Jquery Update". Firebug give that:
uncaught exception: Syntax error, unrecognized expression: [value=01.jpg]
But without Jquery Update it works.

2. I can't see blinking line in top right corner. The panel shown when I slowly move mouse in that corner.

3. Don't use jQuery(document).ready(). You need to change JavaScript behaviors. Use

(function ($) {
  Drupal.behaviors.initPerfecto = {
    attach: function (context) {
      ...
    }
  }
})(jQuery);

And some of my wishes for your module:
- Try to add theme('perfecto_composition') to the page without ajax, like "Admin Menu". Becouse when I refresh the page, I have to wait until the panel will load the images.
- Set initial position of the image to the center of the screen.

Hello ,
First of all I would like to mention that your module name is very interesting.I really like the name.
You can add detailed description of your module on its project page so that it can be easy for others to understand the flow of the module.
You can add how it differs from other similar projects(you mentioned above) in detail.Right now you have just mentioned the version compatibility difference between your module and the other similar module.Check http://drupal.org/node/894256 for more details.
It would be beneficial for you if you will consider these points at the initial stage of your project.

Thanks!

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1804580
Project 2: http://drupal.org/node/1773848

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

Please follow #1410826: [META] Review bonus. Don't forget to add all your reviews to the issue summary and to add the "PAReview: review bonus" tag.

manual review:

1. perfecto_permission(): use "restrict access" => TRUE to mark a permission as dangerous.
2. perfecto_unmanaged_delete_page_callback(): this vulnerable to CSRF, the delete link is not protected by a token or a confirmation form. Please read http://drupal.org/node/178896 again.
3. Same for perfecto_disable_module_page_callback().
4. perfecto_composition_panel_page_callback(): why print() and not return?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please don't remove the security, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. Please add your new reviews to the issue summary.

Added more reviews to get first review bonus

Nice work, I like the design of this module !

There is an additional security issue here.

The file name is saved and displayed as it is on the admin page, that allows to insert files names like: <script>alert('hello');</script>.png
Some OS doesn't allow to create filenames with the / symbol but other yes. This can at least break the page and at worst be used as an XSS attack.

You can use check_plain at line 304 to solve that.
'filename' => check_plain($file->filename),

This line of javascript is giving me problems because I don't have clear url's enabled. The ajax call leads to a 404 response.
var compositionControlPanel = Drupal.settings.basePath + 'admin/settings/perfecto/composition_control_panel';

When I set this to the compatible value : var compositionControlPanel = Drupal.settings.basePath + '?q=admin/settings/perfecto/composition_control_panel';

There is an infinite loop of calls because the result of the call is inserted after the body ( with the same ajax call that inserts agains after the body etc etc ).

That's not an issue but more an idea maybe for other projects, why not to use variable_get() and variable_set() to store the fid of the managed_files so its easier to delete them later ?

You can create a reference of an managed file on the database by using:
file_usage_add(stdClass $file, $module, $type, $id, $count = 1)

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 :-)

Added new review

Hi Hkirsman,

Very nice work first off. I looked at the code and couldn't find any flaws.

I checked the workings using a super admin account and a normal admin account running firefox 20 en and the lastest Chrome version 26.0.1410.64. Everything seems to be working accordingly!

I would still have 2 notes that you might be able to implement before the release of your module.

• A way to minimize the toolbar to adjust the image, it's blocking part of my view and I would like to hide it on command so I can see the entire screen
• Set a maximum size on your dropdown box(id: perfecto-imagecompositioncontrols-files) because the dropdown falls offscreen because I use images that have longnames.

1 problem that isn't that big, but you might want to try and fix (if it's even possible).

• When I use the webbrowsers zoom (ctrl+ & ctrl-) the image gets displaced so i have to readjust it again

I hope it helps.

You have a typo in the @file block in perfecto.module: "weg".

You use the string 'public://mod_perfecto' in many places, that should probably be a define or variable instead.

In perfecto_check_multiple_access_controls_access_callback() you can get a slight performance gain by just returning TRUE as soon as a valid permission is found. Currently the code will check all the permissions passed in.

In perfecto-admin.css you should have a blank line between the definitions.

What is /*[fmt]1A20-1A0B-E*/ at the top of perfecto-control-panel.css?

Those issues all seem minor though, and I did not find any security issues.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Looks good to me!

Thanks for your contribution, hkirsman!

I updated your account to let you 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 get 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.

This queue is used for new contributors, there is a peer review group for this purpose https://groups.drupal.org/peer-review (although it might be hard to get a response there I suppose).