Write load and parse code for Skinr include files in PHP format

Nope, hook_library() is for registering libraries that are shipped with a module. A module that exposes a skin may use hook_library() and attach that to a skin, but Skinr does and must not.

Skinr will solely use http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_pr...

I think we could stop calling the custom classes "additional." It was named that way since the template support was also there, but that's gone, so this should be named more appropriately. In our mockups for this, we've got this in an "Advanced" tab. Not sure whether that helps or not, but just thought I'd mention it.

$skin->skins definitely needs to change to $skin->options as I've expressed elsewhere.

Other than that, I don't have much to contribute to this conversation at this point.

Regarding #1, what's wrong with just using "id"? Since it's a skin object property there's no namespace conflict.

Regarding #2, I think skin import/export is a critical ability so I'm definitely in favor of a unique machine name that enables that.

Regarding #3, I'm unclear on the use case for this being in the Skinr module if it doesn't use it. Couldn't other modules just extend the class if they needed to add values for their own purposes?

I agree with @sociotech on #1 and #3.

As for #2, I'm hoping we can get feedback from @sun, @ericduran or someone else more familiar with exportables. I don't see how having this defined would hurt anything though, so I'd lean toward doing it.

Regarding CTools, I thought we decided against using it for a two reasons:

1. The only benefit the CTools module would provide is the loading plugin files.
2. We'd need to horribly complicate the syntax we've agreed upon, but making users explicitly define hooks.

I'm all for using CTools if it makes sense, but it doesn't appear to in this case. We need to keep skin creation as easy as possible.

1. Odd. Until now, I imagined that a sid would be an auto-incremented integer; i.e., an entirely internal database ID. Looks like it's actually a html_id...?
2. Exporting/importing skin configurations requires to do not store the configuration in an auto-incremented primary key. As long as that is the case (it sounds like it is), it does not matter whether the primary key consists of 3 columns or just one.
3. We don't need separate options and settings. We should use a single settings column, since 1) that is consistent with some schemas in core, and 2) clarifies that settings is not identical to the 'options' definition of the skin info hook. settings will only contain the configured key/value pairs of the skin options, not the options themselves.
4. Additionally,
   

   $skin->options; // Applied skins; this is an array of skin options keyed by skin name (e.g. array('skin_name' => array('option1', 'option3'))).
   

   we should consider to "extract" (normalize) individual skins into separate table rows; i.e., $skin->name (ex. skin_name) should be a separate schema column and part of the primary key.

5. Furthermore,
   

   $skin->module; // Shared primary key: module the element belongs to (e.g. panels).
   

   is an ambiguous column name. Normally, module refers to the module that exposed a configuration item in the first place. In Skinr, there are two kind of modules related to a configured skin. I'd suggest the following:

   $skin->module: The module (hm, or theme) that defined/registered the skin via hook_skinr_skin_info() (e.g., "example_skins").
   $skin->element_module: The module the element belongs to (e.g., "panels").
   $skin->element_id: The element ID the skin applies to (e.g., "display-1-pane-3" or "display-1-region-center").
   

Why 4 and 5, you ask? Because modules that are providing skins can be installed and uninstalled. If a module is uninstalled, then we want to i) remove all skin configurations for skins the module exposed, and ii) also remove all skin configurations that affect elements belonging to the module.

Normalizing the skin configurations into separate table rows per skin helps to maintain that data, and also, to properly export/import skin configurations. (Speaking in "features", you should be able to build one feature module that only configures typography across the site, as well as additional features that may touch the same elements but configure other styling aspects.)

EDIT: To summarize:

$skin = new stdClass();
$skin->theme: PK: The theme name the skin applies to (e.g. Bartik).
$skin->module: PK: The module (hm, or theme) that defined/registered the skin via hook_skinr_skin_info() (e.g., "example_skins").
$skin->name: PK: The skin's name, as registered via hook_skinr_skin_info() (e.g., "font_family").
$skin->element_module: The module the element belongs to (e.g., "panels").
$skin->element_id: The element ID the skin applies to (e.g., "display-1-pane-3" or "display-1-region-center").
$skin->settings: A (serialized) array of skin options (e.g., array('option1' => 'blue', 'option2' => 'red')).

Here's the patch for a test skin.

Err, here it is with the right CSS file path.

Ok, changing the hook based on recent discussions.

It would help if I returned the $skins. :)

A modified version of #13 has been committed: http://drupal.org/cvs?commit=461574

Now we need to write an actual test. Assumptions from @sun:

1. The info hook is found + invoked.
2. We get a skinr_test_font skin.
3. It appears to be configurable.
4. It appears in a 'typography' group.
5. There are two options.
6. Saving/applying the skin leads to skinr_test.css being loaded.
7. The skin should only appear for blocks and regions
8. The block or region gets the class of the selected option

$skin->source_type;
$skin->source_name;

If $skin->source_name is unique on the install is $skin->source_type necessary? At the point the object exists we only care about applying the skin, and whether it was defined by a module or a theme doesn't change that application, right?

Ok, I am totally lost with where to even begin to write a test for this stuff. I could continue fumbling, but it's very frustrating and probably a waste of time when I could be fixing other issues, so I am going to stop now.

If $skin->source_name is unique on the install is $skin->source_type necessary? At the point the object exists we only care about applying the skin, and whether it was defined by a module or a theme doesn't change that application, right?

I would assume that $skin->source including the module or theme name would be enough here, but I'm not sure.

Hi, I'm trying to test this. I have the following in my grids.module file:

/**
 * Implements hook_skinr_api().
 */
function grids_skinr_api() {
  return array('api', 2.0);
}

