Block Title Tooltip

Hi mayank-kamothi,

Points i found after manual review :

Your info file is incomplete. You haven't provided required data there. You need to mentioned drupal version in your info file like -

description = Some Description
core = 7.x

I am getting error on module page, See attached screenshot for the same.

Hi

Automatic Review

Ran the code through ventral.org and got the following errors: http://ventral.org/pareview/httpgitdrupalorgsandboxmayank-kamothi1913870git

Manual Review

In the "hook_uninstall" would it be better to build an array of all the variables you have used and then loop over them using variable_del()?
Not sure which is quicker to be honest.

I would normally put my css files in a folder labelled "css" but that's just how I do it.

Spotted a few issues that are also covered in the ventral.org errors:

• Inline comments
• Files required to end in new line character

Apart from these few issues, all looks fine to me.

Hallo mayank-kamothi!

Nice module!

There is still a master branch in your git repository. You must create a 7.x-1.x branch and delete the master branch.

On this page you can found help in this topic http://drupal.org/node/1127732

I don't understand why you use the init hook to add css files. Why did you use the function_exists function?

/**
 * Implements hook_init().
 */
function block_title_tooltip_init() {
  if (function_exists('drupal_add_css')) {
    drupal_add_css(drupal_get_path('module', 'block_title_tooltip') . '/css/block_title_tooltip.css', 'module', 'all', TRUE);
  }
}

I would use the '#attached' array key: Theres a nice tutorial: http://shomeya.com/articles/getting-used-to-attached

function block_title_tooltip_block_view_alter(&$data, $block) {

  if (is_array($data['content'])) {
    $data['content']['#attached']['css'] = array(
    array(
      'data' => drupal_get_path('module', 'block_title_tooltip') . '/css/block_title_tooltip.css',
      'options' => array(
        'group' => JS_LIBRARY,
        'preprocess' => TRUE,
        'every_page' => TRUE, 
        )
      )
    );
  }
}

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

Add more description.

I agree with jbloomfield about the query in your uninstall function. It should work, but why not take advantage of Drupal 7's database APIs like delete queries to keep everything generic? Better yet, instead of cluttering up the variables table with an unknown quantity of data, set up a schema and the schema API will uninstall your table automatically.

Either way, by using the higher-level APIs, you're able to take advantage of any improvements the core team might make to the database interface, like supporting non-SQL data stores at some point.

This module was working as expected most of the time, but not for example if I added the block to the Bartik 7.20 header. I don't know if that is an issue with Bartik 7.20, but the block title wasn't visible.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

Remove tag

Pretty nice design. Here's my manual review:

- in block_title_tooltip.install don't delete variables with db_query, but rather with variable_del. This way it's still possible to delete with wildcard, but imagine that somebody will create the module block_title_tooltip_extend with it's own variables. Your way, you'd delete that module's variable's too.
- in block_title_tooltip.module on line 50 there's no hook called hook_submit

Otherwise seems pretty clean to me.

Manual review:

block_title_tooltip.install:
- The variable thing what AolPlugin described. This calls cache_clear_all automaticlly.

block_title_tooltip.module:
- Line 31: Missing space in the description (Title[ ]Tooltip)
- Maybe Line 50: Change to something like "Form submission handler for altered block_admin_configure() and block_add_block_form()."
- Line 76: Either add a return TRUE at the end or remove the FALSE. But having sometimes a return value and sometimes not is bad.
- Line 93: Same as above.
- Line 103: Unused global $user.
- Line 109: IMPORTANT! Use php_eval only when the format of the block description allows this or if user has permission 'use PHP for settings'. Otherwise user who don't have normaly access to it, can use php_eval!
- Line 114-119: Unused variable $attributes.

block_title_tooltip.css:
- I'm not a fan of 3 character long colors (#ccc), but I think this is valid, so only wanted to note this ;)

Maybe add additional information at the Drupal Project Page, there is also a typo there (filed).

removed automated reviews.

PAReview.sh returns:

FILE: /var/www/drupal-7-pareview/pareview_temp/block_title_tooltip.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
136 | WARNING | Only string literals should be passed to t() where possible
136 | WARNING | Only string literals should be passed to t() where possible

That said, these warnings about passing stuff that isn't a string literal to the t function seem trivial to me - module works as expected, and there are no syntax errors.

Hello,

1) Your module is vulnerable to XSS attacks. Pasting "

" into the tooltip field causes an alert to pop-up.

I believe this is due to you using t() instead of check_plain() on line 136. t() is used to translate interface strings, whereas check_plain() sanitizes your output for safe viewing.

