Jump to:
| Project: | Drupal.org Project applications |
| Component: | module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | PAReview: review bonus, PAReview: security |
Issue Summary
Perfecto is a Drupal 7 module for themers. It allows you to upload the theme as png or jpg and overlay it on site for easy comparison between current CSS/HTML.
Short screencast: https://www.youtube.com/watch?v=-CkWR2oiY4A.
There are similar projects like http://pixelperfectplugin.com/ or http://drupal.org/project/pixelperfect . Latter does not work in D7.
Project Page: http://drupal.org/sandbox/hkirsman/1763900
Git: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/hkirsman/1763900.git perfecto
Drupal Version: 7.x
PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxhkirsman1763900git-7x-1x
Reviews of other projects
http://drupal.org/node/1772350#comment-6433548
http://drupal.org/node/1780980#comment-6462356
https://drupal.org/node/1706904#comment-6572114
https://drupal.org/node/1706904#comment-6617880
Old review that didn't made it to Pareview bonus.
http://drupal.org/node/1706904#comment-6625960
Latest reviews (16.04.2013)
http://drupal.org/node/1967638#comment-7306388
http://drupal.org/node/1967638#comment-7306454
http://drupal.org/node/1891548#comment-7306656
http://drupal.org/node/1946966#comment-7306860
Comments
#1
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
#2
I've updated the project page. It doesn't exactly follow "Tips for a great project page". For example there's no screenshots. I hope screencast is enough and even better.
I've helped out more in application queue since last comment from ankitchauhan.
Do I have to post the reviewed link in new post to get bonus? I saw somebody did that multiple times.
#3
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.
#4
Thank you DmitriyMakeev for review!
I will fix these erros as soon as possible. I'll comment and ask some questions first.
1. Will check it out.
2. It probably didn't work because it blinks only once on first pageview. You had "uncaught exception" error then.
I didn't want it to blink all the time but only when one first learns to use Perfecto.
3. I didn't see anything useful in behaviours over $().ready(). Perfecto should be loaded only once. I understand behaviours are called for every ajax call too - don't want that to happen.
In D7 every JS is put inside behaviours without exception?
4. I'll try that out!
5. Good idea! Top 0 and horizontally centered.
#5
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!
#6
Thank you DmitriyMakeev and developers_rtpl!
I've now fixed and updated all you've brought up:
* Works with jQuery Update
* Use behaviours
* Includes all images to DOM. It still works with ajax but should be better. I have to append the HTML after </body> so I can't include it with PHP.
* Initial position is center top. I've also set reset button to center the composition.
* Added screenshots, "How to use", "Similar projects" to project page.
ps. DmitriyMakeev, blinking only occurs after your first upload.
pps. Good to hear that I've chosen good name, developers_rtpl! ;)
#7
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.
#8
How do I get bonus for reviewing other projects? I did one here: http://drupal.org/node/1706904#comment-6572114
#9
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.
#10
Can I apply for review bonus if I haven't yet? I mean I've added review links to this issue, but not written all the links to issue summary of my post or added PAReview: review bonus tag.
#11
Applying for first review bonus.
#12
manual review:
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
#13
1. Done
2. Done
3. Done
4. Because it's a small snippet of HTML I'm getting with ajax call. Don't need the whole theme.
#14
#15
For some reason my tags where preset differently than klausi set it.
#16
Trying to put back the tags klausi set.
#17
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.
#18
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>.pngSome 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)#19
Thanks, logicdesign!
1.
I tried creating alert('hello');.png file but could not create it. How do you escape forward slash?
I managed to create this file:
touch \alert\(\'hello\'\)\;\<.png
I've added check_plain for any precaution.
2.
About the path problem. I wonder why it didn't work? I disabled clean urls on my site and everything works! And it should have worked if you changed the url to ?q=... That page should only give you the html for the panel not the whole drupal theme. If you get the latter, then yeah infinite loop will occur because js is called again and again. The problematic page is called in perfecto_composition_panel_page_callback(), line 144 in perfecto.module.
Any thoughts?
3.
I used the unmanaged file api, because maby somebody want's to upload files directly with ftp etc. So it's a feature of this module :)
#20
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 :-)
#21
Reopening the issue, reviewed 3 projects, adding PAReview: review bonus tag.
Also fixed the clean url problem cubeinspire had.
#22
#23
#24
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.
1 problem that isn't that big, but you might want to try and fix (if it's even possible).
I hope it helps.