[D7] Duplicate Images

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.

Hi,

Please fix the basic issues with your codes are listed below (http://pareview.sh/pareview/httpsgitdrupalorgsandboxfietserwin2702627git):

EDIT: removed long pareview.sh dump

Using Coder I found the following issues (learn more about coder and coding practices at https://www.drupal.org/project/coder

EDIT: removed long Coder dump, see http://pareview.sh/pareview/httpgitdrupalorgsandboxfietserwin2702627git

Automated Review

Found more issues on pareview.sh. Follow link: http://pareview.sh/pareview/httpgitdrupalorgsandboxfietserwin2702627git

Manual Review

Individual user account

[Yes: Follows] 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 assets/code

[Yes: Follows] the guidelines for 3rd party assets/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

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[Suggestion:
1. Add filter_input for $_GET
2. Add t() function for links.
3. After proceed search duplicate steps am getting list of errors.
a.Warning: scandir() [function.scandir]: Unable to find the wrapper "private" - did you forget to enable it when you configured PHP? in DuplicateImagesSearch->search() (line 192 of sites\all\modules\duplicate_images\DuplicateImagesSearch.php).
b.Warning: scandir(private://,private://) [function.scandir]: The filename, directory name, or volume label syntax is incorrect. (code: 123) in DuplicateImagesSearch->search() (line 192 of \sites\all\modules\duplicate_images\DuplicateImagesSearch.php).
c.Warning: scandir(private://) [function.scandir]: failed to open dir: No such file or directory in DuplicateImagesSearch->search() (line 192 of \sites\all\modules\duplicate_images\DuplicateImagesSearch.php).
d.Warning: scandir() [function.scandir]: (errno 2): No such file or directory in DuplicateImagesSearch->search() (line 192 of \sites\all\modules\duplicate_images\DuplicateImagesSearch.php).
e.Warning: Invalid argument supplied for foreach() in DuplicateImagesSearch->search() (line 197 of sites\all\modules\duplicate_images\DuplicateImagesSearch.php).

4. Implement hook_help in your code.
5. Add image preview for Results updates section also to avoid accident deletions

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.

just to add in see Sanitization functions | D7 The most general way to protect any input ($_GET or $_SERVER etc..) var in best drupal etiquette should be at least passed through check_plain()

return isset($_GET['op']) ? check_plain($_GET['op']) : 'intro';

although, there should be a better way than using GET in general.

in term of l() use.

l(t('English name'), 'your/path/to-something')

it's the $title that needs t() -- yes, it's funny that drupal doesn't automatically do this..but, it doesn't....

you should have a configure=admin/config/media/duplicate-images on your .info -- as well as hook_help to outline what you module does (and doesn't, ie: monitor revision).. and where to go.

and currently the entire module is giving WSOD level errors.

• excluded_sub_folders in DuplicateImagesSearch->submit() (line 142 of sites/all/modules/2702627/DuplicateImagesSearch.php).
• Undefined index: use_md5 in DuplicateImagesSearch->submit() (line 148 of 5/sites/all/modules/2702627/DuplicateImagesSearch.php).
• TypeError: Argument 1 passed to DuplicateImagesSearch::exec() must be of the type array, null given, called in /sites/all/modules/2702627/DuplicateImagesSearch.php on line 151 in DuplicateImagesSearch->exec() (line 171 of /sites/all/modules/2702627/DuplicateImagesSearch.php).
• Warning: array_filter() expects parameter 1 to be array, null given in DuplicateImagesSearch->submit() (line 140 of /sites/all/modules/2702627/DuplicateImagesSearch.php).

with other errors as well.

please fix these before marking as 'needs review' again.

--head commit: April 16, 2016 8c4afec422ed1a41c38d244b1be3c319835da36e

now i see why you're using $_GET...

Please look into Multistep forms in drupal... it would be a better UX flow for your module than it's current direction

https://www.drupal.org/project/examples does have a basic sample... but there are several ways to do it..

maybe even --> hook_menu allows for 'tabs' which also maybe batter for what your doing. http://drupal.stackexchange.com/questions/37262/create-tabs-with-hook-me...

i've spend a bit more time going over the errors, it seems one side of issues comes from images that have already been deleted from the file system, but still exist on the managed_file table. You'll need to introduce some pre-flight checks to make sure those files do actually exist.

As you are using this Task-Lisk theme item as a step wizard it is awkward as the user can not back up on a step, nor reset to beginning.

In your step intro page i think you should be clearly recommending to users to use Backup_migrate to backup their file system before use -- as your module does effectually delete live content items (thinking of 'an average admin/content editor' -- they will never know 'which is the good file' this way).

re13

file function are failing due to the file not actually being there. When the routine is in the process of deleting a FID.. you need to pre-check to make sure the file is actually there (if not.. routine should then just break; out of that one delete step..and change to just a removal of the FID from the file table.

i might think part of the problem is drupal's File API itself... as there should be an Exception thrown (a good old lack of try{}catch{} kind of thing), so it's up to "use dev's" to work it through.

i've had this happen before myself. (: The FIle API has some annoying holes in it.

@arulraj -- hopefully your tracking still. Could you clarify your testing at all to verify any similarity to my #15 examples?

when error occurs user gets WSOD -- app breaks. no screen message for UI user. logged in watchdog/php console.

Certainly, this could be considered 'an issue'. meaning module could be released 'as is'.. But...it is a big enough issue that will occur for general Users [of course, It won't be my project issues messages to deal with]. Myself, I would release a module with this standing problem.

The UX notes (either multistep or tabs..) are pure suggestion, of course.

as such, i've filed your first issue: https://www.drupal.org/node/2717987

There are still issues when validating through pareview, check http://pareview.sh/pareview/httpgitdrupalorgsandboxfietserwin2702627git

And some other minor coding standards issues:

duplicate_images.info
Add spaces before and after "="-sign
Add package information to have the module shown in a correct group (ex. "media" or "image")

I do like the fact that you are working OOP. However I would recommend to already work out your module partially how Drupal 8 modules are structured. Just to save you some time in the migration process once the module is going to be ported to D8. This can simply be done by using the xautoload module. Then it's only a matter of moving your PHP classes to the src subfolder.

I can see there are many errors in PAReview : http://pareview.sh/pareview/httpgitdrupalorgsandboxfietserwin2702627git

Some of them listed below :

/duplicate_images.module: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function duplicate_help($path) { It should be function duplicate_images_help($path) {
Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesDelete.php
------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------
156 | ERROR | Missing parameter comment
157 | ERROR | Missing parameter comment
199 | ERROR | Missing parameter comment
215 | ERROR | Missing parameter comment
228 | ERROR | Missing parameter comment
------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesIntro.php
-----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------
28 | ERROR | [x] Expected one space after the comma, 0 found
35 | ERROR | [x] Whitespace found at end of line
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesSearch.php
------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 10 LINES
------------------------------------------------------------------------
15 | ERROR | Missing short description in doc comment
20 | ERROR | Missing short description in doc comment
161 | ERROR | Missing parameter comment
162 | ERROR | Missing parameter comment
163 | ERROR | Missing parameter comment
164 | ERROR | Missing parameter comment
188 | ERROR | Missing parameter comment
189 | ERROR | Missing parameter comment
190 | ERROR | Missing parameter comment
191 | ERROR | Missing parameter comment
------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesFinished.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
27 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesBaseForm.php
---------------------------------------------------------------------------
FOUND 23 ERRORS AFFECTING 20 LINES
---------------------------------------------------------------------------
16 | ERROR | [ ] Missing short description in doc comment
32 | ERROR | [ ] Return comment must be on the next line
39 | ERROR | [ ] Return comment must be on the next line
46 | ERROR | [ ] Missing parameter comment
47 | ERROR | [ ] Missing parameter comment
49 | ERROR | [ ] Return comment must be on the next line
86 | ERROR | [ ] Return comment must be on the next line
91 | ERROR | [ ] Inline doc block comments are not allowed; use "/*
| | Comment */" or "// Comment" instead
99 | ERROR | [ ] Missing parameter comment
100 | ERROR | [ ] Missing parameter comment
102 | ERROR | [ ] Inline doc block comments are not allowed; use "/*
| | Comment */" or "// Comment" instead
102 | ERROR | [x] The open comment tag must be the only content on the
| | line
102 | ERROR | [ ] Missing short description in doc comment
109 | ERROR | [ ] Missing parameter comment
110 | ERROR | [ ] Missing parameter comment
111 | ERROR | [ ] Missing parameter comment
112 | ERROR | [ ] Missing parameter comment
112 | ERROR | [ ] Missing parameter type
114 | ERROR | [ ] Return comment must be on the next line
156 | ERROR | [ ] Return comment must be on the next line
165 | ERROR | [ ] Return comment must be on the next line
183 | ERROR | [ ] Missing parameter comment
197 | ERROR | [ ] Missing parameter comment
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesUsages.php
---------------------------------------------------------------------------
FOUND 21 ERRORS AFFECTING 21 LINES
---------------------------------------------------------------------------
262 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line
271 | ERROR | Missing parameter comment
272 | ERROR | Missing parameter comment
273 | ERROR | Missing parameter comment
276 | ERROR | Missing parameter comment
296 | ERROR | Missing parameter comment
297 | ERROR | Missing parameter comment
348 | ERROR | Missing parameter comment
349 | ERROR | Missing parameter comment
361 | ERROR | Missing parameter comment
362 | ERROR | Missing parameter comment
363 | ERROR | Missing parameter comment
380 | ERROR | Doc comment short description must be on a single line,
| | further text should be a separate paragraph
396 | ERROR | Missing parameter comment
417 | ERROR | Inline doc block comments are not allowed; use "/* Comment
| | */" or "// Comment" instead
430 | ERROR | Inline doc block comments are not allowed; use "/* Comment
| | */" or "// Comment" instead
470 | ERROR | Missing parameter comment
484 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line
509 | ERROR | Missing parameter comment
610 | ERROR | Return comment must be on the next line
660 | ERROR | Missing parameter comment
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/duplicate_images.module
---------------------------------------------------------------------------
FOUND 14 ERRORS AND 2 WARNINGS AFFECTING 14 LINES
---------------------------------------------------------------------------
13 | ERROR | [x] Inline comments must end in full-stops, exclamation
| | marks, colons, or question marks
52 | ERROR | [ ] Missing parameter comment
53 | ERROR | [ ] Missing parameter comment
83 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
84 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 3
85 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 3
86 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 3
87 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
88 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
89 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 3
89 | ERROR | [x] There should be no white space before a closing "}"
89 | ERROR | [x] Closing brace must be on a line by itself
95 | ERROR | [ ] Missing parameter comment
97 | ERROR | [ ] Return comment must be on the next line
125 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: )
131 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: )
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 12 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/DuplicateImagesUpdate.php
------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------
137 | ERROR | Missing parameter comment
138 | ERROR | Missing parameter comment
139 | ERROR | Missing parameter comment
------------------------------------------------------------------------

Hi @fietserwin,

This is very nice and useful module.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes: Does not cause

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/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
Yes: Meets the security requirements.

Coding style & Drupal API usage
There are still some minor coding standard issues. Please fix.
Pareview status: http://pareview.sh/pareview/httpsgitdrupalorgsandboxfietserwin2702627git

This review uses the Project Application Review Template.

Thanks for your contribution, fietserwin! You are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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, especially my slowness in approving following review. We know it's broken and are trying to fix it.