/**
 * Implements hook_skinr_include_directory().
 */
function grids_skinr_include_directory($type) {
  return $type; // This is what the instructions in the api docs say to do.
}

My directory is setup as follows:

grids/grids.info
grids/grids.module
grids/skins/ninesixty
grids/skins/ninesixty/ninesixty.inc

+  $directories = array();
+  foreach (module_implements('skinr_include_directory') as $module) {
+    // Check if the module implements the correct api version.

This is code is not listing my directory and my skins aren't loading. Am I missing something? The status is manually set to enable them, etc.

Also, I get that we don't really need a version for hook_skinr_api(), but we might, so can't we just define our path to the skins directory inside of hook_skinr_api()?

Powered by Dreditor.

I just tried pasting the same code into skinr itself, and it loaded, so there's definitely a problem with loading from modules at least.

So, I've discussed this whole hook_foo_api() approach a bit in IRC.

Direct problem space being: For Skinr, we should fundamentally differ between:

1. Modules integrating with Skinr
2. Extensions providing skins

The current code basically applies the hook_skinr_api() concept to both, but for 1), it may not be necessary, because if we're going to do major changes to Skinr's module integration API, then this will likely mean Skinr 3.x.

So for module integration hooks, we may use the regular module system. To advance on that a bit, Drupal 7 provides two new mechanisms to deal with this kind of situation:

• hook_hook_info() allows to register so called "hook groups", which in turn allow (other) modules to place their Skinr integration code into $module.skinr.inc by registering the appropriate hooks.
• If there's a hard dependency (i.e. required dependency) on Skinr, then modules are able to specify a specific, compatible version for it in their .info files:
  

  dependencies[] = skinr (=2.x)
  

The other issue is hook_skinr_api() for skins. For that, you need to know that Drupal 7 core uses advanced mechanisms to play nicely with module hooks:

• Hook implementations of other modules are individually prepared and cached.
• As mentioned above, hook implementations may be "lazy-loaded" on demand if a group has been specified.

However, the existing hook_foo_api() concepts are effectively destroying all of those built-in core facilities, because we invoke a single hook (which may involve lazy-loading already), and only later on we check the returned 'api' version in order to decide whether we actually want or need it, and since by that time, the code has been potentially loaded, we cannot use well-known/standard module system functions anymore, as their primary condition for invoking something is whether a function does exist or not. That's stupid.

What we really want is to leverage the built-in facilities in D7 core. Therefore, the basic idea would be to

1. move the API version from the returned 'api' array property into the hook name itself.
2. i.e., hook_skinr_api_2()
3. this allows us to easily (lazy-)load all compatible implementations in a simple way:
   

   foreach (module_implements('skinr_api_2') as $module) {
     $module_skins = module_invoke($module, 'skinr_skin_info');
   }
   

4. hence, if skinr_api_2 would be registered via hook_hook_info(), then module_implements('skinr_api_2') would automatically lazy-load the right + proper module include files containing compatible code for the requested Skinr version 2.
5. it further allows code to be compatible with multiple Skinr API versions.

Please note that these are very fundamental considerations that have been triggered by reviewing the patch. It's possible that we want to use hook_skinr_api_2() to identify compatible module integration code, but also to retrieve filesystem paths to scan for skins.

However, given that IRC discussion, I'm pretty sure that a hook_skinr_api() (without version number) doesn't make sense anymore in times of D7.

sun's example:

foreach (module_implements('skinr_api_2') as $module) {
  $module_skins = module_invoke($module, 'skinr_skin_info');
}

does imply non-plugin implementations of skin definitions, and seems like a regression.

hook_skinr_api_2() seems great for satisfying fundamental #1, integration with Skinr.

To allow for plugins skinr_load_includes() could check for an (optional) include directory defined in the skinr_api_2 hook and otherwise load $module.skinr.inc and look for hook_plugin_skinr_info().

Actually as I look into hook_hook_info() more I understand that module_implements() would load the include file, where skins would be defined, and we wouldn't need to load it ourselves. We would, however, need to come up with another way of providing plugins.

I've been running the patch for since #20 for about a week and all appears to be working well. I've also tested the top patch in #29 and it's working fine for themes. Nice job! :D

Regarding the API stuff, I'm fine with hook_skinr_api_2(), but I'd much prefer the top patch. Making themes include this information in the .info file doesn't feel right at all, and I'd like to avoid it if we can.

Here's some minor issues with the patch (mostly comments) that need work:

