CVS edit link for DeeLay

Hi,

I have built a small extension for the video filter module (http://drupal.org/projects/video_filter). The filtering is done by the video filter module, this module just enables video filter to be able to handle video hosted by KIT Digital / The Feedroom.

The module was built for use on The Economist Online, it is functional and working on our live site, and I want to contribute it back to the community in case any one else is using video hosted by KIT Digital / The Feedroom.

Thanks
Dominic

CommentFileSizeAuthor
#4 feedroom_codec.zip6.33 KBDeeLay
#1 feedroom_codec.zip5.94 KBDeeLay

Comments

DeeLay’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new5.94 KB

Attached source code

avpaderno’s picture

Issue tags: +Module review

Hello, and thank you for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

avpaderno’s picture

Status: Needs review » Needs work
  • This is a partial review.
  • The points reported in this review are not in order of importance / relevance.
  • Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
  • Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.
  1. /**
     * Implementation of hook_uninstall()
     */
    function feedroom_codec_uninstall() {
      // Find all the variable created by the system_settings_form.
      $result = db_query("SELECT name FROM {variable} WHERE name LIKE '%s%'", array('feedroom_codec'));
      while ($row = db_fetch_object($result)) {
        variable_del($row->name);
      }
    }
    

    Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.

  2. project = "feedroom_codec"
    

    That line needs to be removed because it is already added from the packaging script; having two different values for the project name confuses the update manager, which would not understand which project needs to be checked in order to report any module updates.

  3. Hook implementation comments should be like the following one:
    /**
     * Implements hook_menu().
     */
    

    As reported in Documenting hook implementations:

    If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

    /**
     * Implements hook_help().
     */
    function blog_help($section) {
      // ...
    }
    

    This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

    In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

    /**
     * Implements hook_form_FORM_ID_alter() for node_type_form().
     */
    function mymodule_form_node_type_form_alter(&$form, &$form_state) {
      // ...
    }
    

    This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

DeeLay’s picture

Status: Needs work » Needs review
StatusFileSize
new6.33 KB

Fixed the comment in feedroom_codec.install, also changed to explicitly only delete the variables that we know about.

Also removed the project = 'feedroom_codec' line.

Thanks
Dom

avpaderno’s picture

Status: Needs review » Fixed
  // Clear the variable cache to make sure they are gone.
  cache_clear_all('variables', 'cache');

It is not necessary, as the cache is already cleared from variable_del().

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

avpaderno’s picture

Component: Miscellaneous » new project application
Assigned: Unassigned » avpaderno
Issue summary: View changes
Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.