Make template overridable by theme

after moving the videojs.tpl.php file to your theme did you clear the cache/theme registry?, per drupal core docs

it wasn't mentioned which is why I asked. Especially considering it is a common issue when tpl.php files aren't recognized in a theme.

Please reopen if this is still an issue.

I believe this is still an issue. It is related to this http://drupal.org/node/342350.

The implementation of hook_theme should look like the below. There should be three separate tpl files. Plus videojs.theme.inc should be moved to the videojs/theme folder. This would allow proper overriding from within a theme.

function videojs_theme() {
  return array(
    'videojs_formatter_videojs' => array(
      'arguments' => array('element' => NULL),
      'template' => 'videojs_formatter_videojs',
      'path' => drupal_get_path('module', 'videojs') . '/theme',
      'file' => 'videojs.theme.inc',
    ),
    'videojs_view' => array(
      'arguments' => array('view' => NULL, 'items' => NULL),
      'template' => 'videojs_view',
      'path' => drupal_get_path('module', 'videojs') . '/theme',
      'file' => 'videojs.theme.inc',
    ),
    'videojs' => array(
      'arguments' => array('items' => NULL, 'player_id' => NULL, 'attributes' => NULL),
      'template' => 'videojs',
      'path' => drupal_get_path('module', 'videojs') . '/theme',
      'file' => 'videojs.theme.inc',
    ),
  );
}

I discovered this trying to change preload="auto" to preload="none", in order to have the poster image load in IE.

The attached patch fixes the above problem.

I don't think that is a good idea. IMO, there is no need to confuse people with yet another template, since configuration could be done completely via the module's GUI.

I partially fixed this issue: the template file is still the same, but I added the path key like you suggested. Thanks!

I don't understand what should be better when the videojs.theme.inc file resides in the "theme" directory (besides the template file) instead of the "includes" directory of the module. Why should this allow the template to be overridden?

It is because of the following code:

      'template' => 'videojs',
      'path' => drupal_get_path('module', 'videojs') . '/theme',
      'file' => 'videojs.theme.inc',

The path variable determines both the path of the template file and the path of the include file. I had to add the path file such that the template file name could be set to a proper value. It used to be 'theme/videojs', which prevented overriding.

The 'path' array key of a theme hook is completely optional. It is not required to make a template overridable and most modules implementing an overridable theme template do not use it. If it is set, you loose the freedom of separating the directories for the include code file (which is not relevant to the end user) and the template (which is the only file interesting to the end user) and are forced to place both files into the same directory, making it more unclean to the end-user who is only interested in .tpl.php files. I cannot see any advantage of using 'path' at all - only disadvantages.

You're right, it is not pretty. I will try to remove the path key later.

The reason for using the path variable is to be able to place the tpl file in a subdirectory. I see very often that modules have a theme.inc that is located in the directory where the templates are, just look at views or date.

However, with a small trick it is possible to put the theme include in another directory:

    'videojs' => array(
      'arguments' => array('items' => NULL, 'player_id' => NULL, 'attributes' => NULL),
      'template' => 'videojs',
      'path' => $path,
      'file' => '../includes/videojs.theme.inc',
    ),

However, given that major modules use the theme.inc approach, I tend to go for that one too, renaming videojs.theme.inc to theme.inc.

The reason for using the path variable is to be able to place the tpl file in a subdirectory.

You should *not* use path at all. Really. It just doesn't make anything good. Forget the statement of jreashor in #6 above regarding path - it is simply not true what he thought. You do not need it either to make a template overridable nor for being able to place it in a sub directory. I suggest you revert the corresponding commit because the way it was before is better and cleaner:

/**
 * Implementation of hook_theme().
 */
function videojs_theme() {
  return array(
    'videojs' => array(
      'file' => 'includes/videojs.theme.inc',
      'template' => 'theme/videojs',
      'arguments' => array('items' => NULL, 'player_id' => NULL, 'attributes' => NULL),
    ),
  );
}

I only included one theme hook (videojs) in the code example above. You just need the array keys file, template and arguments, and by using

      'file' => 'includes/videojs.theme.inc',
      'template' => 'theme/videojs',

you perfectly separate the directories of both files "videojs.theme.inc" (in the "includes" sub directory) and "videojs.tpl.php" (in the "theme" sub directory).

If you name the file specified by file "videojs.theme.inc", "theme.inc" or "whatever.you.like.inc" does not matter at all. Just a matter of your own coding standard and style. The way it currently is is perfectly fine. You could also drop file completely and place the template_preprocess_HOOKs in your main .module file (hower, outsourcing it is the smarter way).

Hope you got me this time.

I have reverted the commit. If everything works like it should, I don't know what I should change to the module. I therefore close this issue. Thanks roball for the comments.

OK, the attached patch solves the problem for me. It turned out that the "videojs" theme hook is not needed at all.

I have successfully tested the following: Copy "videojs-formatter-videojs.tpl.php" from the module's "theme" directory into your default theme directory and flush all caches. Then on node display the video HTML is built by using the template residing in the default theme directory, so overriding works. There were a few bugs in the previous code that prevented that possibility.

I haven't made any tests with the other template "videojs-view.tpl.php", since I didn't investigate its purpose, but since it is a copy of "videojs-formatter-videojs.tpl.php", it should at least do the same thing as before (maybe nothing...).

Have to switch the Version to 6.x-2.x-dev so the testbot tests the patch against the 6.x-2.x branch.

#18: videojs-1102046-18.patch queued for re-testing.

As reported in the long-alive issue #1747890: Views style plugin not working, the "videojs-view.tpl.php" template does not do anything, and it seems that has never worked. Thus it is safe to commit this patch. Maybe we should also remove the whole view-related code, since it doesn't work anyway. I see no real usecase for that as well. A page with more than one embedded video on the same page does not make much sense for me.