Posted by moonray on August 30, 2010 at 8:39pm
13 followers
| Project: | Skinr |
| Version: | 7.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
This will solve several existing issues:
• #855602: Allowing modules to contains skins
• #761422: Skinsets not working inside theme directories.
Comments
#1
Moving all data into a .skinr file will accomplish several things.
1. You can now have skins attached to modules without causing conflict with its .info files.
2. You can move the skins from the module layer to the theme layer without changing any information; just move the files.
3. Having extra skins in the theme layer will now no longer have those show up as themes on the themes admin page.
4. In addition, people complaining about the large size of .info files should be pleased with this change.
#2
This patch changes the following:
• Filename to enter skinr data into changed to *.skinr
• Patch for issue #855602: Allowing modules to contains skins is included
Documentation needs updating:
• All *.skinr files need to be formatted the same as stand-alone skins (previously in a .info file).
• If a module wants to include *.skinr files, it needs to include the following (skins => TRUE, being the part related to adding *.skinr files to a module):
function hook_skinr_api() {return array(
'api' => 1,
'path' => drupal_get_path('module', 'modulename'),
'skins' => TRUE,
);
}
All modules and themes that want to make use of Skinr 2.x need to be updated.
#3
This patch needs to be rerolled against the latest head.
I'm honestly not 100% on moving the info to .skinr file but I haven't came up with a better implementation. So I guess *.skinr files works :) . (I'll see if I can think of something)
Regarding the patch I can't really test it since it's not applying correctly :-/
#4
I don't like the idea of .skinr at all, unless it's going to be PHP and should probably be .skinr.inc. I'd rather go straight to PHP files, as I've mentioned elsewhere in this queue. I was told this is too drastic a change for this version, but I think that changing this change is drastic too, and would rather not bother with this because it's only a temporary solution. I'd rather get the problem fixed at the source.
Also, I thought one of the problems with getting Skinr to do exportables right had something to do with way the system names of the skins were saved?
#5
Yeah, I too agree that it might be too of a drastic change for this release. But that's not really my place to comment ;-)
But I definitely like the idea of .skinr.inc over .skinr files. Is just such a hassle to have another set of file extension :-/
Also I'm curious is we're just cheating by changing from .info file to a different extension. It seems like there should be a better implementation? No? (I might be wrong as I'm honestly not familiar with issues other people are having with the .info file, just thinking out loud :-) )
#6
@eric, they are completely inflexible. We went with them in the beginning because they are easy for themers to setup. However, as Skinr has become more popular and features have been added, they aren't exactly simple anymore.
In addition, they are really hard to extend and maintain. When you have a skin that has 20+ options, like a font face picker or something, it gets insane hard to do something as simple as adding ONE option, when you want the options in a certain order. Something as simple as keying the options is a tedious task, that is REALLY error prone. In PHP this stuff is actually easier to deal with.
I want to be able to use PHP and in the next version, have an option to create entire skin definitions in the UI. It's impossible with .info files, and I think just introducing the new extension is completely sidestepping the actual problems we are facing. I think using PHP is the answer, so until then I don't see the point of introducing something that is a PITA for people to switch to, and will not end up being a solution in the near future.
#7
Well, I guess we have 2 options at this point:
1) Create a release right now, and ignore critical issue #855602: Allowing modules to contains skins
or
2) Delay release and try and get a PHP implementation of the .info file
If we go for (2, we'll need to figure out an array or class structure to use for the skinr settings.
Which do you think we should go with? Keep in mind that implementing the PHP version of .info files will probably take some additional time. One good thing is that this patch already addresses some of the issues we'll run into when trying to switch to the PHP format.
#8
For reference, here's the current object after it's been imported from the .info file:
<?php
$info = array (
'name' => 'Acquia Slate',
'description' => '<a href="http://www.acquia.com">Acquia</a> Slate is a bright and colorful grid-based theme with point-and-click theming. Requires the <a href="http://drupal.org/project/fusion">Fusion Core</a> base theme and the <a href="http://drupal.org/project/skinr">Skinr</a> module. Created by <a href="http://www.topnotchthemes.com">TopNotchThemes</a> and <a href="http://fusiondrupalthemes.com">Fusion Drupal Themes</a>.',
'base theme' => 'fusion_core',
'core' => '6.x',
'version' => '6.x-3.0-beta1',
'skinr' =>
array (
'options' =>
array (
'inherit_skins' => 'true',
),
'grid12-width' => '; Acquia Slate background styles',
'slate-background-styles' =>
array (
'title' => 'Acquia Slate - Background Styles',
'type' => 'radios',
'features' =>
array (
0 => 'block',
1 => 'panels_display',
2 => 'panels_pane',
3 => 'panels_panel',
4 => 'views_view',
),
'options' =>
array (
1 =>
array (
'label' => 'Dark transparent background',
'class' => 'dark-transparent-background',
),
),
'scripts' =>
array (
0 => 'js/jquery.testtest.js',
),
),
'slate-title-styles' =>
array (
'title' => 'Acquia Slate - Title Styles',
'type' => 'radios',
'features' =>
array (
0 => 'block',
1 => 'panels_display',
2 => 'panels_pane',
3 => 'panels_panel',
4 => 'views_view',
),
'options' =>
array (
1 =>
array (
'label' => 'White title with bold first word and capital letters',
'class' => 'title-white-bold-first',
),
2 =>
array (
'label' => 'Dual color bold title and capital letters with arrow',
'class' => 'title-dual-color-arrow title-dual',
),
3 =>
array (
'label' => 'Yellow title with bold and capital letters',
'class' => 'title-yellow-bold',
),
),
),
'slate-rounded-styles' =>
array (
'title' => 'Acquia Slate - Rounded Corner Styles',
'type' => 'radios',
'features' =>
array (
0 => 'block',
1 => 'panels_display',
2 => 'panels_pane',
3 => 'panels_panel',
4 => 'views_view',
),
'options' =>
array (
1 =>
array (
'label' => 'Rounded corners with gray background',
'class' => 'rounded-corners-gray-background',
),
2 =>
array (
'label' => 'Rounded corners with gradient background and border',
'class' => 'rounded-corners-gradient-background',
),
),
),
'slate-banner-styles' =>
array (
'title' => 'Acquia Slate - Banner Styles',
'type' => 'radios',
'features' =>
array (
0 => 'page',
),
'options' =>
array (
1 =>
array (
'label' => 'Beach stones',
'class' => 'banner-background-beachstones banner-background',
),
2 =>
array (
'label' => 'City street lights',
'class' => 'banner-background-citystreetlights banner-background',
),
3 =>
array (
'label' => 'Greek heads',
'class' => 'banner-background-greekheads banner-background',
),
4 =>
array (
'label' => 'Seascape',
'class' => 'banner-background-seascape banner-background',
),
5 =>
array (
'label' => 'Windows',
'class' => 'banner-background-windows banner-background',
),
),
),
),
'screenshot' => 'screenshot.png',
'php' => '4.3.5',
);
?>
#9
Hi,
So both solutions would essentially solve the current problem but they're both architectural changes. Are we looking to implement a sustainable solution for 2.x branch? or are we honestly just pushing 2.x out so we can work on a good solution for the 3.x branch with all the correct fixes.
I think switching to a php array would be nicer in the long run but that would required a lot more work. Switching to .skinr would work but it seems weird.
Also, I'll like to point out that the theme listing page is a drupal form which we can easily exclude the skins with a hook_form_FORM_ID_alter or we can probably get rid of the skins info file using hook_system_info_alter (this is just a guess, I don't think it'll work). I don't think these are good solutions but for now it'll let the skins keep the .info file and it'll stop them from showing up in the theme listing pages. :-/
@moonray, all that aside. I still can't test this patch. Is it against the latest head?
#10
I believe head has been patched since I rolled the patch. I'll need to re-roll it when I get a little bit of time.
#11
Just a thought:
Could you treat skinr skin sets as libraries? Then you could use the libraries module for users to install global(used by all themes) skinr sets in sites/all/libraries/skinr/name_of_skin_set or people could store their theme specific skinr sets in sites/sitename/themes/name_of_theme/skinr/name_of_skinr_set/
What do you think?
I like the idea of getting skinr out of the info files and into a file preferably php. This would make it easier to version control your skinr files separate from the theme and the above technique would help increase the adoption of people sharing their skinr sets between themes.
Alternatively you could handle it like the Panels module does with custom plugins/layouts where you specify in your .info in what folder does skinr find it's skin sets in the theme.
Hope that is helpful.
#12
Huh. I was just figuring out how to add a skin to a theme yesterday, and saw this bug today.
+1 on moving to PHP -- the current format is cumbersome...
I propose following conventions I see all over the place:
* automatically load any .skinr.inc files in modules or themes
* define a hook_skinr_info hook, with no arguments, that returns an array of skins
* keep the existing array structure as already defined in the info files (and visible in the "skinr" part of the vardump above) - make porting easy
* to set items in this array that are not individual skins (options, grid12-width, etc) follow the FAPI convention of starting with "#".
So an example might look like:
<?phpfunction cdt_fusion_skinr_info() {
$skinr = array();
$skinr['cdt-box-styles'] = array(
'title' = 'Box Styles',
'type' = 'radios',
'features' = array('block'),
'options' = array(
array(
'label' => 'Blue box',
'class' => 'blue-box',
),
array(
'label' => 'Green box',
'class' => 'green-box',
),
),
);
return $skinr;
}
?>
... then your patch becomes really simple:
<?php...
module_load_all_includes('skinr.inc');
$skinr = module_invoke_all('skinr_info');
...
?>
... and you have a merged array of all of the skinr arrays returned by all the hook_skinr_info functions in all enabled module, in the same format as your skinr object is today (aside from any hashed options -- if multiple hooks define the same, ones, they'll get turned into an array). Then load up the theme one if it exists, and you're done.
If you keep the array structure, you can also help people with the transition -- go ahead and load the data from the info file, send something to the status report that's a var_dump or print_r of the resulting object['skinr'] array, and you can probably easily generate the whole file for them as something to cut and paste...
#13
bumping this to the top so someone with more knowledge can comment on @freelock's comment...
wish i had an opinion on this but its beyond me.
#14
@gmclelland in #11: The libraries module wouldn't really be appropriate for this use case, from what I can tell. Drupal's built-in way of handling .info files (module and themes) is good enough for this. We already have the functionality to allow skins in sites/*/skins.
@freelock in #12: I agree with you that the skinr.inc file loading method and using hook_skinr_info is probably the best way to go here.
#15
I've got a patch almost done here. One BIG thing is that we can't use *.skinr.inc as our php filenames. They're already being used for skinr's plugin system. So I've used *.skin.inc instead.
Any objections (and other suggestions)?
(patch coming soon)
#16
How about just .skinr?
#17
Oh this makes me sad. I liked having a .info file for skins... makes so much sense; just like themes.
Oh well.
#18
And here's that patch.
We still need to add an easy way for people to migrate from .info (info) to .skinr (php) files. If anyone feels like adding a simple migration form to the admin for that, please post a patch.
#19
I guess I might as well add an example .skinr file for reference.
#20
And perhaps we should use a patch that'll apply properly.
#21
Defining skins in .info files won't stop working, right? This is just another option?
I'd prefer *.skin.inc so it's not yet another PHP file extension to deal with in editors :)
#22
We should be doing this in 7.x first.
#23
+++ skinr.module 13 Oct 2010 21:57:19 -0000@@ -405,6 +405,123 @@ function skinr_get_module_apis() {
+function skinr_load_all_info_paths($reset = FALSE) {
+ static $info = array();
1) This function does not to "load" anything, so "load" is technically wrong. It's merely a wrapper around _scan|_system_listing functions, so it should be named that way.
2) Given 1), the static caching does not make sense. The calling code eventually should implement a static cache, if, and only if that makes sense. Bear in mind that any static cache ups the PHP memory.
+++ skinr.module 13 Oct 2010 21:57:19 -0000@@ -405,6 +405,123 @@ function skinr_get_module_apis() {
+ // Find skins in skins folders.
+ $mask = '\.skinr$';
It has been a very bad habit of Drupal to introduce new file extensions. Luckily, we stopped doing that, and so should Skinr.
If .skinr means a PHP include file, then it should simply be named that way: .skinr.inc
+++ skinr.module 13 Oct 2010 21:57:19 -0000@@ -405,6 +405,123 @@ function skinr_get_module_apis() {
+function skinr_load_all_info() {
+ $info = skinr_load_all_info_paths();
...
+ $result = skinr_load_info($skinset->name, $skinset);
...
+function skinr_load_info($skin, $skinset = NULL) {
+ if (is_null($skinset)) {
+ $info = skinr_load_all_info_paths();
This looks like we'd run into various major performance issues, including potential endless recursion.
+++ skinr.module 13 Oct 2010 21:57:19 -0000@@ -405,6 +405,123 @@ function skinr_get_module_apis() {
+ $return = array(); ¶
(and elsewhere) Trailing white-space.
Powered by Dreditor.
#24
Sorry, cross-posted with Jacine.
#25
This immediately made me think of this issue #721782: Performance: How does skinr perform?. ChrisBryant was supposed to look into that, but hasn't yet.
This was my first reaction too. I don't like .skinr, but I was told by @moonray that we cannot use .skinr.inc because it's reserved for Skinr module plugins. Now, I'm hearing that it doesn't matter, and skinr.inc can be used.... It would be good to clarify what's going on with more details about why skinr.inc would be problem if that's the case, because I don't get it.
#26
Changed skinr_load_all_info_paths() to skinr_get_skinr_files() and removed static caching.
Changed plugins to use *.skinr.plugin.inc so we can use *.skinr.inc for skins.
Fixed potential endless loop by polling for valid skinset variable.
Removed trailing whitespace.
Please re-review.
#27
um, how did this patch suddenly bump from 20 KB to ~120 KB ...? Looks like plenty of unrelated changes are contained now.
#28
It's actually due to the rename of *.skinr.inc to *.skinr.plugin.inc ... CVS diff handles that by deleting the file and recreating it.
I can reroll without that stuff and you'd have to manually rename the files.
#29
There are lots of issues being up by this, so I'm going to attempt to reroll it against the 7.x version :/
#30
This is as far as I was able to get. There are quite a few functions that are renamed and completely revamped to the point where I can't make sense of what to do with them.
I think I got most of it, but the skinr_ui_skinssets_form() stuff makes zero sense to me. It also seems like some of it has nothing to do with the purpose of this patch. I didn't even attempt to apply that because I am just lost.
And it's probably just because I did something wrong, but it doesn't work. The $mask in
skinr_get_skinr_files()threw 2,533preg_match()warnings, and I believe I have fixed that, but still,$filesdoesn't find anything. I am testing with your example, in #19 renamed to sites/all/skins/dropdowns/dropdowns.skinr.inc.The main errors I'm getting are:
On every page:
Notice: Trying to get property of non-object in skinr_skin_data() (line 1099 of /Users/jacine/Sites/drupal-7/sites/all/modules/skinr/skinr.module).When I clear the cache (PDO exception totally breaks the site):
Notice: Trying to get property of non-object in skinr_update_files_database() (line 1064 of /Users/jacine/Sites/drupal-7/sites/all/modules/skinr/skinr.module).Notice: Trying to get property of non-object in skinr_update_files_database() (line 1065 of /Users/jacine/Sites/drupal-7/sites/all/modules/skinr/skinr.module).
Notice: Trying to get property of non-object in skinr_update_files_database() (line 1066 of /Users/jacine/Sites/drupal-7/sites/all/modules/skinr/skinr.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'filename' cannot be null: INSERT INTO {skinr_skinsets} (filename, name, info) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => N; ) in skinr_update_files_database() (line 1071 of /Users/jacine/Sites/drupal-7/sites/all/modules/skinr/skinr.module).
#31
#32
Same patch as #30, but doesn't change plugin names to skinr.plugin.inc.
#33
Trying to make sense of this issue. As far as I'm able to understand,
- The current usage of .info files leads to many unforeseen problems, such as "themes" appearing on the Appearance page, the module system mistakenly looking for a corresponding .module file, aso.
- The current format and syntax of .info files is too limited for recently added Skinr functionality. Even simple implementations need a giant list of properties to make them work.
- The idea is to deprecate usage of .info files in favor of PHP info-style hook implementations.
- However, PHP code in the form of "hooks" means that hooks can only be invoked in modules and themes that actually exist in the system. Otherwise, it would be wrong to talk about "hooks".
- When instead considering a loose/relaxed function naming pattern (which reminds of Drupal hooks), then it has to be taken into account that those arbitrary PHP function names can easily clash with already existing function names of installed modules and themes.
- It looks like, even though the .info file format is cumbersome, some people would still like to use it.
- In order to resolve the systematic problems with .info files itself (being mistaken by other subsystems of Drupal core), it has been proposed to rename them to .skinr instead of .info, but keeping the .info-style syntax.
- One option I don't see being mentioned here is whether $thing.info files could be renamed to $thing.skinr.info, so as to make code in Drupal core ignore them. The dot/period in the basename should already be disallowed for module and theme .info files, because the basename is used to build PHP function names (i.e., [basename]_foo()).
- Of course, $thing.skinr.inc should only be used for PHP files. .inc denotes a PHP include file.
- A single hook_skinr_info() surely won't cut it for registering stuff via PHP. It also does not allow to have stuff in atomic and focused include files. However, I see lots of CTools-alike code in Skinr already. CTools already implements a concept of self-contained plugins that live in nicely separated include files. Therefore, if you actively consider to transition from .info files to PHP include files, then it might make most sense to simply add a dependency on CTools and use its plugin API, which has been designed exactly for this purpose: Allowing other modules (Views, Panels, Skinr) to allow third-party modules to put their plugins into separate PHP include files. Skinr could use a "soft-dependency"; i.e., don't depend on ctools, but if it's there, try to load Skinr PHP plugins.
So far, that's my understanding of this issue. It sounds like we're missing an agreement and clear vision on the intended way forward. Instead of rolling and re-rolling patches, I'd highly recommend to discuss and figure out the right way to move forward first. Because, based on what I understand, the existing patches might be useless.
Thoughts?
#34
FYI: Created #944198: Functions that call drupal_system_listing() act on potentially invalid system items
#35
Here's my take on the situation (and I have discussed this all, some here in the queue and some internally, with the exception of the Forms API stuff, before work began on the issue):
.infofiles are problematic because they conflict with other system listings.module = blocksid = menu_block-1
data =
a:2:{
s:10: "navigation";
s:18:"nav nav-mega nav-b";
}
The above is option 9 of the navigation skin. If I decide to change any aspect of that saved data in the .info file Skinr is completely ignorant of it, and I need to manually re-save each block this skin was applied to for it to continue functioning properly. No big deal when you are talking about a block or two, but anything more than that is a huge problem.
skinr[navigation][options][9][label] = Mega Dropdowns - Blueskinr[navigation][options][9][class] = nav nav-mega nav-b
skinr[navigation][options][9][scripts][] = js/dropdowns.js
skinr[navigation][options][9][stylesheets][all][] = css/nav-mega.css
skinr[navigation][options][9][stylesheets][all][] = css/nav-colors.css
.infofiles do not, and never will support things like IE stylsheets and full use ofdrupal_add_css()anddrupal_add_js(). If we move to PHP, we can have full control, i.e. defining the group/weight of a stylesheet, adding an IE conditional stylesheet, etc..infofiles, and I would like to use CTools as a basis of doing this. If I knew how to do this myself, it would have been done already.Given the above, I don't see why anyone would still want to use
.infofiles. Yes, it's PHP, but the format is still very similar. It's really not much harder IMO, and the functionality, consistency and possibilities moving to PHP will open up are well worth the trade-off. The reality is that.infofiles were fine for 1.x, but we are totally abusing them right now, and they are crippling our ability to fix certain bugs and move forward with new features.#36
Most likely not an obvious consequence, so merely making sure that everyone is aware of it:
The current usage of .info files allows to register Skinr plugins that are independent of modules and themes. By moving to a PHP-based plugin system (like CTools), Skinr plugins are bound to a module or theme.
In other words: If you had a dropdown.info previously, then you need something you can tack that thing on; i.e., to get something in the form of HOOK_skinr_PLUGINNAME_info(), whereas HOOK must be replaced with the name of an installed module or theme.
#37
Hmm, so that means skins can't be placed in
sites/all/skins?I don't necessarily mind having skins tied to a module or a theme. I would actually like some consistency between how both are handled.
I'm not really sure how things would work. Like, with inheritance for example... Right now, skins in a base theme can be overridden by skins in a subtheme. This is good. I know the way this works will change, but I don't know how and it's hard to tell what the disadvantages are without seeing how it might look/work.
#38
Well, we can agree on a predetermined HOOK name. Why not use skinr (unless it's for a specific module or theme)?
For themes: garland_skinr_dropdowns_info (where the theme name = garland)
For module: admin_menu_skinr_dropdowns_info (where the module name = admin_menu)
For skins: skinr_skinr_dropdowns_info (where PLUGINNAME = dropdowns)
As for inheritance... the PLUGINNAME would be the key that can be used to determine inheritance. That would make it so ANY skin can inherit from any OTHER skin.
As for Jacine's conclusion to use ONLY php format... I think that's the way to go. Using .info leaves us with a few too many limitations.
#39
The last submitted patch, 897822-32.patch, failed testing.
#40
Awesome! Testing is enabled :D
For skins: skinr_skinr_dropdowns_info (where PLUGINNAME = dropdowns)But, I thought they all had to be tied to a module or theme? Wouldn't they all look all look the same as CTools plugins except for the hook which would be theme or module name? OR, are you telling me that I can still place skins in
sites/all/skinsthis way?Is there anything you could think of that we would loose the ability to do?
#41
I was originally thinking that it would be best to support both formats and let theme implementers choose which they prefer to use based on their level of expertise and understanding. People starting out would likely start with .info files and then move towards the php format as their implementations became more complicated. Part of the reason Skinr is popular is likely because of the easy (relative) to use .info file format.
My current thought is that supporting multiple formats at the code, documentation, and user levels would be time consuming, increase code complexity, and generally not worth the effort. My vote is to use only the php format for that reason. If anyone wants to chime in and say that it will be easy, won't be hard to support and wants to volunteer to help make that happen, then I'm all for it.
If that's the route we all agree on then we should build provide a painless way to migrate. Ideally we could just create a simple tool to convert from the current format to the new one.
We should also make it easy for non developers to create skins or skin sets. Luckily, moving to php should make it easier to provide a UI with the module to create or build skins and generate the php files for them.
If the majority agrees on the above I'll create new feature requests for each of those items.
#42
Yes, a separate issue, but it's something we should consider doing outside of Skinr. Maybe on skinr.org.
This needs more discussion here as it relates to CTools, IMO. I would like to know what it would take to implement this in CTools, as I mentioned above. If there is UI functionality built in to CTools, and I feel like there is, it would make sense to evaluate that now so we don't have to end up making big changes later.
#43
Clarifying the title here, since it wasn't clear to some.
#44
Ok, so this is a very rough draft, I'll continue to work on this but this is all I got for the day. Anyone is welcome to continue working on this. I won't work on this till tomorrow around this same time.
This patch adds the ctools dependency (It needs to be ctools alpha 1.x).
This patch creates a skin plugging, that can be declare in a module or in a theme.
The skinr.skins_.txt file needs to change to skinr.skins.inc (I'm not allow to upload the .inc files).
You need to place the skinr.skins.inc file i've uploaded in the plugins/skins/ directory under the skinr module folder. I don't know how to have cvs do the whole folder structure :-(
I know is not great practice to provide uncompleted patch but I just wanted to log my work somewhere ;-)
So this patch allows skinr to load the skinr.skins.inc file and you can see it listed in the skins listing page, There's a lot of stuff still missing :-/ but is a start.
#45
As discussed above, I think we should rename our current skinr plugins (for functionality) to *.skinr.plugin.inc and use *.skinr.inc for skins (instead of *.skins.inc).
I'm going to take a swing at adopting the code you've written and working it into my already existing patch above (I have a feeling we won't need a lot of re-writing).
#46
Why?
#47
@sun Because it would be more consistent with what other modules (like views) are doing.
But say we go for a different name that the module name (skinr), why plural (skins) instead of singular (skin)?
Jacine mentioned that you think we can have both types (functionality plugins and skin plugins) have the same *.skinr.inc filename. Wouldn't we run into a conflict and need to try and figure out what type of plugin it is?
#48
Trying to port this code I ran into the following big issue: CTools will only let you load plugins attached to modules and themes.
That means that we're going to have to scrap our sites/all/skins idea if we want to use CTools. Considering that D7 lets you easily download and install modules, it might be worth having a module that just calls some CTools crud to be able to have a skin instead of going with the sites/all/skins idea.
Thoughts?
In the meantime I'm moving forward with implementing skins as CTools plugins.
#49
While I'm not thrilled about loosing the ability to store skins in "skins" directories, I pretty much expected this after @sun's comment in #36, and I don't think it's a show stopper, or even a big deal in the scheme of things.
#50
@moonray, We're aware of the issue that ctools only let you define plugins in themes and module, this was mention by @sun before and @jacine said it was ok to scrap the sites/all features.
@moonray, I also don't understand why not just used skinr.skin.inc (plural vs singular doesn't really matter). This is the default implementation when using ctools and it'll be more standard. View is the one module that doesn't do $module.$api.inc but every other modules does do $module.$api.inc
Also regarding changing the handlers I think that should be a separate issue. I think the handlers should also be ctools plugings but that too should be discuss in a separate issue and it shouldn't hold this issue up.
#51
@sun, @ericduran: Seems I was mis-interpreting the way CTools does plugin naming. I was somehow thinking about the way views does things (randomname.views.inc) vs the way CTools does it (skin/randomname.inc where skin = plugin type).
#52
err... fixing title.
#53
Posting what I've got so far. Everything is working (except for the panels plugin), but I feel there is some excess code in there now.
A few things that spring to mind that still need work:
• CTools allows caching; we're double caching to make account for status being stored in db for each skinset.
• Still need to try and implement our skinr functionality plugins as CTools plugins (or do we want to tackle that in a subsequent patch?)
• Skinr's panels plugin is now broken because it has a duplicate skinr_ctools_plugin_directory() function; either we implement a callback in skinr.module's skinr_ctools_plugin_directory() for plugins, or solve it in another way
If anyone else wants to take a stab at it, let us know so we don't duplicate any work.
#54
@moonray, this looks good you beat me to the patch. A couple of things I notice.
While I was working on this I decided not to use the 'key' key and use the actual name of the skin since ctools_get_plugins will return the file name as the key in the array, there's no need for a declared key.
One other thing is the when the database saves the record it saves the key as the name, which is correct. But when is loaded there's a function that sets the key:value pair to what's being return from the database, essentially overwriting the name from the "Name" to the "key". So in the listing instead of seing the name you see the key in the name. This is most likely happening in the skinr_get_files_database . I had the same problem. Hope this makes sense.
In my implementation I got rid of drupal_static as ctools is also doing caching. Also in my implementation I decided to get rid of the info array and have a top level object with all the property as I felt it'll be easier to declare plugins :-/ that was a mistake as I ended having to rewrite a lot of things. Anyways you're patch is a lot better than mine.
So I'll stop working on my patch and start working with you patch.
Nice work :)
#55
While I can understand that we want some progress in terms of code here, I'd highly recommend to flesh out the future design conceptually first.
So far, my understanding is:
sites/all/modules/example/skins/foo.inc, which must contain a corresponding functionexample_skinr_foo_skin_info()(i.e.,HOOK_skinr_FILENAME_skin_info()). The rest of the skin processing and fucntionality is the same as if the skin would have been registered in a $module.skinr.inc file.#56
(I've added numbers for easy reference in follow-up posts)
1) Yes, we're dropping .info support entirely.
2) 3) Considering how simple CTools makes it, I don't think we should bother with an alternate skin format using hook_skinr_skin_info() in a $module.skinr.inc file.
4) Skins would just have to be dropped in their own subfolder in the */modules/skinr/skins (we would need to update the patch for this... the current patch places them in */modules/skinr/plugins/skins) where they are automatically picked up by CTools.
5) Going with a CTools plugin only system seems a lot simpler than trying to support multiple skin file formats. It allows for having your skin plugins in themes. The only thing you need to do (aside form creating the plugin file; see #44 for that) is add
plugins[skinr][skins] = a_sub_directory_in_your_themeto your .info file.What this means, though, is that CTools becomes a (non-optional) dependency for Skinr.
#57
So that sounds like there's still some misunderstanding, which only supports my belief that we're moving too fast here.
That would be a serious regression. A module's directory should only contain the original and unmodified files of a module, and nothing else. Lots of Drupal documentation explicitly directs users to delete a module's/project's directory prior to extracting the new version.
#58
I'm confused by this comment. If the purpose of the module is to house skins, what's the big deal if a user deletes the directory before updating? Wouldn't the skins themselves need to be modified the way any other override would work, .i.e. you should never hack modules or themes directly? I guess I just don't see why this case would/should be any different than any other CTools plugin.
And really, I just don't understand how this works well enough so I really don't have a grasp on how it will differ from what's going on right now. If someone could outline the process in detail, it would help tremendously to understand how the following things would work (with actual examples):
Until I fully understand how to do these things, I have no idea what the implications of this change will be, and cannot really test any patch.
#59
Sorry for the confusion. -- Right, there would not be a difference to any other module: Don't hack any files and do not add or remove any files.
However, the comment:
...sounded like the idea would be that users/themers would "drop" their current skins from .info files (after conversion) into Skinr's module directory, which I can't disagree more with. ;)
Strictly speaking, the only option to add custom skins to your site/theme would be to drop them into your theme or your custom, site-specific module. Those are the only two places that can be used safely.
However, speaking of, I'm relatively confident that we could also "abuse" CTools' plugin system and optionally allow users to put their skins into
sites/[site]/skins-- by tacking them onto Skinr module's namespace.#60
@sun Okay, thanks. It seems we are completely on the same page with that then. Abusing CTools and allowing for a
sites/x/skinsdirectory would be nice, but I don't know how this would work.If the skin needs to be tied to a module or theme, it's kinda confusing to completely separate one from the other, unless there are significant advantages. The module and theme need to be enabled normally, so what if the module resides in
sites/all/skinsand the skins themselves are dropped intosites/[site]/skins. I would assume that Skinr/CTools will need to be able to count on a stable location of the skins in relation to the module for file paths and such. Or maybe this is a non-issue?Anyway, I should probably stay quiet for a bit until I get some examples here. :P
#61
Ok everyone here's a new patch, that needs some serious reviews.
I'll try to be as clear as possible but I've been fighting an update hook for the past hour so I'm a bit tire.
This patch makes ctools a dependency of skinr. So be sure you have it install. I'm currently running 7.x-alpha-1.
After applying this patch you will need to run update.php. If something fails during update.php that means I did something wrong in the update hook (My apologized, an alternative is to just do a clean install and you should have no problem).
This patch implements a skin ctools plugins which allowed modules and themes to declare skins.
I've also attached a skinr_example.module (is the skinr_example.zip archive) that shows you how to declare skin support in your module. This modules provides and example_skin that changes the color of the text.
There's also a plugins zip attached. This plugins zip belongs inside the skinr directory. This is another skins that's being declare by skinr itself. This skin allows you to change the background color.
Inside the plugins/skins directory you will find a provided_skin.inc file. This is the new skinr info file. I've provided comments on the .inc file to show where everything belongs.
Please be sure to clear your cache, as I believe there's currently a caching issue between skinr and ctools.
All comments welcome :)
Oh I didn't try using a theme, and overwriting it from a child theme. Sorry me = sleepy
I hope the instructions are clear.
#62
I'll change the status to needs review, That way people can review, I didn't test EVERY functionality so I'm sure it'll need work :-)
#63
Oh, almost forgot. Please note that there's an issue saving skinr settings in the overlay #744524: Cannot save Skinr settings when overlay module is enabled, this took me a while to figure out why it wasn't saving. So for testing it might be better to turn the overlay off, as that's a separate issue.
#64
The last submitted patch, skinr_ctools_plugin_patch.patch, failed testing.
#65
@ericduran, this is awesome. I just tried it quickly and it's working! Obviously there are coding style issues here, and this needs a real technical review, but awesome work so far!
Since this is nowhere near the point of being ready for testing, I'm uploading your patch again in .diff format, so the bot will (hopefully) ignore it and we can keep it on "needs review" :)
EDIT: Well, I thought the bot ignored .diff files, but apparently not :(
#66
The last submitted patch, skinr_ctools_plugin_patch.diff, failed testing.
#67
Since Eric's patch includes a skinr_example.module, I'm linking this issue here for reference:
#926740: Include a skinr_example.module with the Skinr module that includes an example skin
Edit: making issue link properly.
#68
I don't think we need to have separate terminology for "skinsets" and "skins" anymore. One of the things I'd like to do as part of this patch is start treating all skins the same way. The concept of "skins" versus "skinsets" is confusing, and I don't see any need to differentiate the two anymore. We need consistency here.
Overview of current vs. proposed functionality
Here's an overview of the current differences between the two and how CTools plugins will change things.
* Indicates this is not currently functional in the latest patch. See Extending and altering skins below.
Key Functionality of Skinsets
What I am trying to illustrate in the above table is that the plugin implementation should take away most, if not all differences between the two as part of this patch. We are not confined to .info files anymore, and I think we need to approach this as if we'd never used .info files in the first place.
The fact is that one of the big advantages of moving to PHP is that we can stop jumping through hoops implementation-wise and make things work more "Drupally" in general. It's also very important that we get this right now, so we don't need to force users to change skin structure again.
Extending and Altering skins (as plugins)
The issue with the current patch is that is doesn't improve how skins are handled. Ideally we'd like to be able to do the following (as I noted in the table):
How we can do this? Using _alter hooks.
We (gravitek) had a long discussion about this yesterday. The result of that discussion takes future goals into account and changes what the implementation would look like, as shown below.
One thing to note is that each skin and group name needs to be unique in order to be able to easily alter them. In the examples below each function, skin and group name is prefixed with the theme_or_module name, followed by the group/skin name.
The implementation will be the same across both modules and themes, and obviously you would prefix with either a module name or a theme name and not both.
Implementation of 1 skin:
<?php
// Define the meta information for the plugin, including its hooks (unless we
// can make skinr determine these automatically).
$plugin = array(
'name' => 'Name of Plugin',
'description' => t('Description of plugin'),
'core' => '7.x',
'version' => 'whatever (optional)',
// define skin hooks.
'skins hook' => 'my_module_or_theme_skin_skins',
'skins hook alter' => 'my_module_or_theme_skin_skins_alter',
);
// Define the skins themselves.
function my_module_or_theme_skin_skins() {
$skins = array();
$skins['my_module_or_theme_skin_a'] = array(
'title' => t('Change Text Color'),
'type' => 'radios',
'features' => array('block', 'panels_pane'),
'stylesheets' => array('all' => array('css/skinr_example.css')),
'options' => array(
array(
'label' => t('Red'),
'class' => 'color-red',
),
array(
'label' => t('Yellow'),
'class' => 'color-yellow',
),
),
);
return $skins;
}
// See how easy I can alter something (D7 only for themes).
function some_other_module_or_theme_skin_skins_alter(&$skins) {
$skins['my_module_or_theme_skin_a']['title'] = 'pwned';
}
?>
Note: No group is defined, so this skin ends up in a default "group" defined by Skinr.
Implementation of multiple skins:
<?php
// Define the meta information for the plugin, including its hooks (unless we
// can make skinr determine these automatically).
$plugin = array(
'name' => 'Name of Plugin',
'description' => t('Description of plugin'),
'core' => '7.x',
'version' => 'whatever (optional)',
// Define skin hooks.
'skins hook' => 'my_module_or_theme_skin_skins',
'skins hook alter' => 'my_module_or_theme_skin_skins_alter',
// Define group hooks.
'groups hook' => 'my_module_or_theme_skin_groups',
'groups hook alter' => 'my_module_or_theme_skin_groups_alter',
);
// Define group(s).
function my_module_or_theme_skin_groups() {
$groups = array();
$groups['my_module_or_theme_group_a'] = array(
'#type' => 'fieldset',
'title' => t('Title of group a'),
'description' => t('This is the description of the group, which is essential a fieldset or vertical tab group.'),
);
$groups['my_module_or_theme_group_b'] = array(
'title' => t('Title of another group b'),
'description' => t('Another description.'),
);
);
return $groups;
}
// Define the skins themselves.
function my_module_or_theme_skin_skins() {
$skins = array();
$skins['my_module_or_theme_skin_a'] = array(
'title' => t('Change Text Color'),
'type' => 'radios',
// Attach this skin to group_a.
'group' => 'my_module_or_theme_group_a',
'features' => array('block', 'panels_pane'),
'stylesheets' => array('all' => array('css/some-file.css')),
'options' => array(
array(
'label' => t('Red'),
'class' => 'color-red',
),
array(
'label' => t('Yellow'),
'class' => 'color-yellow',
),
),
);
$skins['my_module_or_theme_skin_b'] = array(
'title' => t('Change Text Color'),
'type' => 'radios',
// Attach this skin to group_b.
'group' => 'my_module_or_theme_group_b',
'features' => array('block', 'panels_pane'),
'stylesheets' => array('all' => array('css/some-other-file.css')),
'options' => array(
array(
'label' => t('Red'),
'class' => 'color-red',
),
array(
'label' => t('Yellow'),
'class' => 'color-yellow',
),
),
);
return $skins;
}
// See how easy I can alter something (D7 only for themes).
function some_other_module_or_theme_skin_skins_alter(&$skins) {
// I don't like your title.
$skins['my_module_or_theme_skin_a']['title'] = 'pwned';
// What was this person thinking, this belongs in group_b!
$skins['my_module_or_theme_skin_a']['group'] = 'my_module_or_theme_group_b';
}
// See how easy I can alter something (D7 only for themes).
function some_other_module_or_theme_skin_groups_alter(&$skins) {
// This group is much more important the the others, show it at the top!
$skins['my_module_or_theme_group_a']['#weight'] = -10;
// I don't want a fieldset, I want vertical tabs!
$skins['my_module_or_theme_group_a']['#type'] = 'vertical_tabs';
}
?>
Thoughts? Questions?
#69
Thanks for this writeup!
'skins hook alter' => 'my_module_or_theme_skin_skins_alter',Could we not assume that the alter functions will be consistently named based on skins hook ?
#70
Awesome work. Sociotech and I will be working on updating Fusion this week which will give us a chance to really bang on these new concepts.
My one comment is on default status -- I'd like to see the option for skins to be set as either default enabled or disabled. This may plug in to #744706: Add Features support but I think there's a case for both sides here, something super necessary like a layout or basic style option that should always be enabled vs. nice-to-have skins that are fine to have a user manually enable. But I'm worried about the amount of "it's broken!" issues from people who are going to barely manage to install the module/theme in the first place, if they have to then go and enable a bunch of skins (and which ones!).
#71
@ezra-g re:
I sure hope so! I'm not entirely sure though.
@stephthegeek re:
I see no problem adding a "default status" which is "disabled" by default but can be set as "enabled" at the plugin level. I don't want to enable them all by default though, because it's just too much when there are a lot of skins most of the time.
#72
I love this. Great write up Jacine. My only concern was the same as Stephthegeek's but per your previous comment I'm totally fine with having a setting for status, which i can set myself.
#73
I agree with everyone, great writeup.
#74
Now that we're thinking things through and trying to optimize things I've got a problem that may seem trivial, but if solved will lead to improved legibility of our code.
In the current patch out CTools plugin is named "skin". However this "skin" can contain several skins and groups (see jacine's writeup in #68). If we keep using the name "skin" for the plugin, our code will use the variable name skin for 2 different concepts which will make the difficult to read. If we call the plugin "skins" (plural) we run into the issue in code where we need to refer to an array of these "skins" (what's the plural of that?) :-)
Any thoughts or ideas?
#75
re #74: I don't fully understand the problem. :s
#76
I like the plugin name being skins mainly because that's what people are declaring in the plugging (skins) :)
I also like skins as the plugin name because then by default the directory name inside plugins will be skins. Which I think also make sense to plugins creator.
Now I do see the problem moonray mentions which is this
<?php$skins = ctools_get_plugins('skinr', 'skins');
foreach ($skins as $key => $skin) {
// The $skin variable here actually contains multiple "skins' definition.
}
?>
So essentially if we start thinking of skins as a group of "skin" plugins then we can think of each actually skin definitions as a skin_instance which is essentially just a form element.
Then we can do (just an example):
<?php$skins = ctools_get_plugins('skinr', 'skins');
foreach ($skins as $key => $skin) {
foreach ($skin as $skin_instance) {
//$skin_instance is the form element (radio, checkbox, etc...)
//edit: To use Jacine's example you can think of the $skin_instance equal to $skins['my_module_or_theme_skin_a']
}
}
?>
Hope this make sense. I can see how it might still be confusing to have multiple skin_instance inside the skin variable. But this is just my idea, it might not be the greatest idea ;-)
#77
Hmm, $skin_instance is fine. Another option might be: $item, or $widget.
#78
Some key things to provide orentiation:
In general, things are structured in:
It's important to understand that everything is normally tied to some form of a machine name. Initially defined as key in the info hook:
<?phpfunction hook_SOMETHING_info() {
$somethings['foo'] = array(...);
return $somethings;
?>
In turn leading to corresponding callback functions and hooks:
<?phpfunction hook_SOMETHING_foo_form($form, &$form_state) {
}
function hook_SOMETHING_foo_form_validate($form, &$form_state) {
}
function hook_SOMETHING_foo_form_submit($form, &$form_state) {
}
function hook_form_SOMETHING_foo_form_alter(&$form, &$form_state) {
}
function hook_SOMETHING_foo_load($something) {
drupal_add_css($something['css']);
drupal_add_js($something['js']);
}
?>
For example, to understand what CTools is doing, you might want to think of any arbitrary implementation of hook_menu(), which is most common for all modules. hook_menu() is actually a simple info hook; it merely doesn't use the _info() suffix. And so, what you see there is a bunch of registered menu router items, of which all are unique and provide atomic functionality; i.e., they are self-contained and sufficiently describe what exactly should happen when you request the registered path.
Now, if you'd apply CTools' plugin system on hook_menu(), then what it is effectively doing is NOT removing hook_menu(). Modules can still just implement that and be happy. Instead, CTools allows the module that exposes the hook in the first place to state:
In turn, modules may respond with some paths, for example:
sites/all/modules/skinr/menu_router_items/CTools then goes ahead and scans all of those directories for PHP include files, includes them, reads out the meta information, and merges that into the results for all modules. That result is passed back to the module that exposes the hook in the first place, so that module can do whatever it wants to do with the registered information. (To complete the example, the collected items would logically be merged into the exposing module's hook_menu() implementation. But yeah, that module would ensure that each item additionally registers the required information to load the corresponding PHP include file it was found in, whenever the menu path is requested [which is a built-in functionality of the menu system].)
#79
...and re-reading my post leads to the following conclusion as well as action items, which ideally should be approached in the stated order through separate issues (turning this issue into a meta issue):
#80
So, I was just made aware of this issue last night, and wanted to throw my 2 cents in here.
You've discussed a lot of options in regard to where you want to see your skins go, and I want to give an outsider's perspective here a bit. Keep in mind my exposure to skins and skinr in general is rather limited. I have built one or two, but... it's not like I have the process memorized or even know it all that well.
Current discussion seems to center around hook_SOMETHING_info() and potentially ctools plugins (or a cloning of the plugin framework). Let me start here, as ctools is officially a dependency of views in Drupal 7, and this is marked as a 7.x version discussion, I don't see the need to clone something out of ctools and maintain it separately, when there's a VERY good chance that ctools is going to have a rather large install base, and most people won't see that dependency as an issue.
Secondly, I initially like the idea of making skins a plugin, but after discussing what all can be in a skin, it seems painfully obvious to me that skins are much closer to modules than than they are plugins, which gets me to thinking about exportability... which is REALLY what you're wanting here. You want the ability to take various settings, colors, images, tpl files, and more, and package them up just like we do with features for modules now. Ctools has a replacement for color module (hasn't been upgraded to 7.x yet) and color module in D7 got a bit of a boost. In any event ctools can write css files, we've seen the potential of exports in features module, and when I was told a different grid system could be included with a skin, that immediate made me think that skins will need to support a set of plugins of their own. Grid systems, and what else? I don't know, but panel layouts/styles seem obvious, probably various items for views as it transitions to the ctools plugin architecture as well... and all of this leads me to the same conclusion:
Skins are much closer to being modules than they are themes.
Yes I know themes can support ctools plugins, but they don't support exportables of various types, furthermore it's relatively easy to include node/page/etc tpl files in a module. Yes the theme will override them, so skinr compliant themes would actually need to get simpler... i.e. don't provide tpl files in the theme, but instead provide a default skin that contains the default tpl files. You'd utilize the same theme for all skins, and just have a configure option on the theme that picks the skin you want to use. (maybe that's how it works currently... it's been a while)
Ultimately some sort of code generation like features does for skins would be very cool... provide a color picker for a theme with various settings, and then have the ability to save that out to code. Would make skinr+themes really awesome concerning the default skins you could provide since most of the hand coding would go away. My only shyness in all of this is that these would end up showing up in the modules directory but... that seems a very small price to pay for the benefits/payoffs... imo.
Eclipse
#81
Here's the first of the sub-issues:
#954874: Rip out .info file handling from skinr core module
#82
And here are more sub-issues:
#956932: Determine optimal skin include file format
#956990: Clean up and optimize skinset handling code in skinr core
#956994: Write load and parse code for Skinr include files in PHP format
#83
#84
We are still far from done with revamping Skinr's internals yet, but as far as this meta discussion as well as the splitted off issues are concerned, this issue seems to be fixed.
#85
Automatically closed -- issue fixed for 2 weeks with no activity.