Port Kaltura to Drupal 7

+1 I agree! this is very important question that should have been answer one way or the other by now. I have decide on a video solution, and i think this would be great if.. it supports D7.

Hi guys,

There are plans to support D7 of course.
There is no specific timeline yet though as we're still working on closing the current release and the next Kaltura version.
If you're interested and can help getting D7 version done, let us know - we'd appreciate any help, ideas, patches, etc.!

Some update, along with plans for D7 - http://blog.kaltura.org/drupal-module-love

D7 highlights:
1. Drupal 7 compliancy (well duh! :).
2. Complete Drupal coding standards compliance. Detailed testing for:
-- a. Easier integration with other key modules. (Can you suggest the top 5 contrib modules to test with?)
-- b. Easier skinning & theming, and compatibility tests with a few popular themes. (Can you suggest top 5 themes to test with?)
3. Support for HTML5 Video playback (based on the Kaltura html5 video library).
4. Keeping the existing functionality of the Kaltura Module 6.x-2-dev.
5. Develop unit tests for new and existing functionality.
6. Better documentation: inline and guides.
7. Supporting fields and deprecating nodes.

Thanks guys. I'm shortly going to update CVS HEAD to bring in the updates from DRUPAL-6--2.

I'll then add this new branch to the Github repo so Yaniv can start sharing his work to date and we can get people testing these updates.

Hi
I have some pre alpha code in github
https://github.com/yaniv-li/kaltura-7.x-3.0
https://github.com/yaniv-li/drupal-kaltura
I'll move the code to drupal git once it up
it has a lot dsm's and other debug messages.
I just push to it at the end of the day

Yaniv, as discussed a few weeks ago please post your work as either a patch to DRUPAL-7--2 or as a fork of the DRUPAL-7--2 version @
https://github.com/GiantRobot/drupal-kaltura

Having multiple separate repos on Github makes no sense and makes things harder for us both - please don't.

If you are working from current CVS or the existing Github repo then you'll more easily be able to track bug fixes like the commits I made yesterday.

We agreed on this when we discussed it in our call. What happened?

EDIT: apologies if the above is worded strongly - you're welcome to do what you want in a development repo, provided that you're tracking current CVS updates and posting your work back either as pull reqs via github or patches here. You'll save us both work if you make that workflow your default.

Hi
the D7 is a big rewrite so there is no point tracking the CVS
I tried pushing the code to branch from your git hub repo but for some reason it didn't let me push
I can try again if it would make things easier to move git in drupal.org

Yes, please do so. If that doesn't work, you can easily use the alternate approach I suggested on our call; clone the version from GiantRobot/drupal-kaltura to yaniv-li/drupal-kaltura, check out 7.x-2.x locally as 7.x-3.x, pull your commits from yaniv-li/kaltura-7.x-3.0, push back to GitHub and work from there.

If you can't do that, grab me and we can walk through it in #drupal-gitsupport.

The code in branch DRUPAL-7--2 will be the reference point your work on D7 is compared with (currently up to date with DRUPAL-6--2). Your changes will be reviewed against that which is why I advised you not to work from the generated tarballs (your patches will include changes which do not match the files in CVS or Git).

The fact that it's a "big rewrite" is a very good reason to use Git (or CVS) to track current updates in the module. Some of your changes will still be applicable to 6.x-2.x, and some of the bugfixes in 6.x-2.x will be applicable to 7.x-3.x. Source control helps use collaborate on that and will reduce duplication of effort.

Uploading a patch showing Yaniv's work to date (generated by pulling Yaniv's DRUPAL-7--2 and diffing against current 7.x-2.x which is current 6.x-2.x). Posting for review only - this patch is not ready.

+++ b/README.txt
@@ -1,3 +1,4 @@
+// $Id: README.txt,v 1.2.2.3 2010/07/07 09:34:53 kaltura Exp $

Do not need to reintroduce CVS keywords.

+++ b/README.txt
index c218519..eb5e8c8 100644
--- a/includes/kaltura.admin.inc
+++ b/includes/kaltura.admin.inc

Lots of changes in kaltura.admin.inc which duplicate work in #1026426: Installation issues: FAPI rewrite of installer.

