heronog [heronog]

Hey there Herón, I reviewed this module over the weekend but it looks like there's still a bit to clean up.

• Indentation is still off, such as this:
  

    $form['cck_ooyala_global_pcode'] = array(
      '#type' => 'textfield',
    '#default_value' => variable_get('cck_ooyala_global_pcode', ''),
    '#title' => t('Ooyala Partner code'),
    );
  

• A lot of functions are missing PHPdoc.
• Where there is PHPdoc, it's not formatted properly.

However I'd like to say that this module is necessary in Contrib, rather than merging with existing solutions. I had previously thought that the best approach for Ooyala integration was integrating with Embedded Media Field. However Emfield does not support actually uploading videos at all, which is a central feature of this module. You could think of the Ooyala module as being similar to Flickr module, just because Emfield can embed Flickr photos, doesn't mean that it is a full-featured Flickr solution. Considering Drupal offers absolutely no integration with Ooyala currently, this module will be a big step up.

There are still quite a few things I'd like to see improved in the module, but I'd like to have an official home for filing issues rather than sending .zip files back and forth via e-mail or in a forum post (for example). Considering that Henonog has already been responsive and supportive of users trying to use his externally hosted module, I think that he'll continue doing so as a module maintainer.

There are indeed some coding standards clean-up required for this module to be ready for prime time. However, I would lobby for the CVS application to be approved because we have some time and resources available to help get the module ready for prime time and it'd be easier to do that through the issue queue rather than -- as Nate said -- pass the zip file back and forth until it's ready for an initial commit.

Hm, better but still off in all kinds of places. You might try making Coder pick up "Minor" issues instead of the default "Normal", perhaps it'd find more problems yet.

I'm not really all that happy with the result, as it pollutes the "pure text" fields with rendering options that are not adequate.

Coding-standards aside, I think this is actually the biggest problem and certainly something that would need fixing before making an official release.

The code still needs to be formatted as the coding standards reports.

  include(drupal_get_path('module', 'cck_ooyala') . "/ooyala_partner_api.inc");

include is not a function: the parentheses are not obligatory. The code should then probably call module_load_include().

  $tag = module_invoke('taxonomy', 'get_term', $tag_id);

That is conceptually not correct; taxonomy_get_term() is not a hook, but a plain function.
I know that now it doesn't change anything, but there is a proposal to use a different way to name the hook; if that is going to be changed, module_invoke('taxonomy', 'get_term', $tag_id) would probably call taxonomy_hook_get_term(), or taxonomy__get_term(). Still it is not clear why to call module_invoke() in this case, when it is easier to directly call the function (considering also that Drupal 7 dropped the functions registry).

KiamLaLuno, as I noted above there's plenty of other problems besides the ones you just pulled out. I'm hoping to work with heronog on fixing these problems. However I would prefer that we work them out in an actual project, rather than be forced to upload 20 zip files here in a preliminary back-and-forth. I can assure you we'll get this module fitting into coding standards, I'm quite familiar with them.

If you can approve this account we'll work out the issues in the project queue like any other project.

It seems reasonable. I am going to approve this application.

Thanks Kiam!

I am giving credits to the users who participated in this issue.