[D7] Maximage 2

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 briganzia,

Upon Manual Review,
1 - Setting an static paths is invalid (Path can be either sites/all/modules/contrib/ or sites/all/modules/).

/**
 * Install.
 */
function maximage_install() {
  node_types_rebuild();
  $types = node_type_get_types();
  node_add_body_field($types['maximage']);
  maximage_add_custom_fields();

  db_insert('maximage')
  ->fields(array(
    'jspath' => 'sites/all/modules/maximage/js',
    'csspath' => 'sites/all/modules/maximage/css',
    'mid' => '1',
  ))

Fails with invalid directories. Use drupal_get_path(). When my site path is sites/all/module/contrib/, This static path may failed on validation in the maximage_configuration_setting_form.

2 - Configuration form code is written in the .module file but the Administer Configuration forms should be in the seperate .inc file for each of the forms(file name should form_name.admin.inc).

3 - Provide the proper parameters on maximage_configuration_setting_form($form_state) {. Should be hook_form($form, &$form_state) {

4 - Adding the 3rd party dependencies(jquery.maximage.js, jquery.cycle.all.js) in the module file is not an proper way and the data will be deleted when the module gets updated. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on module directly.

5 - Instead of checking the access to the menu by user role.

function maximage_configuration_access() {
  global $user;
  if (in_array('administrator', $user->roles)) {
    return TRUE;

set administer content permission to the menu because only admin role have access. Like

'access arguments' => array('administer content'),
'access callback' => 'user_access', // No need to mention this line because this user_access called implicitly
 

6 - Forming an HTML content in the module file is not allowed.

function maximage_get_html($arr_images, $htmlcustom, $htmlbody) {
  $text = "";
  $text .= "<div id=\'maximage\'>";

For generating the HTML content, use hook_theme() for creating the custom theme.

Thanks
Purushothaman C

Hi,

Yes! hook_theme is the right way of rendering the HTML content. Of course you can inject the output of your custom theme. For more detailed explanation see this link hook_theme()

Thanks
Purushothaman C

Hi Luca,
first of all thanks for your efforts to share your work with the Drupal community :-) I've had a closer look to your module, and found several issues - hope you won't hate me now... ;)

README file

1. I've followed your installation instructions, and ran into one trap: I downloaded and unzipped the whole library, like I am used to other libraries. I didn't understand at first reading, that only the JS file is important. Thus the file was in the wrong directory, and I had to look into your module code to see, what files you expect at what places. Maybe you should explain, that only this file is needed, and should be placed directly into the maximage directory.

2. a small typo: you say "Go to Add content type and create a new Maximage item.", but you mean "Go to Add content and create a new Maximage item." ("content", not "content type") => maybe adding the direct path to "node/add/maximage" would be a nice addition to this explanation.

Usage

1. It seems, that the usage of "*" doesn't work as expected. I tried both "diese*" to match a node with alias "diese-seite-wird-panelized", and "tes*" to match a Panel page with path "test", but the maximage never showed up!

2. Why do you only consider path aliases, and not system paths additionally? I would expect from your module, that e.g. "node/5" also works. Because this path will never change, while a node alias may change.

Code Review

Install file

1. You should overthink your data structure - having an own database table for a module configuration, that will always have this single entry, is really a heavy-weight overkill!! Instead you should use the Variable system of Drupal (variable_get() and variable_set())

2. This one is not so important: the naming of the functions _maximage_installed_fields() and _maximage_installed_instances() could be better chosen in my opinion. e.g. _maximage_custom_fields() and _maximage_custom_field_instances()

Module file

1. You have 3 calls to drupal_set_message() without using the t() function inside.

2. Additionally, instead of using drupal_set_message() in these 3 cases, a call to wachdog() would be more appropriate. So you can control, if the message is visible in the frontend, or just in the db log. With drupal_set_message(), the message is always displayed to the user/guest. As you are printing error messages, it would be bad to have them on a production site.

3. In maximage_get_path(), you are using a foreach loop, where in every loop the same variables get overwritten. I know, that there can always be one result from the DB query. But this kind of coding is in general not clean

4. maximage_get_path() gets always called three times in a row (in your preprocess function). Every function call triggers the same database query. This is also a bad pattern. You could use drupal_static() to reduce it to a single DB call, no matter how often you call the function within one page request

5. code redundancy: maximage_get_path() and maximage_configuration_setting_form() use the exactly same lines of code to query the database. You should put this code in an own function and call this from the two functions instead. Plus, with this you would also separate the DB query in an own function, which is also recommandable in terms of separating the data layer a little bit (like a "poor man's MVC approach")

6. not quite sure, but maybe you shoult think of putting the (non-inline) adding of CSS and JS into a hook_init() implementaion instead. I'm not sure, but I guess, that CSS and JS aggregation could be better handled with that. What do the others think about this??

7. naming convention in maximage_path(): when I see a variable named $node_wrapper, I normally expect an entity metadata wrapper, but this are just path (pattern) values.

8. the name of the maximage_path() function itself: I'm not happy about this either. hmmm maybe maximage_filter_path() or maximage_get_best_matching_path() would be more self-explanatory... what do you think?

9. again maximage_path(): you should not re-invent the wheel. There is a function drupal_match_path(), which is well tested to find if a path matches against a single or multiple patterns. This is also used e.g. by the block system. Using this would prevent issues like the asterisk not working 100%, like I stated above

10. I'm also not 100% satisfied with the way, how you find the best matching path. But as I don't have any better idea at the moment, I just want to mention it. Maybe it's a thought-provoking impulse ;)

11. I'm 99.999% sure, that you can drop line 89, where you call your theme function and add the "template" parameter. The template is defined in your hook_theme() function. Declaring it here wouldn't change anything about this. It will land in your $variables array instead

your tpl file

There is no need to build a PHP string and then do a single print. The purpose of template files is, that you can and should use plain HTML, and just include some PHP prints, loops, if, etc.
You won't need the escaping of quotes anymore, and the whole thing is far more readable. Take any existing Drupal tpl file as reference and compare (e.g. node.tpl.php)

configuration form

1. The call to isset($form) will always return TRUE - you populated a few lines above with serveral values. It's impossible, that $form isn't set here

2. bad english: there's two times "doesn't exists", correct: "doesn't exist"

3. if you would have used Drupal variables instead of your own database table, you could also take advantage of the system_settings_form() function here, in order to save the form input. You wouldn't need your own submit function at all! (only your own validation handler)

general improvements / cleaner configuration

1. You should better pre-configure the content type, that you create. It is absolutely unnecessary, that there are menu entries available in the node form. Also it should not be promoted to front page by default.

2. As a detail view of a maximage node doesn't make sense at all, you should think about disallow the access to its detail page completely, using hook_node_view(). If not, the least thing you should do, is adding a robots metatag with no-index command.

3. Is the "Access to Maximage" really necessary? On the other, the more flexible your module is, the better. BUT I would really strongly recommend that you enable this permission for all roles by default. I'm sure, many users of your module would search a long time, why the image isn't displayed for his visitors

4. A general thought for future projects, not meant to change it for this project: providing an own entity instead of using the node system would be a good idea, if you don't want to show your entities on its own detail page. Of course, this would also mean more programming work, because you'd have to provide your own overview page in the administration, as well as an own edit form, etc

5. I'm unsure, if the dependency to jQuery Dollar is really necesary. Using jQuery instead of $ wouldn't hurt your users, but you can save one additional installed module.

5.a. The usage of the custom jQuery field is not fool-proof. On the one hand you have full flexibility, when writing your own JS lines there. On the other hand, having separate settings with checkboxes, select lists, etc to cover all possible configuration options would be easier for unexperienced users. So there is pro and contra, and I respect your design decision here.

6. I'd personally not force the usage of Image Field Caption module. Imho File Entity is a more generic and better approach add fields to images. Maybe in further development stage of your module you could think of support both modules (check for existance of one of them during installation, and then add your field(s) appropriate to the chosen module). Again, this is just some opinion to think of in future projects, or in future development of this module

So, now I'm really done - I promise!

Of course, you're welcome! It may only happen, that I will have a quite slow reponse time for the rest of this week. But you will always get a response :)

Using Variable system I solved the problems in 3), 4). I've seen the variable_get() code, it seems there is any call to db, so I suppose there is not need to use drupal_static, is it right?

Absolutely right - all the variables are already loaded, you can call variable_get() as often as you like for the same variable, without having negative performance impact.

Regarding the rest: I didn't have time to go through your complete post - hope I'll find time tomorrow evening!

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.

Thanks for the contribution.

Here are my initial comments:

Can you please add drupal_get_form to hook_menu itself.

/**
 * Implements hook_menu().
 */
function maximage_menu() {
  $items = array();
  $items['admin/config/maximage'] = array(
    'title' => 'Maximage configuration',
    'description' => 'Maximage configuration',
    'file' => 'maximage_configuration.admin.inc',
    'page callback' => 'maximage_configuration_form',
    'access arguments' => array('administer content'),
  );
  return $items;
}

Can you please make JS path mandatory

$form['jspathmaximage'] = array(
    '#type' => 'textfield',
    '#title' => 'Path to Javascript maximage libraries',
    '#description' => t('Path to javascript maximage library. No / at the beginning of path!'),
    '#default_value' => $maximage_jspathmaximage,
  );
  $form['jspathcycle'] = array(
    '#type' => 'textfield',
    '#title' => 'Path to Javascript cycle libraries',
    '#description' => t('Path to javascript cycle library. No / at the beginning of path!'),
    '#default_value' => $maximage_jspathcycle,
  );

Also, this module is dependent on external library so hook_requirement should be written to check dependency.

Also, please use variable_del instead of deleting variables from variable table using db_query.

/**
 * Uninstall.
 */
function maximage_uninstall() {
  $maximage_type = 'maximage';
  $sql = 'SELECT nid FROM {node} n WHERE n.type = :type';
  $result = db_query($sql, array(':type' => $maximage_type));
  $nodeids = array();
  foreach ($result as $row) {
    $nodeids[] = $row->nid;
  }
  node_delete_multiple($nodeids);
  maximage_delete_custom_fields();
  node_type_delete($maximage_type);
  field_purge_batch(500);
  db_drop_table('maximage');

  /* cleaning variables */
  db_query("DELETE FROM {variable} WHERE name LIKE 'jspathmaximage' OR name LIKE 'jspathcycle' OR name LIKE 'csspath'");
  cache_clear_all('variables', 'cache');
}

Automated Review

Yes

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 / No: Does not follow] 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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. Not as per coding standard, please see the comments below:
   FILE: /var/www/drupal-7-pareview/pareview_temp/css/maximage.css
   ----------------------------------------------------------------------
   FOUND 2 ERRORS AFFECTING 2 LINES
   ----------------------------------------------------------------------
   18 | ERROR | [x] Blank lines are not allowed in class definitions
   24 | ERROR | [x] Blank lines are not allowed in class definitions
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------

   FILE: /var/www/drupal-7-pareview/pareview_temp/maximage.module
   -------------------------------------------------------------------------
   FOUND 3 ERRORS AFFECTING 3 LINES
   -------------------------------------------------------------------------
   137 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   138 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   139 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 2
   -------------------------------------------------------------------------
   PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   -------------------------------------------------------------------------

2. ...]

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.

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.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxbriganzia2225575git

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

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

Licensing means - Yes, The Module follows all licensing requirements.
and
Secure code: Yes: Meets the security requirements and No: List of security issues identified.

Now it looks fine and there are no Automated review pending issue.
Changing the status to RTBC..

Thanks for your contribution!

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.