+++ b/includes/kaltura.admin.inc
@@ -56,11 +57,18 @@ function kaltura_register_partner() {
+  $config->serviceUrl = $_REQUEST['kaltura_server_url']; //change service url for CE regestration

Use FAPI rather than $_REQUEST values. We'll try to get #1026426: Installation issues: FAPI rewrite of installer merged in which will resolve that.

+++ b/includes/kaltura.admin.inc
@@ -713,72 +732,66 @@ function kaltura_import_entries_page() {
-  else {
+  else
+  {

+++ b/includes/kaltura.admin.inc
@@ -713,72 +732,66 @@ function kaltura_import_entries_page() {
+  else

+++ b/includes/kaltura.admin.inc
@@ -713,72 +732,66 @@ function kaltura_import_entries_page() {
+      if($total==0)$foundFlag = false;

+++ b/includes/kaltura.admin.inc
@@ -713,72 +732,66 @@ function kaltura_import_entries_page() {
+      if($mediaTotal==0)$foundFlag=false;

#850378: Coding standards... (some newly introduced code in this patch does not follow coding standards; all new code should do so, even if the surrounding code is still a mess)

+++ b/includes/kaltura.notification.inc
@@ -117,125 +135,105 @@ function kaltura_test_notification_received() {
+  try {
+    db_insert('node_kaltura')
+      ->fields($entry)
+      ->execute();
   }

is db_insert() likely to raise an exception?

+++ b/includes/kaltura.notification.inc
@@ -117,125 +135,105 @@ function kaltura_test_notification_received() {
+function mk_kaltura_array ($array) {
+  $ent['kaltura_entryid'] = $array['entry_id'];
+  $ent['kaltura_tags'] = $array['tags'];
+  $ent['kstatus'] = $array['status'];
+  $ent['kaltura_media_type'] = $array['media_type'];
+  $ent['kaltura_thumbnail_url'] = $array['thumbnail_url'];
+  $ent['kaltura_partner_data'] = $array['partner_data'];
+  $ent['kaltura_width'] = $array['width'];
+  $ent['kaltura_height'] = $array['height'];
+  $ent['kaltura_download_url'] = $array['download_url'];
+  $ent['kaltura_title'] = $array['name'];
+  return $ent;
 }
 
+function kmc_obj_to_drupal_array($obj) {
+  $ent['kaltura_entryid'] = $obj->id;
+  $ent['kaltura_tags'] = $obj->tags;
+  $ent['kaltura_admin_tags'] = $obj->adminTags;
+  $ent['kstatus'] = $obj->status;
+  $ent['kaltura_media_type'] = $obj->mediaType;
+  $ent['kaltura_duration']= $obj->duration;
+  $ent['kaltura_thumbnail_url'] = $obj->thumbnailUrl;
+  $ent['kaltura_partner_data'] = $obj->partnerData;
+  $ent['kaltura_source'] = $obj->sourceType;
+  $ent['kaltura_width'] = $obj->width;
+  $ent['kaltura_height'] = $obj->height;
+  $ent['kaltura_download_url'] = $obj->downloadUrl;
+  $ent['kaltura_views'] = $obj->views;
+  $ent['kaltura_plays'] = $obj->plays;
+  $ent['kaltura_votes'] = $obj->votes;
+  $ent['kaltura_rank'] = $obj->rank;
+  $ent['kaltura_total_rank'] = $obj->totalRank;
+  $ent['kaltura_title'] = $obj->name;
+  $ent['kaltura_description'] = $obj->description;
+  return $ent;
+}

#352670: Function names must be prefixed by the module name, #850378: Coding standards...

+++ b/includes/kaltura.themeing.inc
@@ -362,22 +364,25 @@ function kaltura_get_params_from_tag($tag) {
-    echo theme('kaltura_contribution_wizard_field', $theme_params, $field_id, $no_collect_entries, $kshow_id, $add_filter);
+    //TODO: change this to tpl file
+    echo theme_kaltura_contribution_wizard_field($theme_params, $field_id, $no_collect_entries, $kshow_id, $add_filter);

theme_kaltura_contribution_wizard() needs update to D7 here so the hack of calling the func directly instead of via theme() can be reverted. Can see you're only partially through porting the theme layer to D7 so no crisis, just want to flag that the call here will need to be updated when done or we'll lose the ability for themes to override the rendering.

+++ b/includes/kaltura.themeing.inc
@@ -733,22 +742,22 @@ function kaltura_simple_editor() {
 function theme_kaltura_simple_editor($theme_params, $no_refresh) {

theme functions accept a single array(), as above this is D7 theme in progress.

+++ b/kaltura.info
@@ -1,6 +1,18 @@
+version = "7.x-3.0-dev"

Version is added by drupal.org release build scripts, and should not be included in the .info file. Refer #445294: Remove incorrect version data from info file and writing .info files.

+++ b/kaltura.info
@@ -1,6 +1,18 @@
+files [] = kaltura_client/generate.php
+files [] = kaltura_client/KalturaClientBase.php
+files [] = kaltura_client/KalturaClient.php
+files [] = kaltura_client/kaltura_helpers.php
+files [] = kaltura_client/kaltura_logger.php
+files [] = kaltura_client/kaltura_notification_client.php
+files [] = kaltura_client/kaltura_settings.php
+files [] = kaltura_client/Php5ClientGenerator.php
+files [] = includes/kaltura.admin.inc
+files [] = includes/kaltura.notification.inc
+files [] = includes/kaltura.themeing.inc

space is not required (files[] not files []), and this is only necessary for files containing classes and interfaces. pretty sure the last three here don't (unless you've added some there)

+++ b/kaltura.install
@@ -16,34 +17,21 @@ function kaltura_schema() {
-      'kaltura_entryId' => array(
-        'description' => '',
+      'kaltura_entryid' => array(
+        'description' => 'Kaltura entry id',

we'll need to handle this column renaming for upgrades then?

+++ b/kaltura.install
@@ -159,8 +147,32 @@ function kaltura_schema() {
-    'primary key' => array('vid'), /* #1022310: Duplicate entry 'xx' for key 1 query: INSERT INTO node_kaltura ( ... ) */
+    'primary key' => array('kaltura_entryid'),

Can you update that issue and discuss this approach please? That patch should be filed against #1022310: Duplicate entry 'xx' for key 1 query: INSERT INTO node_kaltura ( ... ), not this issue.

+++ b/kaltura.install
@@ -208,10 +219,10 @@ function kaltura_uninstall() {
-  $get_vars = 'SELECT name FROM {variable} WHERE name LIKE \'%s\'';
-  $result = db_query($get_vars, 'kaltura\_%');
+  $get_vars = 'SELECT name FROM {variable} WHERE name LIKE :name';
+  $result = db_query($get_vars, array('name' => 'kaltura\_%'));
   $vars_deleted = '';
-  while ($var = db_fetch_object($result)) {
+  foreach ($result as $var) {
     variable_del($var->name);
     $vars_deleted .= $var->name .',';
   }

These changes look OK, but it's wierd that this isn't just a DELETE WHERE don't you think?

+++ b/kaltura.module
@@ -144,6 +158,27 @@ function kaltura_menu() {
+  // maybe variable check here and don't create the handler on unless
+  // enabled
+  $items['crossdomain.xml'] = array(
+    'title' => 'crossdomain.xml (auto by kaltura module)',
+    'file' => 'includes/kaltura.crossdomain.inc',
+    'description' => 'Provide crossdomain.xml handler for Kaltura',
+    'page callback' => 'kaltura_crossdomain_xml',
+    'type' => MENU_CALLBACK,
+    'access callback' => TRUE,
+  );
+

This is a separate issue, #887354: Don't require crossdomain.xml be installed in the Drupal site root, so please do as for #1022310 above.

+++ b/kaltura.module
@@ -388,21 +428,15 @@ function kaltura_list_entries() {
+function theme_list_of_entries($entries, $allow_insert = FALSE, $field_name = '') {

#352670: Function names must be prefixed by the module name again

OK lots of stuff there and I only got down to kaltura_block_info() or so! Hope this is helpful for you.

NB: I've tried to avoid repeating comments here, so that you can get a feel for what needs sorting, rather than have me list each instance of $Id$ that's been re-introduced. Please review the changes in the light of the comments above and apply fixes globally as appropriate.

Hi Chris,
My name is Lior and I'm working with Yaniv (gambaweb) on this.
Thanks for the excellent input. We'll be incorporating it shortly in to the D7 branch.
I'd love to meet up on irc/skype and see how we can commit yaniv's work to the new d7 branch in git.
Can't we initiate the code directly based on the d7 code rather then patching the branch? I'm not sure how the new git stuff and management works.

We're trying to get a beta of the core functionality up and running by DrupalCon so I want to address the deployment and distribution now and to have you brainstorm with us directly over the candidate code base.
Are you planning to attend drupalcon? We might conduct a code sprint on the module if you or other interesting parties are interested...

The cool D7 news from today was that I added a kaltura field to the comments (obsoleting teh need for kaltura_comments) and I added a video_bio instance of the kaltura fild to a user so the basic field is working pretty well!
Let's see how we can work together to bring this to a bigger set of testers and to brainstorm on the next challanges (views support, edge scenarios etc...)

Great news about the field implementation - looking forward to having a play with 7.x myself now :)

The patch submission process gives us an opportunity to review the code before putting it into Git. I think that's an important step. Best practices say: post all code to an issue and have it marked RTBC (ideally by another maintainer) before committing, so that's my stance.

As long as 7.x shares code with 6.x, the regular issue / patch workflow means we can easier fix issues in both versions. Eg Yaniv can save reworking the installer by testing and improving existing work in #1026426: Installation issues: FAPI rewrite of installer (and other issues above where time in 7.x dev could be saved by tracking fixes in the 6.x branch rather than reimplementing them). Likewise, much of the improvements from 7.x will be applicable to the substantial number of 6.x sites around. So that's a win both ways.

So: we should generate the patches between branches (eg existing Git 7.x-2.x vs Yaniv's work), review here, and then apply when the patches pass review. At that point it's probably cleanest to apply as a single merge commit rather than as a set of separate commits.

Let's talk soon! Skype or IRC is good, I'll check into #kaltura later this arvo my time to see if you're in.

thanks for the comments
as for improving #1026426 the all registration process is going to change soon so it is kind of a temporary hack just for now

here is the patch for my D7 port

+++ b/README.txt
@@ -1,3 +1,4 @@
+// $Id: README.txt,v 1.2.2.3 2010/07/07 09:34:53 kaltura Exp $

do not reintroduce $Id$

+++ b/includes/kaltura.admin.inc
@@ -56,11 +57,18 @@ function kaltura_register_partner() {
+  $config->serviceUrl = $_REQUEST['kaltura_server_url']; //change service url for CE regestration

as per previous review

+++ b/includes/kaltura.admin.inc
@@ -69,17 +77,16 @@ function kaltura_register_partner() {
+    variable_set('kaltura_server_url', $_REQUEST['kaltura_server_url']);

as per previous review

+++ b/includes/kaltura.notification.inc
@@ -58,18 +59,35 @@ function kaltura_notification_handler() {
+  try {
+    db_insert('kaltura_notifications')
+      ->fields($notify)
+      ->execute();
+  }
+  catch (Exception $e) {
+    watchdog('kaltura', 'problem updateing notifaction table');
+  }

why not just db_write_record()?

AFAIK unlikely to raise an exception, so not sure why it's wrapped in try{}

seems like this takes eight lines when one would do?

+++ b/includes/kaltura.notification.inc
@@ -117,125 +135,105 @@ function kaltura_test_notification_received() {
+  try {
+    db_insert('node_kaltura')
+      ->fields($entry)
+      ->execute();
   }

not sure why this is needed. if you are really seeing exceptions here, you want to know why - not just record it in watchdog.

+++ b/includes/kaltura.notification.inc
@@ -117,125 +135,105 @@ function kaltura_test_notification_received() {
+function kaltura_mk_karray($array) {
+  $ent['kaltura_entryid'] = $array['entry_id'];
+  $ent['kaltura_tags'] = $array['tags'];
+  $ent['kstatus'] = $array['status'];
+  $ent['kaltura_media_type'] = $array['media_type'];
+  $ent['kaltura_thumbnail_url'] = $array['thumbnail_url'];
+  $ent['kaltura_partner_data'] = $array['partner_data'];
+  $ent['kaltura_width'] = $array['width'];
+  $ent['kaltura_height'] = $array['height'];
+  $ent['kaltura_download_url'] = $array['download_url'];
+  $ent['kaltura_title'] = $array['name'];
+  return $ent;
 }
 
+function kaltura_kmc_obj_to_drupal_array($obj) {
+  $ent['kaltura_entryid'] = $obj->id;
+  $ent['kaltura_tags'] = $obj->tags;
+  $ent['kaltura_admin_tags'] = $obj->adminTags;
+  $ent['kstatus'] = $obj->status;
+  $ent['kaltura_media_type'] = $obj->mediaType;
+  $ent['kaltura_duration']= $obj->duration;
+  $ent['kaltura_thumbnail_url'] = $obj->thumbnailUrl;
+  $ent['kaltura_partner_data'] = $obj->partnerData;
+  $ent['kaltura_source'] = $obj->sourceType;
+  $ent['kaltura_width'] = $obj->width;
+  $ent['kaltura_height'] = $obj->height;
+  $ent['kaltura_download_url'] = $obj->downloadUrl;
+  $ent['kaltura_views'] = $obj->views;
+  $ent['kaltura_plays'] = $obj->plays;
+  $ent['kaltura_votes'] = $obj->votes;
+  $ent['kaltura_rank'] = $obj->rank;
+  $ent['kaltura_total_rank'] = $obj->totalRank;
+  $ent['kaltura_title'] = $obj->name;
+  $ent['kaltura_description'] = $obj->description;
+  return $ent;
+}
+
+

not sure about these. they need to have more meaningful names i think. if they are really meant to be re-used then they prob don't belong in .admin.inc

+++ b/includes/kaltura.themeing.inc
@@ -362,22 +364,25 @@ function kaltura_get_params_from_tag($tag) {
+    //TODO: change this to tpl file
+    echo theme_kaltura_contribution_wizard_field($theme_params, $field_id, $no_collect_entries, $kshow_id, $add_filter);

as per previous review

+++ b/kaltura.info
@@ -1,6 +1,17 @@
+; $Id: kaltura.info,v 1.1.2.5 2010/07/07 09:34:53 kaltura Exp $: kaltura

as per previous review

+++ b/kaltura.info
@@ -1,6 +1,17 @@
+files[] = includes/kaltura.admin.inc
+files[] = includes/kaltura.notification.inc
+files[] = includes/kaltura.themeing.inc

as per previous review

+++ b/kaltura.install
@@ -16,34 +17,21 @@ function kaltura_schema() {
-      'kaltura_entryId' => array(
-        'description' => '',
+      'kaltura_entryid' => array(
+        'description' => 'Kaltura entry id',

as per previous review

+++ b/kaltura.install
@@ -159,8 +147,32 @@ function kaltura_schema() {
-    'primary key' => array('vid'), /* #1022310: Duplicate entry 'xx' for key 1 query: INSERT INTO node_kaltura ( ... ) */
+    'primary key' => array('kaltura_entryid'),

as per previous review

+++ b/kaltura.module
@@ -152,38 +176,38 @@ function kaltura_menu() {
-    'kaltura_list_of_entries' => array(
-      'arguments' => array('element' => NULL),
+    'list_of_entries' => array(
+      'variables' => array('entries' => array(), 'allow_insert' => FALSE, 'field_name' => NULL),

this is a regression against #352670: Function names must be prefixed by the module name

+++ b/kaltura.module
@@ -152,38 +176,38 @@ function kaltura_menu() {
-    'kaltura_contribution_wizard_field' => array(
+    'contribution_wizard_field' => array(

#352670: Function names must be prefixed by the module name

I've reviewed about a third of the way through the patch, and the final comment here is from hook_theme(). There are plenty of things to fix here, and some of these changes reverse fixes made in other issues. I haven't flagged minor coding standards issues in this review.

I'm not going to mark this RTBC myself :) but I'm happy to get this initial work in as 7.x-2.x even though it has plenty of work to be done; that means we can get some 7.x-2.x-dev tarballs for people to test with. We can then break the various items out into separate issues against 7.x-2.x-dev and co-ordinate a release for 7.x-2.0 from there.

How does that sound?

Hi Grobot,
After our session tonight in IRC we decided to commit and push to the drupal git repository to check the packaging and to use the drupalcon momentum to get more developers on board to test and to use it in their D7 sites and challanges.
We acknowledge this is currently alpha quality code yet we will use the next code sprints to get this to beta quality.
Great work guys!

Yaniv, please can you commit the code marked RTBC above to Git so I can publish the 7.x-2.x-alpha1 release?

Workflow to do this - typing from memory here so please verify independently or compare to the instructions given yesterday morning in IRC.

1. Switch to local copy of git.drupal.org 7.x-2.x
2. Apply patch from issue: curl http://drupal.org/files/issues/kaltura-1019652.patch | patch -p1 (use --dry-run first to verify)
3. git add [files affected by patch]
4. git commit -m '[message as per http://drupal.org/node/1061754]'
5. git push origin 7.x-2.x

Please do not include any changes which have not been reviewed already.

Thanks!

Update about the D7 alpha release - http://blog.kaltura.org/kaltura-drupal-7-module

Is anyone still working on this? I have been working through the dev release currently posted and made a few changes, but where are we? I can help and would like to know where to start so I'm not duplicating someone else work.

Kaltura is available on D7 for a long time now.