[D7] Image styles mapping

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.

Hey, cool looking module!

• Your git clone command in your description should be git clone --branch 7.x-1.x http://git.drupal.org/sandbox/florenttorregrosa/2310347.git image_styles_mapping. Otherwise it will ask us for your password.
• The function image_styles_mapping_report could use a @see image_Styles_mapping_menu() for doxygen purposes.
• Line 262: '#empty' => t('No image atom have has been allowed in any atom reference fields yet. Or such fields are not used.'), // Or perhaps you meant "atoms"
• The break on line 480 is not necessary. No biggy though. In the drupal coding standards the example shows no break, but it doesn't mention if it is required or not.

Other than those very minor things, I couldn't find anything else. Looks great!

Hi,

Please find some review comments below

1) image_styles_mapping_fields_report contains too much of nested loop. I think to make code simpler and moduler it is better to split the it into multiple functions.

2) Same observation in image_styles_mapping_views_fields_report,image_styles_mapping_scald_image_report function also.

3) I feel like we can try using some drupal static variable caching to improve performance since field_info_instances is getting used in multiple places for a single request.

4) Same observations as point 3 for image_styles_mapping_get_image_styles function also.

5) A small code format issue , please add line break above return in image_styles_mapping_search_image_styles function

6) A minor observation if you want to update in image_styles_mapping_display_view_mode_link it is actually a switch for entity type not entity right?

7) Too much of nesting in image_styles_mapping_view_display_link i can u can improve it.

8) Configure all the links as contansts.

Regards,
Prajwal

Thanks for admin/config/people/accounts/display/ you can define them as constants.

Regards,
Prajwal

@Grimreaper, thankyou for your contribution.

I appreciate your module idea and your hard work.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None. Good Job.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template. It contains very few information about this module like in what case this module is require and how initially user will configure/use this, little bit enhancement would be so useful to understand this module usage. Moreover this module is useful with atom_reference, scald_image and views_ui, these points also missing in README file as it should be there.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (+) Extra if condition is there, that need to be remove. Simply return the $help form switch case, like following code.
   

   function image_styles_mapping_help($path, $arg) {
     switch ($path) {
       case "admin/help#image_styles_mapping":
         $help = '<p>' . t('This module allows you to have a report listing
             the image styles per image fields and per view modes on all
             entities.') . '</p>';
         $help .= '<p>' . t('The report can be accessed <a href="@url">here</a>.', array('@url' => '/' . IMAGE_STYLES_MAPPING_REPORT_PATH)) . '</p>';
         return $help;
     }
   }
   

2. image_styles_mapping_report: Redundant if condition exist in following code.

   Instead of following code:

   if (module_exists('views')) {
       // Get the views.
       $views = views_get_all_views();
     }
   
     $output = array();
   
     $output[] = image_styles_mapping_fields_report($image_styles, $image_fields, $instances_info);
   
     if (module_exists('views')) {
       $output[] = image_styles_mapping_views_fields_report($image_styles, $image_fields, $views);
     }
   

   it should be:

   $output = array();
   
     $output[] = image_styles_mapping_fields_report($image_styles, $image_fields, $instances_info);
   
     if (module_exists('views')) {
       // Get the views.
       $views = views_get_all_views();  
       $output[] = image_styles_mapping_views_fields_report($image_styles, $image_fields, $views);
     }
   

3. (+) In image_styles_mapping_menu(), It use "access site reports" - which implies the user can change other report related permissions also. A separate permission would be good, too.
4. image_styles_mapping_menu(): It is not compulsion to use constant, you can directly write admin/reports/image_styles rather using separate constant. define('IMAGE_STYLES_MAPPING_REPORT_PATH', 'admin/reports/image_styles'); is not required in this case.
5. You can also configure this admin/reports/image_styles in .info file using configure=admin/reports/image_styles to make it directly accessible from module page.
6. (+) Project page also contains very few information https://www.drupal.org/sandbox/florenttorregrosa/2310347. Enhanced this too in similar way as I described above for README.
7. However there is nested if-else, for loop in your module but inline comments looks good to me. Also your code looks very well organised. Good Work.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Overall your module looks good to me, nice job!

Thanks again for your hard work.

All contributors are encouraged to review as much project applications as they can, and there was no need to remove the bonus tag by yourself;)

adding the PAReview bonus tag as there are many reviews by Grimreaper