+++ skinr.module
@@ -421,38 +422,105 @@ function skinr_rule_visible($rid) {
+ * Load include files for skinr implemented by all modules.

Skinr should be capitalized.

+++ skinr.module
@@ -421,38 +422,105 @@ function skinr_rule_visible($rid) {
+ *   The type of a plugin; can be 'skins' or 'modules'.

Since this is such a point of confusion for people, we should briefly describe what each one is.

+++ skinr.module
@@ -421,38 +422,105 @@ function skinr_rule_visible($rid) {
+ * Return a list of directories by modules implementing hook_skinr_include_directory().

This is over 80 chars.

+++ skinr.module
@@ -421,38 +422,105 @@ function skinr_rule_visible($rid) {
+ *   An array containing module names suffixed with '_' and their defined
+ *   directory.

The return includes themes and modules, so it needs to be updated. Also, is this "_" suffix happening? I don't see it, but maybe I am missing something.

+++ skinr.module
@@ -421,38 +422,105 @@ function skinr_rule_visible($rid) {
+  // Load directories for modules.
+  foreach (module_implements('skinr_api_2') as $module) {
+    // Load the path where these plugins can be found.
+    $result = module_invoke($module, 'skinr_api_2', $plugintype);
+    if (isset($result) && is_string($result)) {
+      $directories['module'][$module] = drupal_get_path('module', $module) . '/' . $result;
+    }
+  }
+
+  // Load directories for themes.
+  foreach (list_themes() as $theme) {
+    if (!empty($theme->status) && !empty($theme->info['skinr'][$plugintype])) {
+      // Load the path where these plugins can be found.
+      $result = $theme->info['skinr'][$plugintype];
+      if (isset($result) && is_string($result)) {
+        $directories['theme'][$theme->name] = drupal_get_path('theme', $theme->name) . '/' . $result;

This "load" business isn't really accurate, right? It's just "looking" for module implementations of Skinr, so it can store the paths which will be used to load the files in skinr_load_includes(). Right?

+++ skinr.module
@@ -881,19 +931,21 @@ function skinr_get_skin_info() {
+          $skin_infos = array_merge_recursive($skin_infos, $result);

Please change $skin_infos to $skin_info.

+++ skinr.module
@@ -919,22 +971,24 @@ function skinr_get_group_info() {
+          $group_infos = array_merge_recursive($group_infos, $result);

And $group_info here.

Powered by Dreditor.

I really, really don't like $infos (anywhere), as I've mentioned previously in other issues as well. While it may be grammatically "correct," it is extremely uncommon to use the plural and because of that it looks downright illiterate. I've never been asked to provide my "Billing Informations" during checkout. Information and info can be both singular and plural interchangeably. There are a gazillion hooks in Drupal core with info in their names, and none of them refer to any of the contents as infos. None of them. In fact there isn't even one occurrence of "infos" anywhere in Drupal at all.

And from your description, and the description of what the function is doing, it seems like it could easily just be called $skins and $groups, with info dropped all together.

On further thought I'm not sure the benefit of hook_skinr_api_2() when compared to having to support another integration style for themes is worth it. I think an installation running Skinr is unlikely to have a significant amount of old skins laying around such that there would be a performance problem around calling old skinr_api functions just to check API compatibility. Unless there's a core solution that also handles themes?

@moonray, does your patch in #20 account for themes?

@coltrane The patch in #29 works with themes, but #20 does not.

looks like you're making good progress, Im soon going to be able to do test 10 themes that I'm updating simultaneously so I'll get some feedback in this queue. Thanks for all the effort so far.

Here's the approach of #20 (using hook_skinr_api()) with the skins directory being an option of the api implementation.

As I was working on this I wondered why $filter is an argument of

 function skinr_load_includes($type = 'skins', $filter = NULL) 

Are there cases where Skinr include files will not be of the format *.skinr.inc?

Here's that patch for #36.

Regarding #36, I was not understanding the distinction between module plugins and skin plugins, and that module plugins have a file include format of *.skinr.inc and skin plugins of *.inc. I've reopened #956932: Determine optimal skin include file format but after looking more at skinr.module it may just be necessary to refer to them both as plugins but prefaced with either "module" or "skin".

Bah, I don't know how we'd have one method in Skinr that could return skin plugins from a module *and* a theme's template.php. We could write our own active theme template.php parsing, something like theme_implements(). Or, move all integration with Skinr into *.skinr.inc files and use drupal_system_listing() to retrieve a list of files and parse.

Previous patches in this issue loaded Skinr include files using skinr_load_includes(). Skinr includes could be of two types:

1. module plugins, which provide module-specific functionality integration with Skinr
2. skins

These were defined differently, *.skinr.inc and skins/*.inc, respectively, but were loaded in the same function (skinr_load_includes()) and was quite confusing. [Part of the confusion came from the limited differentiation in the code between a module plugin and a skin plugin (further distinction is needed).]

There were recommendations on taking advantage (and following cues) from core by using include methods and lazy-loading plugins. However, there is desire to continue to allow skin plugins in themes, which is not possible using core's standard include file method module_implements().

So, I've tested and attached a method of writing Skinr plugins using specific file formats. This copies the existing *.skinr.inc formats for module plugins and applies it for skin plugin use.

The benefit of this method for all Skinr plugins is that

1. Skins can exist in modules and themes, and likely elsewhere in the filesystem (sites/all/skins)
2. Skinr doesn't have to figure out what type of include file it is (module or skin)

This method may add more processing overhead, I haven't benchmarked it.

To test this with your skins, define a plugin.skinr.inc file in a module or theme with the following hooks, swapping "plugin" with the name of your skin plugin:

function plugin_skinr_api() {
  return array('api' => '2.0');
}

function plugin_test_skinr_skin_info() {
  // current skin_info format array
}

plugin.skinr.inc could of course be under a skins directory in your module or theme.

This needs much testing, as I've only gotten stuff to load, not yet apply. And I'm considering this method to be in the RFC stage, so please comment.

Yeah, patch in #40 does $name . '_skinr_skin_info' where name is the plugin name from [plugin].skinr.inc of the filename. So it needs some more thought to allow it to work for plugin entity groupings ... (there are far too many names for things in Skinr :P).

Handling no skins defined so the tests pass in this patch.

Instead of including a plugin grouping in the function definition of an implementation of _skinr_skin_info() could skin authors not define a new skin file?

So before you'd have a skins directory and a .inc file containing two _skinr_skin_info() functions:

• zen_grids_skinr_skin_info()
• zen_styles_skinr_skin_info()

What I propose, along with this patch, is that you create

• grids.skinr.inc with function grids_skinr_skin_info()
• styles.skinr.inc with function styles_skinr_skin_info()

Or, we could consider allowing hook_skinr_api() to define plugin entity groups or something.

What works good for you?

I spoke with sun at good length about progressing with module and theme Skinr plugins using a hook-like method, as opposed to the standalone Skinr include files in #43.

The attached patch follows his recommendations of using hook_hook_info() and builds on what moonray wrote in #29. skinr_get_directories() does API and *.skinr.inc checking instead of theme integration in .info files.

I also altered the plugins array (built in skinr_get_directories() and skinr_load_includes()) because the level of separation of "source type" (module or theme) seemed unnecessary to me and complicated the code far too much.

However, this patch is not exactly tight either. Because I wanted to support plugins under sub-directories like 'modules' (within a module - such as Skinr itself) and 'skins', the plugins array in skinr_get_plugins() (what was previously skinr_load_includes()) has a level for 'name', the prefix on *.skinr.inc files in a sub-directory of a Skinr skin or module plugin.

That seems a bit confusing, but I believe it allows for a scenario like the following:

A theme, mytheme, can define, at its root level, a mytheme.skinr.inc file, which implements hook_skinr_api_2():

function mytheme_skinr_api_2() {
  return array('directory' => 'skins');
}

And the sub-directory skins of the theme mytheme can contain multiple *.skinr.inc files, each implementing hook_skinr_skin_info():

• grids.skinr.inc with function grids_skinr_skin_info()
• styles.skinr.inc with function styles_skinr_skin_info()

You'll see that this removes the "entity grouping" pattern in the _skinr_skin_info() definitions.

I have this working for skin plugins in a module and a theme but I haven't exhaustively tested this of course. I'm unsure of the ramifications of altering the plugin array keys as it pertains to skinr_skin_info_process(), though I did update a few.

How do you test this?

1. alter your skin plugins to be a file called myplugin.skinr.inc
2. place it in the root of a module or theme
3. implement hook_skinr_api_2 in this file
3. this file can also implement hook_skinr_skin_info and define your skin array
- or, you can create a new *.skinr.inc file with this hook under a 'skins' directory of your module or theme
- but, you'll need to specify the 'skins' directory in your API hook of myplugin.skinr.inc

You think I'd have learned the first time. "Handling no skins defined so the tests pass in this patch."

I just attempted to test this, and I'm not able to get it working. I have both module and theme skins running on a site I'm working on and can't get either to work. Here's what I did:

Theme = Sky

1. Created a file called sky.skinr.inc

in the root of my theme.

2. Added the following code to it:
   

   function sky_skinr_api_2() {
     return array('directory' => 'skins');
   }

3. Renamed my skin file from default.inc to sky/skins/default.skinr.inc.
4. After that didn't work, I also tried putting all the code in sky.skinr.inc

Module = Grids

1. Tried changing the name to grids/skins/ninesixty.skinr.inc and adding the hook_skinr_api_2() to grids.module
2. Tried changing the name to grids/grids.skinr.inc and adding the hook_skinr_api_2() to that same file. This caused me to keep an empty .module file just to keep the module enabled. Hopefully this isn't something we are shooting for here.

None of it worked. Any ideas what I'm doing wrong?

Big note for anyone testing: Make sure you back up before testing these patches.

Thanks for the feedback moonray!

The whole point of that comment was to point out that using drupal_static was pointless here. Use static.

I didn't realize that, maybe it could be said in the comment more explicitly? I'll revert it back to using static if I write the next patch.

As for the skinr_hook_info() function... I believe you're using it wrong. The point of that function is to load *.skinr.inc whenever the function hook_skinr_api_2() is called. However, that is the function that defines the include path, and thus should be put in the module's main file... also, this only lazy loads *.skinr.inc files in the root of the module, leaving out lazy loading for most of the plugins which will be in subfolders.

You're right, though I think it is being used correctly in the patch. I didn't, however, explain what is happening and why. So allow me to try and provide that here.

Yes, by using hook_hook_info() a module, and module only, must place it's implementation in its .module file or in a *.skinr.inc in the root of the module's directory. That was understood and intended to be taken advantage of by defining skinr_hook_info().

However, I've seen that there is a desire to allow for filesystem grouping of skin definitions into sub-directories, like '/skins'. In an effort to continue to allow for that, and to avoid different handling for Skinr include file formats, the patch in #47 moves any Skinr integration plugin (whether it's a module or skin plugin) into *.skinr.inc files (though Skinr integration hooks can be implemented in a .module file as well).

However, that is the function that defines the include path, and thus should be put in the module's main file

The 'directory' key of implementations of hook_skinr_api_2() is not required and is only used when the plugin author wishes to place plugin files in a sub-directory. If that is not so then the skinr_skin_info() hooks should be implemented inside the *.skinr.inc file.

Also, I believe it will only load MODULENAME.skinr.inc which is again less than ideal.

hook_hook_info() does not load *.skinr.inc from themes, that's correct. A difference between #47 and #29 is that #47 alters your theme->info to allow for *.skinr.inc file implementations in themes. So the Skinr skin definitions and integration need not differ if the skin is being integrated from a module or from a theme. Does that make sense?

Just to recap: what was your objection to the patch in #29? It doesn't seem you've added much that improves functionality since then.

I have little objection to the patch in #29 and started with it for #45. I believe the only thing I've actually added is implementing hook_hook_info(). Otherwise the difference is:

• allowing themes and modules to implement Skinr plugins in the exact same way
• loading module and skin plugins the same way
• making an effort, in my opinion, to make the code more concise and explanatory

#47 needs work for reasons that I understood from Jacine after she tested it out. The patch needs to handle non unique plugin names such that, for example, themes Zen and Fusion can each provide a skin plugin called 'grids'.

stepping back

On a side, I want to step back and talk about this issue and the Skinr code in general.

I'm not entirely happy with the approach using hook_hook_info(). I think it's a clever way to do lazy-loading, it follows cues and practices from core, and it allows for .module file separation which is a big goal for Skinr. Those are the good points. (I don't think we need lazy-loading.) The bad points happen when hook_hook_info() meets the other goals of Skinr, particularly with Skinr wanting to provide theme-level skin definitions.

To document, here's what I understand to be some of the main goals for Skinr plugins:

• provide module plugins which are functional handlers
• supporting module plugins in a sub-directory (like Skinr does with '/modules')
• provide skin plugins
• supporting skin plugins in a sub-directory (/skins)
• allow multiple (non-unique) skin definitions for a single skin plugin
• allowing skin plugins to come from a module or a theme
• optionally allow standalone skin plugins
• optionally allow skin inheritance

The other method than hook_hook_info() could be called the "atomic include file" method, which is in #40 and #43. That method is a standalone plugin include file that IMO bastardizes the hook paradigm, but works and satisfies most of the goals without introducing a ton of code. I'm also not happy with the approach :).

sun makes the strong argument, from a best-practices theory approach, that plugins should ultimately be hooks (if I am correctly summarizing my IRC conversation with him). *.skinr.inc files are an abstraction on .module hooks achieved using hook_hook_info() and Skinr plugins in themes should also be hooks, thus requiring us to invoke them. On its own, and in theory, that seems great, but it's once we try to achieve that and meet the goals of Skinr that it gets hairy and ugly and difficult. And I'm done writing for now, sorry for the book of a comment :).

This is not an easy issue to read through. As a matter of fact is a very hard one :-). I currently have no opinion, but I seem to like coltrane approach because I like the themes and modules declaring their includes files in the same manner. I didn't test either patches lol so right now I'm pretty useless. I need sleep now, hopefully I'll be more helpful another day.

#47 vs #29 which one is faster/less overhead? Differences in how module or theme implement a skin/plugin does not seem like a deal breaker - nice if they are the same but certainly not critical.

#29 is working for me nicely. Building a skin in my theme was very strait forward and using the info file to declare the directory (I assume this is what skinr[skins] = skins is doing) seems very intuitive.

I don't think we need to have an API version check, no. hook_skinr_include_directory() is fine if we don't have an API hook.

Using .info files does mean less overhead but it means Skinr plugin authors have to use a different method to get their skins found. Could we use hook_skinr_include_directory() for themes in template.php?

We can't use hook_skinr_include_directory() in themes, as far as I know. I'm pretty sure we tried this before (in #20) and these types of hooks wont fire in the theme layer.

I completely agree with Jeff Burnz in #29.

It seems to me that we are jumping through hoops to try and treat the module and skin plugins the same, when they are just not the same at all. The only thing they actually have in common is that their files need to be loaded. It's quite frustrating that we can't seem to find a direction and just get it done.

We can't use hook_skinr_include_directory() in themes, as far as I know. I'm pretty sure we tried this before (in #20) and these types of hooks wont fire in the theme layer.

I know we can't use it like module-level hooks, but is even the approach in #47 too late? #47 is basically a theme_implements().

In #30 Jacine said "Making themes include this information in the .info file doesn't feel right at all, and I'd like to avoid it if we can."

Is it worth whatever performance cost there is for us to search out implementations of a "hook" in a theme to satisfy that?

General

I still don't like the wed between hooks and file names used for hook_plugin_skinr_skin_info(). It's complexity that puts Skinr further outside standard practices and it requires plugin authors to be sure their filename matches the skinr_skin_info function definition. One more thing to document. I'm open to discussing alternatives if this can be put on the dissection table.

The patch in #60 works.

I'm ok with #60 (after the particulars, below) but I think there are still some fundamental conflicts between the goals and the ideals. I believe sun would argue strongly for the ideals. I'm not him though and don't share that opinion to the depth he does, so I won't hold up #60.

Particulars

+++ skinr.module
@@ -421,38 +422,123 @@ function skinr_rule_visible($rid) {
+    // this function indicates API version 1. API version 2 would require
+    // hook_skinr_api_2() to be implemented.

If there's nothing else in the module about hook_skinr_api_2 I suggest removing the last sentence since it could add confusion. "Wait, what is that hook?"

+++ skinr.module
@@ -421,38 +422,123 @@ function skinr_rule_visible($rid) {
+        $file_list[$source] = drupal_system_listing("/{$filter}.inc\$/", $path, 'name', 0);

I think this needs the period in '.inc' escaped, "/{$filter}\.inc\$/" and technically $filter would need it's period escaped as well.

+++ skinr.module
@@ -421,38 +422,123 @@ function skinr_rule_visible($rid) {
+ * Return a list of directories by modules and themes implementing hook_skinr_api_2().

"implementing hook_skinr_api_2()" -> hook_skinr_api()

+++ skinr.module
@@ -421,38 +422,123 @@ function skinr_rule_visible($rid) {
+ *   An array containing a 'module' and/or 'theme' key. Eah of those is

"Each"

+++ skinr.module
@@ -421,38 +422,123 @@ function skinr_rule_visible($rid) {
+ * @see skinr_load_plugins()
+ * @see hook_skinr_api_2()

"hook_skinr_api"

I also think all the "infos" variables should become "info".

I know we can't use it like module-level hooks, but is even the approach in #47 too late? #47 is basically a theme_implements().

I'm not against something like this. I just don't fully understand the implications of it. If this is something that will work for us, then I'd like to hear from both @moonray and @sun on whether or not that will work out. It seems to me though, that it probably wouldn't work out because a theme would need to be enabled for the hook to be found? What about the use case where a subtheme needs to inherit from a base theme that isn't enabled? I know this seems like an edge case, but it's something that happens all the time and we need to make sure we're able to deal with it. Maybe there wouldn't be a problem here. I really don't know, but it is one of the things that concerns me.

In #30 Jacine said "Making themes include this information in the .info file doesn't feel right at all, and I'd like to avoid it if we can."

Yeah, I was referring to this line specifically:
skinr[api] = 2.0

Is it worth whatever performance cost there is for us to search out implementations of a "hook" in a theme to satisfy that?

I don't know the answer to this. Of course, I'd like Skinr to perform the best it possibly can, but I don't know what the performance implications are. All I can really go on are the comments that are going back and forth in this thread.

I still don't like the wed between hooks and file names used for hook_plugin_skinr_skin_info(). It's complexity that puts Skinr further outside standard practices and it requires plugin authors to be sure their filename matches the skinr_skin_info function definition. One more thing to document. I'm open to discussing alternatives if this can be put on the dissection table.

Can you elaborate on this? I'm sorry if I seem like a moron, but I really would like to make sure I understand your concerns. I don't see what the big deal is with naming the include file to match the function name. I guess I'm just used to doing that though. :)

The approach in #47 for themes wasn't technically a hook being called, it was a plain function call. It searched through enabled themes and looked if THEME_skinr_api_2 was defined and if so invoked it. I hear that at the time the code executed template.php of the enabled theme wouldn't be included, but I'm pretty sure I tested with an enabled theme and it did work. I think it's worth further testing.

To elaborate on what I don't like about hook_plugin_skinr_skin_info(), it's the scheme of using "hook" and "plugin". To my knowledge, nothing else in Drupal does what Skinr is doing, which is looking for files called "plugin".inc in a module or theme, concatenating that module or theme's name with the "plugin" name and calling that a hook.

I understand this is the way it's been done, and legacy has weight here, but I think the goal of multiple skin definition plugins per module or theme can be accomplished another way than this.

To provide an extravagant example, let's say my theme's name is super_theme and I want to provide skin definitions in a plugin file called grid_styles.inc. I have to remember that both those names are used in the skinr_skin_info() definition which will be super_theme_grid_styles_skinr_skin_info(). It's not a big deal though. I'm happy to discuss other methods if the point is deemed sound and worth it.

Headache. But should work. But of course, entirely untested. And I forgot to revert/remove the other API docs patch.

Sorry, sloppy me.

Ok, I'm gonna test this now. Can we please take the docs out of this patch though? I want to commit that ASAP, and don't want it counting on this.

Er, or not. I'm not sure how I am supposed to test this.

I am trying with a module. I did:

function grids_skinr_api_2() {
  return array(
    'directory' => 'skins',
  );
}

and grids_skinr_skin_ninesixty_info(). I tried naming the file ninesixty.inc and ninesixty.skinr.inc and I'm not getting anything. :(

EDIT: I don't even get the blank Skinr settings form anymore with this patch applied.

I committed the API docs, so here is #68 without them. I'm still not able to get it to work, but I assume that's because I must be doing something wrong, so I'll leave it on needs review for now.

Thanks for trying to test. Speaking of, this is not something you should even try to do manually. Writing unit tests now.

Now with full-blown tests. Hooray! :)

#73: skinr-HEAD.include.73.patch queued for re-testing.

Hm. Tests are passing for me locally. No idea what's wrong with the testbot.

The tests pass locally, but nothing works. There isn't even a skinr setting form for blocks anymore, which should be showing regardless of whether or not any skins have loaded.

I don't even have a clear idea of how this is supposed to be working right now, so I'm gonna stay with the patch in #29 until there is a consensus here, and it's explained, because honestly I am frustrated as hell with this issue and don't have anything of value to contribute anymore. :(

Sorry about that.

The latest patch incorporates the best ideas and code from this issue (resp. @moonray's #29, @coltrane's #47, and @moonray's #60) and most importantly, the in-depth discussion since #47, but has been restarted from scratch. In concrete terms:

• Implements the hook_skinr_api_VERSION() approach, as outlined in #24.

  I spent some time with thinking through the API compatibility topic in the past weeks. The most important detail for Skinr is that all of the hooks implemented by modules and themes are almost always optional support code - the module/theme still works without Skinr; Skinr merely adds some visual configuration options.

  Especially because we currently have the precedent of entirely revamping Skinr's API right now, we have to take possible future API changes into account. Likewise, already existing Skinr integration code that has been "ported" from Drupal 6 without any changes should no longer be compatible now.

  This means that most of the built-in facilities in Drupal core are mostly useless for Skinr. The only facility that we can leverage is the built-in lazy-loading of $module.skinr.inc files of module_implements() - which only makes sense for third-party modules that optionally integrate with Skinr.

  The _VERSION suffix in the hook name works, but is not set in stone yet and still open for discussion. The positive consequence of it is that simple implementations only need to define the function without any function body or return value. I'm interested to hear arguments (not opinions) for or against the _VERSION suffix.

• Implements a central skinr_implements() function to determine and collect modules and themes (extensions) that are compatible with the current Skinr API.

  This allows us to handle modules and themes consistently. However, I've double-checked the stated argument in the discussion above that we cannot use template.php for Skinr API hook implementations in themes. That is true for a multitude of reasons, which have been documented in the code. Therefore, themes cannot implement hook_skinr_api() in PHP, and need to put that information into the .info file instead.

• Implements hook_skinr_skin_info() and hook_skinr_group_info() with optional support for plugin include files.

  I've tried to explain and clarify a couple of times that the most flexible and most simple API design treats the hook implementations in plugin files identically to the regular hook implementations. We do not need to force third-party modules integrating with Skinr to use plugin files. Likewise, if there would be a hypothetical dedicated 960gs module that only consists of skin integration code, it wouldn't necessarily make sense to use separate plugin files. Modules and themes can choose to use separate/atomic plugin files for their skins, but they are not forced to - the regular hooks work, too.

  As documented in .api.php, the plugin files are named skins/example.inc (whereas the 'skins' directory name is customizable).

• Implements the very first range of actual unit tests for the Skinr API.

  We badly need to get away from stating arbitrary expectations in various issues and testing them manually. Every single expectation needs to live in a test, so ultimately, every single patch against Skinr is automatically tested against all expectations, so we don't break them, unless we intend to.

Hope this helps. Please ask any questions. Let's also try to read the code; I've tried to add explanations where needed.

@todo based on your review/testing:

1. Fix lazy-loading of $module.skinr.inc with custom 'path' definitions (Skinr's own /modules directory) and test that those built-in files are loaded.
2. Add a first simple test/expectation for hook_skinr_config_info() to verify that module integration information is returned.
3. Check what's wrong with the testbot.

I did read the code (and the tests), thoroughly. I do appreciate all the extra comments you made, and that you got rid of all the "infos" references. I am especially confused by a few things:

• Why are we including the hook_skinr_api_x() in skinr_test.skinr.inc and leaving a blank module file? This makes no sense to me. This will happen once per module, right? Is this the only way it's going to work? If I have a module with a couple of plugins, is this hook going to be randomly located one of the .inc files?
• What does this mean for the file naming conventions? I see that skinr_test.skinr.inc wasn't touched and you've added the example.inc and I don't see anything that refers to an "example" plugin.
• Since hook_skinr_skin_PLUGIN_info() an hook_skinr_skin_info() how does this affect being able to alter skin implementations? Are we going to need to alter hook_skinr_skin_PLUGIN_info() implementations separate from from the base hook?
• I don't think the API version makes any sense in skin plugins whatsoever especially in themes, but I could care less about it at this point.

Everything you noted above sounds fine to me.

The one thing that would really help me, so that I could at least write the documentation is how these skins are supposed to be implemented in modules and themes with the latest round of patches. What I tried in #70 didn't work, and I am still not sure if my attempts were correct, even after reading all the docs and your last reply.

This is my understanding:

Modules

1. Implement hook_skinr_api_VERSION() in .module file. What I don't know is:
   • If the api information is only useful for functionality plugins, what should this look like for my skin if I am using the default "skins" directory?
   • What's the deal with $module.skinr.inc. can/should that be used for skins as well?
2. Include skins/$plugin_name.inc which implements hook_skinr_skin_info() or hook_skinr_skin_PLUGIN_info()

Themes

1. Add the following to .info:
   
   

   skinr[api] = 2.0
   skinr[skins] = skins

2. Include skins/$plugin_name.inc which implements hook_skinr_skin_info() or hook_skinr_skin_PLUGIN_info()

Attached patch fixes 1 and 2, including tests. Patch size increased a bit, since I had to move the skinr.skinr.inc file.

Why are we including the hook_skinr_api_x() in skinr_test.skinr.inc and leaving a blank module file? This makes no sense to me. This will happen once per module, right? Is this the only way it's going to work? If I have a module with a couple of plugins, is this hook going to be randomly located one of the .inc files?

We have various different Skinr integration scenarios:

1. A third-party module (for example, Block) that only optionally integrates with Skinr to expose configurable/"skinable" elements that it outputs.

   Such a module likely wants to put the Skinr integration code into a separate $module.skinr.inc file, which is only loaded when Skinr is actually installed. On sites where Skinr does not exist, this code is dead/unused and never loaded.

   Whether such a module will also expose and implement skins depends on the module author and whether that makes sense in the first place. In case it also implements skins, it likely depends on the amount/number of skins

   • whether the module will directly put hook_skinr_skin_info() into $module.skinr.inc (i.e., no need for a separate/additional include file if there is only one default skin),
   • or whether the module will use separate plugin files for multiple skins, and thus, implement hook_skinr_skin_PLUGIN_info() in each plugin file (instead of or in addition to hook_skinr_skin_info()).
2. A third-party module that depends on Skinr.

   Such a module has all of the previous options, but may rightfully want to put all of the hook implementations into the primary .module itself, because it requires Skinr module anyway to work and style the module's output.

   Thus, it has all remotely possible options, ranging from putting everything into the .module file, putting everything into $module.skinr.inc, and may also put individual skins into plugin files.

3. A module that exposes/implements skins (and groups, FWIW).

   Such a module will most likely not implement hook_skinr_config_info() to integrate any output elements with Skinr, but merely register skins and groups. But anyway, it has all of the aforementioned options.

4. A theme that exposes/implements skins and groups.

   Themes have all options too, but depending on the theme engine, there might not be a template.php file (or any other PHP code by default), so the choices a theme has are mainly limited by the theme itself. But as of now, I think we expect themes to only use skin plugin files anyway.

skinr_test.module exists to verify that code that has been placed in either $module.skinr.inc or skin plugin files (or both), and is correctly found and loaded by Skinr. Only because we want to test that, skinr_test.module itself is empty (for now).

What does this mean for the file naming conventions? I see that skinr_test.skinr.inc wasn't touched and you've added the example.inc and I don't see anything that refers to an "example" plugin.

• $extension.skinr.inc may contain hook_skinr_skin_info().
• A skin plugin file, e.g., skins/example.inc, must contain hook_skinr_skin_example_info() in order to be detected. Each existing plugin file is loaded automatically. If hook_skinr_skin_PLUGIN_info() is found after loading, it is invoked and therefore, the skin gets registered.

Since hook_skinr_skin_PLUGIN_info() an hook_skinr_skin_info() how does this affect being able to alter skin implementations? Are we going to need to alter hook_skinr_skin_PLUGIN_info() implementations separate from from the base hook?

Alter hooks are one time operations. Like everywhere else in Drupal, we first collect all information, and then invoke an alter hook to allow extensions to manipulate all of the collected information. If you want, the very rough flow is:

1. Invoke hook_skinr_skin_info(), collect return values
2. Optionally invoke any hook_skinr_skin_PLUGIN_info() that exist, add return values to what we have
3. Lastly, invoke hook_skinr_skin_info_alter() to allow manipulations on everything

Note: This flow/pattern is identical for everything else (and not limited to Skinr).

I don't think the API version makes any sense in skin plugins whatsoever especially in themes, but I could care less about it at this point.

Especially for skin plugin files the API version is highly important, since those may or may not will be distributed through other means (skin repositories?) in the long run. That said, perhaps we need to put API version information into plugin files at some point - but we can deal with that later.

Implementation examples:
(As mentioned above, these are just one of many possibilities)

Module

1. Implement hook_skinr_api_VERSION() into the .module file:
   

   function mymodule_skinr_api_2() {
     return array(
       'directory' => 'skins',
     );
   }
   

   Modules do not default to a 'skins' directory, because we cannot know whether some other contributed module might already use that directory for something else.

2. Implement hook_skinr_skin_PLUGIN_info() in skins/funky.inc:
   

   function mymodule_skinr_skin_funky_info() {
     $skins['mymodule_funky'] = array(
       ...
     );
     return $skins;
   }
   

Theme

1. Add the equivalent of hook_skinr_api_VERSION() to the theme's .info file:
   

   skinr[api] = 2
   

   Themes default to a 'skins' directory, because we want to provide sane defaults.

2. Implement hook_skinr_skin_PLUGIN_info() in skins/awesome.inc:
   

   function mytheme_skinr_skin_awesome_info() {
     $skins['mytheme_awesome'] = array(
       ...
     );
     return $skins;
   }
   

Final note: The run-time functionality of Skinr (i.e., not administrative Skinr UI functionality) is most likely still incompatible with the plugin file system. We need to rethink and revise all of the other module code in separate steps.

@Jacine committed the test changes, so let's try whether the testbot passes now.

Posted testbot problem to #689990-76: Contrib projects to be included in beta stage of automated testing for modules

@moonray: The skinr.skinr.inc file has been copied into the module's root folder. This change and some other test file additions have already been committed, in order to check whether the testbot might have a problem with those file changes. In short: The file still exists, but is located in the root folder now. The patch merely removes the orphan copy from the modules/ folder.

#92: skinr-HEAD.include.91.patch queued for re-testing.

well, ok, let's run tests locally then, before each and every commit.

I'm very happy to say in comment #100 that this has been committed! Yay :D

Thanks so much @moonray, @sun and @coltrane for all your help with this!

@sun didn't recurse on purpose, but I think we need this. Is there a performance concern with this?

This is something we need to discuss. I don't really want to recurse.

If the goal is to have people create their skin plugins in the structure of

skins/example/example.css
skins/example/example.inc

instead of

skins/example/example.css
skins/example.inc

then we should enforce this structure consistently on all skin plugins; i.e., the latter structure would not work.

But first of all, let's remove the additional debug code that has been added to figure out testbot failures since #80.

Committed.

The first example is the goal. I don't mind enforcing that consistently.

This should normally work, but doesn't. Looks like we need to recurse.

Here we go.

And... @Jacine, you did not run the tests before committing #105. Two bogus changes in there, reverted in #108, making tests pass again for me.

Sorry. I didn't. It looked innocent enough. I'm testing the other one now.

Ok, tested it manually, ran tests locally, and committed. Thank you @sun! :D

Always start with tests...

@Jacine: It looks like you forgot to

cvs rm tests/skins/example.inc
cvs add tests/skins/example/example.inc

EDIT: Also looks like the old wasn't removed, according to http://drupal.org/cvs?commit=481328

Added actual (debug) data to the test output.

Merged in the actual fixes.

Committed patch in #116, hopefully correctly this time: http://drupal.org/cvs?commit=482342 Thanks :)

I know this is marked fixed, but I'd still like to report that I tested creating a skin using the current format, @sun's documentation notes in #83, and by having a look at the skinr_test module. I tested with the module approach without using a skins sub-directory and it went pretty smoothly. The module works and the class was applied to the test block I setup. It was pretty easy. :-)

I also tested having the skin include a javascript file and that worked nicely as well. The only thing I couldn't figure out (and this is because I'm not a dev and don't have a clue about php arrays) was the format for adding multiple css or javascript files. Here is what I tried:

    'attached' => array(
      'js' => array(
        'test.file.1.js', 'test.file.2.js'),
    ),

    'attached' => array(
      'js' => array(
        'js' => 'test.file.1.js',
        'js' => 'test.file.2.js'),
    ),

Neither of those approaches worked and I assume it needs to be more like the second approach, but don't know what should go in place of the "js" in the array. Either that or one of those should work and something isn't right with loading multiple files?

Thanks everyone for getting through this issue, nice work!!

@ChrisBryant: Can you file a separate bug report for that multiple JS file issue, please? Thanks!

Done, thanks:

#1028948: Skins don't appear to be loading multiple javascript (and possibly CSS) files