Media Nivo Slider

Please check your Media Gallery Link, it is pointing to a page not found, and there is a number of media galleries.

I use views, and not media gallery. The Nivo Media Slider for views is (like most D7 Modules of this type) rather unstable. Hopefully we will get a more solid version soon! There are allot of sliders, even a doc page on them all, but most never got ported and the chances of porting is slight.

I am browsing your code and I see your not using PDO.

Its pretty clear you know what your doing in PHP, your not using all of the Drupal API functions, but your using more then most "offical" 7.x modules I have seen. You have been patient and I would hate to see your enthusiasm diminished.

You are using watchdog and print_r in it, I cant say for certain, but I don't think you should be inserting $node variables into query directly as they MAY contain SQL injects. I think we need someone more experienced to review that aspect of it.

/engage super-pedantic-mode

git repository full of executable files
Changing file permission masks on Windows

couple of the files missing EOL

db_query Could be converted to using DB:TNG. Use of parameter in first query in .install file seems unnecessary? Parameter is a static string.

non-expanding strings surrounded by "double quotes" Use single quotes if the string doesn't need $variable expansion

.info file the listed files[] are supurfluous as neither file includes a class or interface

image_get_info($media->uri) - result be checked to ensure it's not FALSE or you're about to get a couple annoying of error messages about indexes being invalid

What happens on a multilingual field? Is it possible? Does it matter? Comment says you do it for simplicity but doesn't justify why it can do that.

indenting gone whacky in _media_nivo_slider_block_cleanup

The XSS potential I was looking at evaporated when the unfiltered input was passed to theme('image') or theme('image_style') instead of being used directly.

Fix all that up and prepare the git repository as you would for a release and it's pretty much ready to go.

When your able to publish, please also update http://drupal.org/node/418616

Thanks:)

git commits look great - in nice logical commits.

At the top of media_nivo_slider.module, you have this
require_once(dirname(__FILE__) . '/media_nivo_slider.fields.inc');

This should use the API function module_load_include instead. Also, it should probably be included only when necessary? Surely it's only needed occasionally? Then again, looking at it, potentially not. Does fields core module pull in the fields.inc files when needed? Or is that only views.inc for the views module.

This seems a little hard coded - there is a Libraries API module available to take care of this for you. Use the 7.x-1.x version and the same instructions for D6.

  // The default path to the nivo slider directory.
  define('MEDIA_NIVO_SLIDER_PATH', 'sites/all/libraries/nivo-slider');

  drupal_add_css(MEDIA_NIVO_SLIDER_PATH . '/nivo-slider.css');
  drupal_add_css(MEDIA_NIVO_SLIDER_PATH . '/themes/default/default.css');
  drupal_add_js(MEDIA_NIVO_SLIDER_PATH . '/jquery.nivo.slider.pack.js');
  drupal_add_js(drupal_get_path('module', 'media_nivo_slider') . '/js/media_nivo_slider.js');

I'll leave the req for a release branch for when you're ready to release, but you should understand the implications of the master branch when it comes to managing releases in a full project - Release naming conventions

Setting back to 'normal' since I should have done it on the last reply.

Just make sure you have the libraries module as a requirement in your .info

If you do/did that then I say lets promote you:)

This contributor will be an asset to the community, we are crazy not to allow him to promote his module as he has done more then most.

The media module was ironically the module I looked at to see how it used module_load_includes ; I was a little disappointed to find they had the same thing.

The actual function module_load_include can only be used once drupal_get_path is working ... in terms of load order, I really would have expected most of Drupal to be completely bootstrapped by the time it starts loading sliders from fields loaded off nodes ;) Then I had to ponder if there was really a need to split it out into fields.inc.

BUT I have been told that if some big module like media is doing the same thing, then it can't be too detrimental. Of course, it may just be hanging onto its D6 roots too :P

And yes, this module and author are ready to move on.

This looks like a great module and I'm glad to see from the discussion so far that there has been a lot of great collaboration to make the module better.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. Please see the documentation about release naming conventions and creating a branch in git.

Thanks for identifying the potential duplication with http://drupal.org/project/views_nivo_slider - it would be good to add a link to that module to your project page and describe how they are different.

Your .info file and .js files have windows line endings. Please fix these to be Unix style line endings.

For the
$slider_id = ctools_cleanstring(check_plain($node->title), array('separator' => '_', 'lower case' => TRUE)); can you use drupal_html_class instead of the ctools and check_plain?

I see also in the block info that you are using node titles to create the name of blocks:

$blocks[$nid]['info'] = t('Media Nivo Slider: !title', array('!title' => $title));

This should use the @ placeholder instead of ! because the $title could contain javascript. The @ placeholder will filter the title using check_plain which should prevent xss.

Marking this back to needs work only for the block title issue. The rest are lower priority suggestions.

Thanks for your contribution, peronas! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.