I'm trying to use a library for a carousel skin and I'm unable to reference the path without totally hacking it in. I'd honestly rather have people enter the full path like they do everywhere else, instead of running into problems or having to hack it together like this. What do you think?

function skins_skinr_skin_carousel_info() {
  global $base_path;
  $jcarousel_path = libraries_get_path('jcarousel');
  $skins['jcarousel'] = array(
    'default status' => 1,
    'title' => t('Carousel'),
    'description' => t('Display the content in a carousel.'),
    'type' => 'select',
    'attached' => array(
      'css' => array('carousel.css'),
      'js' => array(
        'http://' . $_SERVER['HTTP_HOST'] . $base_path . $jcarousel_path . '/lib/jquery.jcarousel.min.js',
        'carousel.js',
      ),
    ),
    'options' => array(
      'horizontal' => array(
        'class' => array('jcarousel', 'jcarousel-horizontal'),
        'title' => t('Horizontal'),
      ),
      'vertical' => array(
        'class' => array('jcarousel', 'jcarousel-vertical'),
        'title' => t('Veritcal'),
      ),
    ),
  );
  return $skins;
}
CommentFileSizeAuthor
#5 patch_commit_02b9f28fc4f3.patch1.37 KBmoonray

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Two options here:

  1. Implement special support for Libraries API, so skin (definitions) can rely (require/depend) on external libraries, or otherwise cannot be enabled/used.

    This might be a good idea to do either way, since Skinr's "distributed" skins being placed in the sites directory basically follow the same rules and methodologies as external libraries. Next to depending on external libraries, a skin may also depend on a certain module or theme being installed.

  2. Use the 'dependencies' definition of #attached to only load a skin's support files when the dependencies can be loaded.

    This approach requires some work in Libraries API to make it work with those #attached 'dependencies' -- http://drupal.org/project/jquery module basically has a similar requirement, although that module is trying to load external libraries as a replacement for built-in/module-provided libraries registered via hook_library().

moonray’s picture

moonray commented

For 2. this seems to be the issue @sun is referring to #864376: Loading of external libraries.

Not sure what the desired skin structure would look like for a library include... @jacine, @sun, care to suggest something?

jacine’s picture

jacine
she/her
Location New York
commented

@moonray I'm not familiar with Libraries API, so not sure what it would look like, but hopefully it doesn't have to change the structure too much.

moonray’s picture

moonray commented

@jacine: Would this be a more reasonable way to do this?
If so, we would only need to exclude paths with a '/' prefix from having the local path prepended, just like paths that start with 'http://' are excluded.

<?php
function skins_skinr_skin_carousel_info() {
  $skins['jcarousel'] = array(
    'default status' => 1,
    'title' => t('Carousel'),
    'description' => t('Display the content in a carousel.'),
    'type' => 'select',
    'attached' => array(
      'css' => array('carousel.css'),
      'js' => array(
        libraries_get_path('jcarousel', TRUE) . '/lib/jquery.jcarousel.min.js',
        'carousel.js',
      ),
    ),
    'options' => array(
      'horizontal' => array(
        'class' => array('jcarousel', 'jcarousel-horizontal'),
        'title' => t('Horizontal'),
      ),
      'vertical' => array(
        'class' => array('jcarousel', 'jcarousel-vertical'),
        'title' => t('Veritcal'),
      ),
    ),
  );
  return $skins;
}
?>
moonray’s picture

moonray commented
Title: Can't add files in sites/all/libraries » Can't attach files from sites/all/libraries to a skin
Status: Active » Needs review
StatusFileSize
new patch_commit_02b9f28fc4f3.patch1.37 KB

And this is what the patch would look like.

jacine’s picture

jacine
she/her
Location New York
commented

Hey, I'm not sure... I could be wrong, but don't think that using libraries_get_path() returns an absolute URL. I think it works like drupal_get_path(), so it would give you 'sites/all/libraries/lib/whatever.js'. Though, I guess that's what the TRUE argument is for in your example?

Anyway, I'm not sure if that's the best way to go... The main concern I'd have is that it's be different for no good reason from everything else. For example, when you use hook_css/js_alter(), you'd expect the keys of all those files to be relative, so if in some module or theme you wanted to swap out a version or something you'd likely get a false positive result. Another example is that sometimes you might want to aggregate files differently based on whether it's external and you might check that it's external based on it starting with http.

I'm not sure what to do about this issue honestly, but I'd lean toward not assuming the paths are relative and leaving it to the person writing the skin config to figure out. This is after all how we do it in themes in template.php after all. It's up to you though. :)

moonray’s picture

moonray commented
Status: Needs review » Closed (duplicate)

Since there was a bug around this issue I wrote the above patch into it, with some additions. See #1342698: Notice in _skinr_add_path_to_files when attached CSS or JS file has advanced settings.
If an absolute path or one relative to root is set, we're not going to mess with it at all. If the path is relative, we first check for file_exists() in the skin's path, if it doesn't, we leave it alone as well. This way libraries_get_path('jcarousel') . '/lib/jquery.jcarousel.min.js' will simply work.
The check for the file's existence only happens on clearing cache for the site, so it's not that expensive.

I'm marking this as duplicate of the other issues since that's where it's been fixed.