Override skin status per theme

I think it's important to be able to disable skins, as we had it before, due to the same reason you mention. Disabling the module or theme providing it wouldn't be sufficient.

Ideally access to enabling and disabling skins should also be able to be controlled by a specific permission for it.

Agreed.

I would like to stop allowing edits of all enabled themes at the same time, but that's really a separate issue.

So, what do we need to do to fix this?

Ideally access to enabling and disabling skins should also be able to be controlled by a specific permission for it.

We already have different permissions for configuring skin settings vs. administering skinr. Anything else would be overkill IMO.

@jacine, makes sense and fine with me. :-)

x-posting:

set the status of a skin (from plugin info, not an applied skin) has nothing to do with the ability to apply skins

I understand. However, manual administration/management of skin statuses is a much larger topic, which we need to discuss in detail in a separate issue. Based on the current code, it seems like that information was previously stored in the database. Technically, this information rather belongs into a system variable — unless we are going to revamp the entire handling of skin information to be stored in the database, too (i.e., a {skinr_info} table that'd be similar to the {system} table, tracking what exists, information and API versions, status, etc); but as of now, I don't really see a need for this yet, and TBH, if you ever dealt with bug reports and issues related to that {system} table behavior, you'd be similarly hesitant of forking/using that pattern. In the end, there's a lot to discuss with regard to manual configuration of skin statuses, and most likely also plenty of expectations (which might have changed?).

In order to satisfy the order you provide moonray would the user-applied status override default status in the skin object? Otherwise there needs to be another property that is checked when applying.

Here's the start of this, it allows the admin/appearance/skinr/skins screen to save Skin statuses to skinr_skinsets table, and correctly loads the saved status with a new skinr.module function skinr_get_skin_statuses().

The next step, assuming this approach is correct, would be to invoke skinr_get_skin_statuses() in skinr_preprocess() I think. But I'm not sure what skinr_skin_extract() is about and where the best place to check if the skin is enabled.

Also, I've seen discussion about the table skinr_skinsets. It seems the agreement is to handle the renaming of the tables (if at all) in another issue.

The next step, assuming this approach is correct, would be to invoke skinr_get_skin_statuses() in skinr_preprocess() I think.

skinr_preprocess() is the heart and performance-critical part of the runtime logic. In there, we only deal with applying configured skins, and nothing else. If a skin has been configured, and later on disabled, then we will still apply it. The administrative UI needs to disallow disabling of skins for which configuration exists.

@coltrane - I started to build out this patch as well. This is a hard project to jump into! And this seems like a natural starting point. The "enabled" checkbox in the admin UI is a manual override to the skin info's "default status". That default status itself has a default --- disabled --- which was chosen to encourage site administers to only enable the skins they intend to use, to avoid end-user UI clutter. Right now, unless someone is aware of this "default status" property, they'll never get to the point of fully registering a new skin.

skinr_preprocess() is the heart and performance-critical part of the runtime logic. In there, we only deal with applying configured skins, and nothing else. If a skin has been configured, and later on disabled, then we will still apply it.

That makes sense to me, but where should this go? What function is responsible for listing out applicable skins to the end (administrative) user?

Technically, this information rather belongs into a system variable — unless we are going to revamp the entire handling of skin information to be stored in the database, too (i.e., a {skinr_info} table that'd be similar to the {system} table, tracking what exists, information and API versions, status, etc); but as of now, I don't really see a need for this yet

System variables are only name/value pairs, and the values are json encrypted. It would not allow us to write a query to gather a list of enabled skins, for example -- unless that is exactly the data stored in the json-encrypted object. What benefits would come from using the variables table?

Either way, moving from {skinr_skinsets} to {variable} for storage seems fairly easy at this point. As for the bigger consideration that would require lots of discussion, using a more robust structure like {system}: is there a need for storing multiple skinr data formats (i.e., from different API versions) in the same table? Unless there's a strong case for that, it'd be great to move forward with this patch as-is.

Hey Everyone,

I definitely understand the need for granularity. Hashing out some ideas on paper, let me put this out there.

Sun mentioned here http://drupal.org/node/1029058#comment-4014070
The need for "one theme, one theme hook, and one skin".

Moonray posted a suggestion here http://drupal.org/node/1050472#comment-4048414
Stating the need for a default status, per theme status, and then user status.

Moonray also gave an example of a situation where many skins may become available but they might not be relevant
http://drupal.org/node/1050472#comment-4044918

So why not have a skin install settings menu (see attached)
This could provide a default enable skinset and enable skin option per theme. And even a default setting for new
themes. This would essentially control the default toggles for these urls
admin/build/skinr/skins
admin/build/skinr/skins/settings/tables

Now if a theme or module creates a skin that it NEEDS but the system is set to NOT ENABLE it by default, send a system warning saying this module needs it to be enabled.

Lastly, for situations where there is a conflict. Perhaps a skinset (particularly in a theme or module) defines a new default tag so that if one has both the skinset and the skins enabled by default for a theme, that only one or one set of the initial skins are enabled per toggled theme.

Thoughts?

@rickman, I'm not seeing how that's better/different than the current UI, except that the current one lives within admin/appearance

Patch works well!

Minor issues:

1. skinr_ui_skin_status_set() is defined in skinr_ui.module but the menu definition loads skinr_ui.admin.inc, so I think the definition could be moved over to it

Questions:

2. There's little UI change when you enable/disable a Skin's configuration status, perhaps a drupal_set_message() in skinr_ui_skin_status_set() would be good?

3. Is this patch/functionality suppose to touch the appearance of the widget settings form? For example, with a Skin's element disabled on admin/appearance/skinr the form for it still appears (with it's default value set) and when saved it re-enables the element. Is that suppose to be? If not that's fine, if so I'd like to talk about the UX of it. A status of disabled (IMO) implies the form wouldn't be available.

4. The skin configuration enable/disable URL is susceptible to a CSRF attack, though there's little value for an attacker to exploit it. Does it seem worth it to you to tokenize those URLs?

5. Additionally, the enable/disable URL could be one menu router definition, admin/appearance/skinr/toggle/%skinr_skin which would just flip the status of the requested skin. Seem worth it?

Attached patch does items (1), (2), and (5).

#20 was wrong patch actually. This patch implements items 1, 2, and 5 of what's discussed in #20

Regarding tokenizing the URLs, I think it'd be worth it. It would be strange to only be able to toggle these settings via bulk updating. Having the "actions" listed next to each skin seems to be standard UI practice anyway.

Still works great, leaving as CNW for the tests though per #26

One change I'd recommend is the text on the confirmation form when you disable skin sets for an entire theme, I found it pretty confusing. I propose changing

to

Attached patch with the change, or here is the relevant code (note that $themes = list_themes(); can be removed from later in the fn)

function skinr_ui_admin_skin_info_settings($form, &$form_state, $skin_info) {
  $themes = list_themes();
  if (isset($form_state['storage']['disable_sids'])) {
    foreach ($form_state['storage']['themes'] as $theme) {
      $items[] = $themes[$theme]->info['name'];
    }
    $message = t('Would you like to disable the skin configurations for the selected themes?') . theme('item_list', array('items' => $items));
    // Insert a confirmation form.
    return confirm_form(
      $form,
      t('Disable all skin configurations for selected themes?'),
      'admin/appearance/skinr/skins',
      $message,
      t('Yes'),
      t('No')
    );
  }

subscribe

Can someone please tell a newbie how to apply a .patch?

I've got the problem discussed in this tread but don't know how to fix it.

I did a bit of googling and found http://drupal.org/node/60108 which may have the needed information.

But I get an access denied on that page. Why?

@CharlieCastle Here's an example of how to apply a patch: http://data.agaric.com/how-apply-patch-drupal-module