Grimreaper, this is a great idea for a module and it works great, but one thing about the presentation of it could use improvement (see my attached screenshot). The columns need to be the same widths, if possible. I know that some tables have different number of columns. but the report looks a sloppy to me with all different widths.

One possible solution:
Add some DIVs to the presentation with an included CSS file then add drupal_add_css() to line up the tables.

One more suggestion: Please add the reports link to your README.TXT file as well. I had to search for the URL and finally found it in your .info file, but non-programmers will not search there, so it needs to be more obvious.

Other than that, it is a great module and thank you for this contribution! I will be using your module on all my clients' websites.

Bruce

Thanks, GR. Great work!

Bruce

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Few minor issues that need to be fix.

FILE: /var/www/drupal-7-pareview/pareview_temp/image_styles_mapping.module
--------------------------------------------------------------------------------
FOUND 11 ERRORS AFFECTING 11 LINES
--------------------------------------------------------------------------------
188 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 2
189 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
190 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
191 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
192 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
193 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
194 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
196 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 6
197 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
198 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
202 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------------

Manual Review

1. (+) Wrong configure link (admin/reports/image_styles) in .info file, need to be fix.
2. image_style_mapping.module: At line 241 and 256, why you are using multiple t() within a single line text, please add your comment on this
3. (+) image_styles_mapping_field_is_in_fields(): this function return $check variable that is not declared initially due to that following warnings producing on admin/reports/image_styles_mapping/views_scald page.
   Notice: Undefined variable: check in image_styles_mapping_field_is_in_fields() (line 202 of C:\xampp\htdocs\drupal731\sites\all\modules\2310347\image_styles_mapping.module).
4. In Readme.txt, need to correct spelling recommanded.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

@Grimreaper, I appreciate your hard work and coding standard. From above issues are I am not seeing anything to prevent RTBC, but these needs to be fixed before stable release.

Even you have also started to develop this module for D8, that's really good.

Currently in development. It has not all the features of the D7 version. Should I open a new project application issue when the D8 version will be ready or should I use this one with a double tag [D7][D8] ?

I am not the right person to answer this question but as I understand there is no need to open a new project application issue for D8 module, once this D7 module get pass through this process you can do these enhancement at later stage.

Thanks again for your hard work!

Automated Review

Review of the 7.x-1.x branch (commit 191ebcf):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. Addressed, merge reqeust with other module made.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Explicitly tested some XSS vectors w/o problems.

Coding style & Drupal API usage

recommends[] is not part of the module .info file https://www.drupal.org/node/542202

(+) The URL in image_styles_mapping_help() should not include the leading slash.

(+) Avoid breaking translatable strings across lines.

(+) image_styles_mapping_get_image_styles() needs a proper docblock with a @return

(+) image_styles_mapping_get_image_fields() needs a proper docblock with a @return

More missing docblock info. Fix these when you can.

(+) Your hook_menu() implies the existence of module that may not be enabled. For example admin/reports/image_styles_mapping/views, will run the views support w/o checking to see if Views is enabled and will throw a Fatal. You need to feature detect these links.

image_styles_mapping_reports_overview(). I think this router item can simply be

      $items['admin/reports/image_styles_mapping'] = array(
        'title' => 'Image styles mapping reports',
        'description' => 'Image styles mapping reports.',
        'position' => 'left', // adjust add needed
        'weight' => -8, // adjust as needed
        'page callback' => 'system_admin_menu_block_page',
        'access arguments' => array('access image styles mapping reports'),
        'file' =>  'system.admin.inc',
        'file path' => drupal_get_path('module', 'system'),
      );
    

Also, in mene router callbacks, it is best to hust return a render array, rather than calling theme().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

The 7.x branch has the possibility for some pretty big fatals in them, but those are bugs and not blocking issues. I am not seeing any blockers. The links in the reports all look XSS safe (they are build via l()), and the other row elements are machine names, which are alpha+underscore (cf, field_ui_fields_list() for $items['admin/reports/fields'] in core).

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.

The direct call to theme() was in image_styles_mapping_reports_overview(), line 18.

Automated Review

Review of the 7.x-1.x branch (commit 11c4884):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Looks like all of my (+) issues were addressed, except that image_styles_mapping_help() still has a translatable
string broken across lines.

I like the change to local tasks.

Read `git diff 191ebcf..` as well as another visual pass. Not seeing any blocking issues, and no objecttion to RTBC for a few days.

Per an IRC conversation with @klausi, we agreed that we do not need to do a review of the 8.x branch, as the API is still in flux.

Thanks for your contribution, Grimreaper!

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.