Implementation of hook_islandora_CMODEL_PID_derivative() is incorrect

[McFateM]

I'm trying to do some mass ingest of oral histories using the Islandora Multi-Importer (IMI) module and keep running into this problem. At one point the IMI tries to call islandora_oralhistories_islandora_oralhistoriesCModel_islandora_derivative, which is defined in the .module file, but this hook is to be called with NO arguments, and it only returns an array of derivative specs. In the IOH module this function requires an Abstract Object argument and it actually performs the derivative creation.

Compare the following from the basic image solution pack, to the same hook function from IOH...

/**
 * Implements hook_islandora_CMODEL_PID_derivative().
 */
function islandora_basic_image_islandora_sp_basic_image_islandora_derivative() {
  return array(
    array(
      'source_dsid' => 'OBJ',
      'destination_dsid' => 'TN',
      'weight' => '0',
      'function' => array(
        'islandora_basic_image_create_thumbnail',
      ),
      'file' => drupal_get_path('module', 'islandora_basic_image') . '/includes/derivatives.inc',
    ),
    array(
      'source_dsid' => 'OBJ',
      'destination_dsid' => 'MEDIUM_SIZE',
      'weight' => '1',
      'function' => array(
        'islandora_basic_image_create_medium_size',
      ),
      'file' => drupal_get_path('module', 'islandora_basic_image') . '/includes/derivatives.inc',
    ),
  );
}

/**
 * Implements hook_islandora_CMODEL_PID_derivative().
 */
function islandora_oralhistories_islandora_oralhistoriesCModel_islandora_derivative(AbstractObject object, $ds_modified_params = array()) {
  $derivatives = array();

  if (variable_get('islandora_oralhistories_media_uploaded_type') == 'video') {
    if (module_exists('islandora_video') && function_exists('islandora_video_islandora_sp_videoCModel_islandora_derivative')) {
      $derivatives = islandora_video_islandora_sp_videoCModel_islandora_derivative();
    }
    else {
      drupal_set_message(t('Error in creating video derivatives. Cannot find required functions.'), 'error', FALSE);
      drupal_exit();
    }
  }

  if (variable_get('islandora_oralhistories_media_uploaded_type') == 'audio') {
    if (module_exists('islandora_video') && function_exists('islandora_video_islandora_sp_videoCModel_islandora_derivative')) {
      $derivatives = islandora_audio_islandora_sp_audiocmodel_islandora_derivative();
    }
    else {
      drupal_set_message(t('Error in creating audio derivatives. Cannot find required functions.'), 'error', FALSE);
      drupal_exit();
    }
  }
  if (variable_get('islandora_oralhistories_make_vtt', TRUE)) {
    $derivatives[] = array(
      'source_dsid' => 'TRANSCRIPT',
      'destination_dsid' => 'MEDIATRACK',
      'function' => array(
        'islandora_oralhistories_create_vtt',
      ),
      'file' => drupal_get_path('module', 'islandora_oralhistories') . '/includes/derivatives.inc',
    );
  }

  // Create a derivative if a text/vtt file is ingested and the config is true.
  if (variable_get('islandora_oralhistories_vtt_index_ds_generation', TRUE)) {
    // We don't know the datastream id,
    // thus need to filter if from the object keys.
    $arr_vtt_data_streams = array();
    foreach ($object as $key => $value) {
      if ($object[$key]->mimetype == "text/vtt") {
        array_push($arr_vtt_data_streams, $key);
      }
    }

    // Loop through all MEDIATRACK or TRANSCRIPT text/vtt datastreams to see
    // if derivative needs to be created.
    foreach ($arr_vtt_data_streams as $key => $value) {
      $source_id = $value;
      $target_id = "INDEX" . $value;

      $derivatives[] = array(
        'source_dsid' => $source_id,
        'destination_dsid' => $target_id,
        'function' => array(
          'islandora_oralhistories_create_vtt_indexing_datastream',
        ),
        'file' => drupal_get_path('module', 'islandora_oralhistories') . '/includes/derivatives.inc',
      );
    }
  }

  return $derivatives;
}

[McFateM]

OK, looking at the code I see the immediate problem... The hook implementation does return an array of derivative specs as prescribed, but it appears that an Abstract Object was added to the argument list for this function to support the islandora_oralhistories_make_vtt option, but requiring this argument is not compatible with the design of this hook function.

So I made two simple changes in the code and now it is working for me.

In islandora_oralhistories.module I changed lines 261-265 to read:

/**
 * Implements hook_islandora_CMODEL_PID_derivative().
 */
function islandora_oralhistories_islandora_oralhistoriesCModel_islandora_derivative(
  AbstractObject $object = NULL, $ds_modified_params = array()) {

This makes $object an optional argument so that the function works as intended when called by IMI.

I also changed line 299 to this:

  if ($object && variable_get('islandora_oralhistories_vtt_index_ds_generation', TRUE)) {

This way, the function doesn't blow up when it is called without an $object.

[MarcusBarnes]

Thanks @McFateM. Would you be able to do some more testing and then create a pull-request? I'll schedule some time to review and test.

[McFateM]

I can certainly do more testing since I need to ingest a few dozen OH objects this week. Not sure about the pull-request... I made these little changes on the fly and am honestly not sure my codebase is in a state that would allow me to properly pull against your Github repo. I'm affraid there might be lots of unrelated merge conflicts to deal with due to other changes I have experimented with.

[MarcusBarnes]

@McFateM OK. I'll still schedule some time and maybe reach out to you if I need more information on the suggested changes. Thanks again for using the module and testing against the multi-importer. FYI, there is an older issue https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/issues/1 that may be related to this (if only in terms of batch functionality).

[kstapelfeldt]

For review and inclusion in release.

[MarcusBarnes]

Addressed in pull-request #128 (merged with commit ca77d27).