Read more:
http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/check_p...
http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

2) Your block_title_tooltip table needs work. There is no primary key assigned to a column, meaning that your module's data will not be in normal form and multiple entries could exist for the same block. It looks like it should operate off of the block table's bid column, which is the primary and unique identifier for blocks.

This practice is called database normalization, and you can read more about it here:
https://en.wikipedia.org/wiki/Database_normalization

Typically having data in 3NF is good enough for most applications.

Hi Mayank,

My manual review:


You have a mistake in the documentation, it must be hook_block_view_alter()

/**
 * Implements hook_init().
 */
function block_title_tooltip_block_view_alter(&$data, $block) {
  // ...
}

and (my point of view) this one also, it must be hook_preprocess_HOOK()

/**
 * Implements hook_preprocess_block().
 */
function block_title_tooltip_preprocess_block(&$vars, $hook) {
  // ...
}

In block_title_tooltip_preprocess_block()

if (module_exists('php')) {
  $vars['block']->title_tooltip = $vars['block']->title_tooltip;
}

Perhaps you want to use php_eval()?

if (module_exists('php')) {
  $vars['block']->title_tooltip = php_eval($vars['block']->title_tooltip);
}

In that case, you should consider about the security risks, and also check for user permission to input PHP code or not (I think so).

Also in that function

• $block->title is already checked for plain text in block.module, function _block_render_blocks() line 879, you don't have to call check_plain($vars['block']->subject) again
• You defined $attributes without using it.
• You must call theme('image', $arbitrary_array) instead of theme_image($arbitrary_array)

One small thing, just my point of view about the coding style. In block_title_tooltip_preprocess_block(), you should assign $block = &$vars['block'], then use $block any places you want, instead of calling $vars['block'] everytime. It's faster and also easier to maintain.


Your module conflicts (on ui) with contextual links (see attached image)


Consider about the compatibility with other contrib modules, e.g Block Title Link. And I think you should use hook_theme_registry_alter to move your preprocess function to the end of the function list. That will make your function run last, no body can alter your subject and you won't miss any changes on the subject.


Thank you for your contribution and good luck with your application.

Hi,

For this purpose, there are some issues on drupal.org. In general, it is this:

Always use the theme() function to generate theme output. It takes care of routing the request to the appropriate theme function.

For more information on how this works, see:
http://api.drupal.org/api/function/theme/7.
http://drupal.org/node/1004556

The Correct Way to Call a Theme Function.
=> print theme('image', array('path' => 'path/to/image.png', 'alt' => 'Image description'));

The Wrong Way to Call a Theme Function
=> print theme_image(array('path' => 'path/to/image.png', 'alt' => 'Image description'));

In block_title_tooltip.module (row 136)
call: theme('image', $arbitrary_array) instead of theme_image($arbitrary_array)

Automated review:
PAreview.sh on ventral.org: http://git.drupal.org/sandbox/mayank-kamothi/1913870.git ... 0 problems found.

Manual review:
- Install a fresh drupal 7.22
- Install module
- go to the block admin configuration page
- add tooltip in the "Block Title Tooltip Settings"
- save the block.
- result as expected: get a tooltip when you hover on block title

Otherwise, it looks good

Hi Mayank

medienverbinder already answered your question about the theme image very clear so I don't have anything more to say.
About the thing that I called 'conflicts with contextual links', don't you think that the tooltip should stay above the gear icon of the contextual links?

:-)

It looks like a nice module, but I've found some small issues / improvements:

• If the block title is set to hidden (<none>) the tooltip is still rendered. It might not be a logical combination, but it might give unexpected results to render the tooltip and not the title.
• It might be nit picking, but it would be nice to use Drupal's default 'element-hidden' class to hide the tooltip. See http://drupal.org/update/themes/6/7#element-hidden

Also it might be nice to (optionally) allow formatted text input, to have more control over the content of the tooltip.

Manual Review:

At line 121: This code block do nothing.

if (module_exists('php')) {
        $vars['block']->title_tooltip = $vars['block']->title_tooltip;
      }

Others seems in order.

Module code looks fine. Your README.txt is better than the project description, you may just want to copy your README and use that.

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

manual review:

1. "Code changes" is not a useful commit message, please use https://drupal.org/node/52287 in the future.
2. block_title_tooltip_save_data(): you could use db_merge() instead of deleting and inserting?

But that are not blockers, so ...

Thanks for your contribution, mayank-kamothi!

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.