[D7] KinSlideshow

add git repository.

Please add a link to your sandbox project page, in addition to the git repository location.

Edit: Never mind ... I added the link.

I've taken a quick look at the sandbox. The coding structure and logic both look pretty clean, but I do have a few of questions/suggestions/concerns:

project page

Please take a moment to make your project page follow tips for a great project page. Even though the project is still in the 'sandbox' phase, reviewers are going to look to your project page to get a sense of what the module is supposed to do ... and currently, it's a little too empty to provide the necessary insight.

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

License

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

files[] without classes or interfaces

The line files[] = kinslideshow.module should be removed from your .info file. It's only necessary to declare files[] if they declare a class or interface.

_get_all_image_fields()

This, and all other functions, must be prefixed with your module namespace in order to avoid the potential for duplicate function definitions between this and other modules installed on a given website.

if (db_table_exists('field_data_' . $field_name))

Ideally, the preference would be to use the appropriate API method for this check, rather than simply checking for the presence of the field's database table. In this case, field_info_field() or field_info_instance() may be good candidates. Also, if possible, I would recommend using the various Field related APIs (links at the bottom of http://api.drupal.org/api/drupal/modules!field!field.module/group/field/7) and EntityFieldQuery in place of the query you've constructed ... though I recognize this may be difficult given the number of joins present.

3rd party code and licensing concerns

KinSlideshow appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org, as per policy described in the getting involved handbook. It appears that the javascript library is licensed under an Apache Version 2.0 license, while all code to be hosted on Drupal.org must be GPLv2 licensed. Given the licensing incompatibility, the preferred approach would be to provide instructions in the README file explaining where an end user can obtain the code; and then leveraging the Libraries API module (which is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org).

I've ran an automated coding standards scan, with the results available at http://pareview.ventral.org/pareview/httpgitdrupalorgsandboxelim05118889... ... not alot there, mostly just whitespace issues. If you can address the items flagged by the review script, it should save you from other reviews which don't offer anything beyond telling you to fix your whitespace. ;)

elim051:

Even with permission, the software would have to be licensed under the GPLv2 license in order to be allowed on Drupal.org. By hosting code on Drupal.org, it falls under the drupal.org repository licensing, and is automatically considered GPLv2 licensed. The KinSlideshow code is listed as Apache 2.0 licensed, so it doesn't meet the licensing requirements.

I see two options here:

1. The original author re-releases the code as GPLv2, or
2. You utilize the Libraries API and provide the direct download link in your README.txt file, which is the preferred and recommended method used by 99% of Drupal.org modules leveraging external libraries.

Exceptions to the above have been made for 3rd party code included in Drupal core, but each needed to obtain a specific exception. To my knowledge, no contrib module has been granted this exception to date.

There is another motivation to using the Libraries API approach ... while this is not likely with the KinSlideshow module in particular, consider the case where two different modules include a particular 3rd party jquery library in their module folder. This opens up all sorts of possibility for code conflicts, especially if they use two different software versions of the particular library. By using the Libraries API, both modules can leverage a single instance of the library codebase, and the potential for namespace/code/versioning conflicts is avoided.

Even though end users are not likely to encounter two different modules which use the KinSlideshow library in particular, it would be prudent to leverage the Libraries API in order to maintain consistency with the other libraries used by your end users.

Assigning to self as a follow-up reminder. (Though another reviewer can feel free to review if I haven't got to it yet.)

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Found a couple of errors when doing automatic test with ventril.org.

• Line 36 should be split across multiple lines
• You should wrap the text part of the l() function inside a t().

You should handle the possibility that someone puts your block in a region before they add content to it. Now it throws errors:

• Notice: Undefined index: node in _kinslideshow_get_kinslideshow_content() (line 195 of /home/anders/public_html/drupal/sites/all/modules/kinslideshow/kinslideshow.module).
• Warning: array_keys() expects parameter 1 to be array, null given in _kinslideshow_get_kinslideshow_content() (line 195 of /home/anders/public_html/drupal/sites/all/modules/kinslideshow/kinslideshow.module).

Other than that it was easy to set up and the readme-file was easy to understand.

Hi elim051.

I installed your module, it works flawlessly. Is a little uncool that we can add only one block, it would be great if we could add more block instances.
I think Your project page is too short, you have a good README.txt , maybe copy them to the project page?

Regards.

From automatic review (http://pareview.ventral.org/pareview/httpgitdrupalorgsandboxelim05118889...):
36 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
36 | ERROR | The $text argument to l() should be enclosed within t() so that
| | it is translatable

elim051, I think you're probably right, but can you explain *why* you think the reported errors don't need to be fixed?

You project page should include more detail, consider copying over the text from your README.

You don't need the empty return statement in kinslideshow_block_save().

The l() comment on line 36 is ignorable, because the text is a plain url and does not require translation. The line length issue is valid though - you could break it up into multiple lines for readability.

Minor issues though. I don't think there are security problems.

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

Well, it's been two weeks and the code still looks ok. All of the above issues still apply however, and there's a few more at http://ventral.org/pareview/httpgitdrupalorgsandboxelim0511888950git.

Thanks for your contribution, elim051!

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.

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