[D7] mixcloud_field

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.

Overall a well implemented module using all standard Drupal methods and hooks to provide a field which can be added to any content type. This modules show a strong understanding of Drupal methods and best practices and gives sufficient options to get a Mixcloud player or links rendered on the page.

Suggestions

These are merely suggestions and not blockers depending on whether they match the goals of your module.

Default Width & Height

define('MIXCLOUD_FIELD_DEFAULT_WIDTH', 300);
define('MIXCLOUD_FIELD_DEFAULT_HEIGHT', 120);

It might be helpful to move both of these settings into system variables using the variable_set function and the names "mixcloud_field_default_width" and "mixcloud_field_default_height". You can set the variables to your default values using hook_install and remember to also use hook_uninstall with variable_del("mixcloud_field_*") to clean up.

Mixcloud Player HTML

In the function "mixcloud_field_field_formatter_view" you render the HTML for the case "mixcloud_iframe" using strings and then replace URL values using preg_replace. It may be more user friendly to develop the query string using an array and then append the results of your logic using http_build_query after the '?'. This may make more sense to people who are not comfortable with even mild preg functions.

You could also take the player rendering one step further and move all of the code into a theme function or tpl file using hook_theme. This would allow people to override your theme or allow you to easily reuse the same render HTML if you need it some where else in your module, e.g. you add token support in the future.

The additions all look good to me.

manual review:
I agree with previous reviewers; very well done module. My only suggestion would be to remove the files[] in your .info file since D7 auto-includes those:

FILE: /var/www/drupal-7-pareview/pareview_temp/mixcloud_field.info
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
8 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
9 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

Other than that, looks great!

Moving to RTBC. However, some feedback:

There are still a few non-blocking findings @ http://pareview.sh/pareview/httpgitdrupalorgsandboxwoutersfrederik220117...

module file:
Variable 'form' might have not been defined (at line 131)
No need to mention Pseudo-hook.
TODO's should be listed as @todo: https://drupal.org/coding-standards/docs#todo

  $autoplay = ($settings['autoplay']) ? 'true' : 'false';
  $showartwork = ($settings['showartwork']) ? 'true' : 'false';

Variables should use underscores as a separator ~line 197.
Use url() to build urls ~line 208. This lets the URL get altered with hook_url_outbound_alter().

        $encoded_url = urlencode($item['url']);

        $oembed_url = $oembed_endpoint . '?hide_cover=1&hide_tracklist=1&embed_type=widget_standard&iframe=true&feed=' . ($encoded_url);
        $final_iframe = '<iframe frameborder="0" height="' . $settings['height'] . '" width="' . $settings['width'] . '" src="' . $oembed_url . '"> </iframe>';

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation. This is your moment to let the project shine. So do yourself a service and promote the module.

Non-blocking issues:

• The project page could use more detail
• "contenttype" should be two words in the README
• In the info file the description says "en" - I think it's meant to be "and"?
• Consider using drupal_attributes() in mixcloud_field_field_formatter_view() to build up your iframe
• A few style issues reported here http://pareview.sh/pareview/httpgitdrupalorgsandboxwoutersfrederik220117...
• I don't think there's a duplication, but it would be helpful if you could mention the differences between your module and https://www.drupal.org/project/mixcloud_filter and https://www.drupal.org/sandbox/adnasa/1821108

Checked for security, licensing, Drupal API, duplication, and individual account, no issues found.

Thanks for your contribution, drupal_sensei!

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.