The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Using entities for storage of skin configuration will let drupal core handle most of the caching. In addition, it just seems a much cleaner approach.
A good reason to do this now is that we need to break up the current skin configuration object. Instead of one object that contains an array of all skins and their options that are configured for a page element, we will now have a separate object (and db entry) for each skin. This allows us to do things like disable all skin configurations for skins (from the plugin, not user configured) that are disabled by the user, allowing us to take the checking out of our preprocess function in a follow-up patch. It gives us much more granular control.
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | patch_commit_46db4ba3743b.patch | 108.55 KB | moonray |
| #13 | patch_commit_6b4d49140df0.patch | 99.89 KB | moonray |
Comments
Comment #1
moonray commentedUgh, so I lost my whole post. Here we go again.
I'm trying something different. With drupal having moved to git it'll be easier to review these code updates (2 commits) by merging them into your local setup. Checkout skinr from d.o's git repo. Then merge in the changes from the dev-1050472 branch of git@github.com:moonray/skinr.git (see https://github.com/moonray/skinr/tree/dev-1050472).
The commits to look at are 50e7aaee595da5afe0e7692ac3f338835caa5187 (see https://github.com/moonray/skinr/commit/50e7aaee595da5afe0e7692ac3f33883...) and a02d0c81a9ebfcb621215a5b10dca54a6be8c94e (see https://github.com/moonray/skinr/commit/a02d0c81a9ebfcb621215a5b10dca54a...)
The first commit basically just renames every instance of sid to element. Having that included in a patch will make it hard to review, hence the separation.
The second commit applies the above functionality. It also updates the admin skin configuration overview page to account for the additional DB fields: sid (primary key), skin and status; skins was renamed to options. The old DBs were dropped from the module's schema.
Tests are included (I finally figured them out!).
EDIT: I forgot to mention that I've ignored the panels and views module plugins. As soon as #1044222: Remove skin configuration functionality from third-party forms goes in that code won't need it anymore and it seemed a waste of time.
EDIT 2: I also didn't mention that you'll need a fresh drupal/skinr installation due to the DB changes.
Comment #2
moonray commentedComment #3
moonray commentedAfter going over this with some people I've come to the conclusion that using entities will add too much unneeded overhead. Going with a more direct approach where we load data straight from db instead of going through entities.
See commit 7a2012ced3108bc9dd0b1f1452a56e36c5bbcd53 (I've created a new branch just for this issue; should have done that in the first place).
Comment #4
moonray commentedComment #5
coltranemoonray, can you describe the usage of the skinr_skins table columns for me? I'm unclear of the usage and necessity of module and element in particular. If a module defines a skin are you storing the module name in the skinr_skins table for its skins to avoid parsing through all modules? Is the element field being used to store what page element a skin has been applied to? If so, how does that relate to rules?
* sid
* theme
* module
* element
* skin
* options
* status
Comment #6
moonray commentedsid: a unique, auto-incremental id
theme: the theme these skin settings apply to
module: the module that implements the element these skin settings apply to (this is needed by Skinr's functionality plugins to generate a unique element id)
element: this is the element that these skin settings apply to. In the case of blocks it is [module implementing block]_[delta]. This id is generated by the functionality plugins and ensures each element (blocks, regions, panel panes, etc.) all have a unique identifier.
skin: the skin name whom's settings are applied (available skins are provided by the skin plugins)
options: which options are selected by the user (available skins are provided by the skin plugins)
status: enabled or disabled (whether this setting is applied to the appropriate element on the front end, or not).
The module and element are not the ones that implemented the skin plugin, but the ones implementing the element we're trying to skin on the page.
Rules, at present, only apply to pages (the body tag) and regions. If we're skinning a region, for instance, the module (in the above skins ettings table) would read 'rules', and element the rid (rule id).
Comment #7
coltraneBut 'rules' in this case is not a module, it's a part of Skinr module, correct? Is rules a special case of the module storage in skinr_skins?
Comment #8
moonray commentedIt is indeed a special case, and so far the only exception.
EDIT: If/Once we get the new rules api figured out, everything would wind up being a rule. What is now in 'module' would sit in the rule's table.
Comment #9
coltraneThanks for the explanation moonray, I was going to say that instead of it being 'rules' then would it not be better to refer to the module as 'skinr' - but if it's going to be stored differently anyways I'm ok ignoring.
btw, I'm commenting inline on https://github.com/moonray/skinr/commit/7a2012ced3108bc9dd0b1f1452a56e36... and will followup with another comment here in a bit.
Comment #10
coltranePending moonray's responses to my inline comments on https://github.com/moonray/skinr/commit/7a2012ced3108bc9dd0b1f1452a56e36... I think the method is sound.
I get failures in the API tests of testSkinrImplements() and testSkinrSkinInfo() however. I'll look into the tests next.
Comment #11
moonray commentedReplied to comments, and it looks like some minor work is required. Don't forget, there's also this commit to look at: https://github.com/moonray/skinr/commit/a02d0c81a9ebfcb621215a5b10dca54a...
Comment #12
moonray commentedI've updated db_select to db_query wherever possible.
See the full diff at https://github.com/moonray/skinr/pull/2/files
Comment #13
moonray commentedAnd here is a regular patch of everything above.
Comment #14
branana commentedThe 'name' column is taken out of skinr_skins, but there are still a few references to it.
"DOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'name' in 'where clause': SELECT 1 FROM {skinr_skins} WHERE name = :name AND skin = :skin LIMIT 0, 1; Array ( [:name] => skinr_test [:skin] => skinr_test_example ) in skinr_ui_admin_skin_infos_settings_submit() (line 935 of /Users/bran/Sites/reports/sites/all/modules/skinr/skinr_ui.admin.inc)."
^ in skinr_ui.admin.inc for example
Comment #15
moonray commentedAh, looks like I overlooked that because all that stuff is being replaced in the follow-up patch at #1050472: Override skin status per theme. But it's probably a good idea to keep head error-free. So marking as needs work.
Comment #16
coltranemoonray, I'm confused now on what to review. Your pull request to merge into master from dev-1082842 contains changes I'm not seeing in the head of dev-1082842. Neither of the github branches for #1082842 or #1050472 match some of the code in that pull request, unless I'm reading github wrong?
Comment #17
moonray commented@coltrane: You must be reading github wrong. The pull request lists all commits, which are also listed as commits for branch dev-1082842. The pull request just combines all the commits that aren't yet merged into the master branch.
Comment #18
moonray commentedI'm coming back on my point to mark this as needs work. HEAD (or master now) is already broken for the following admin sections: admin/appearance/skinr/skins (submit button doesn't work), admin/appearance/skinr/skins/settings/* (submit button doesn't work).
This patch doesn't break thing much worse, and considering there's a patch that takes care of these issues, I don't think we should put time into that right now.
So, what that means is: ignore the submit buttons on the above parts of the site for your tests. :)
Comment #19
nomonstersinme commentedThis patch breaks my site giving me the follow error:
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 's.sid' in 'field list': SELECT s.sid AS sid FROM {skinr_skins} s WHERE (theme = :db_condition_placeholder_0) AND (module = :db_condition_placeholder_1) AND (element IN (:db_condition_placeholder_2)) AND (status = :db_condition_placeholder_3) ; Array ( [:db_condition_placeholder_0] => orange [:db_condition_placeholder_1] => block [:db_condition_placeholder_2] => system-powered-by [:db_condition_placeholder_3] => 1 ) in skinr_skin_get_sids() (line 756 of /Users/admin/Sites/drupal7/sites/all/modules/skinr/skinr.module).Comment #20
moonray commentedDid you uninstall your old skinr module before enabling the new one?
The whole db schema has changed, and we haven't written any of the upgrade code yet.
Comment #21
nomonstersinme commentedi didn't have skinr enabled before. its a fresh drupal/db. i'll try again anyway.
Comment #22
moonray commentedAdded tests to ensure skins are applied to front-end, and CSS and JS files are properly included.
Fixed a bug where it CSS and JS files weren't properly included.
Optimized the patch: require the preprocess index handler for functionality plugins to return an array to prevent using is_array, which is slow.
This additional commit is at: https://github.com/moonray/skinr/commit/f2ce3fbf0529924e310d17df4ad64ae5...
The full patch is (still) located at: https://github.com/moonray/skinr/pull/2/files
Also attached the patch the regular way.
Comment #23
vrajak@gmail.com commentedDid a fresh local DB install then installed D7 and then patched skinr with this patch and enabled it. All seems to work fine so far but I haven't done extensive testing. Unless you install a new DB or remove/uninstall skinr first it will error on you.
Comment #24
Hari commentedInstalled on a fresh install of Drupal and had no issues. Skins are rendering properly.
Comment #25
moonray commentedI've gotten some feedback with so far no errors, so I'm going to assume this is RTBC. This patch is holding everything up and needs to be committed.
If any problems pop up we'll file additional patches.
The commit is at http://drupalcode.org/project/skinr.git/commit/8604bc1