Separate permissions for "site information" and setting site theme

This is something that I could use as well, even a module that broke these things apart for permissions would help a lot. As it is now, when I give a client permission to change the 'site info' they are also granted permission to blocks, modules, ect.

This is a very good idea.
A very very good idea.

Just found it - http://drupal.org/project/config_perms

Tagging for the novice queue.

I think this is what the issue is looking for...

Sorry for forgetting the status...

This is a fantastic idea. Sadly over my head at the moment.

Hey, afreeman. What about this is over your head? Maybe we can help. :)

I can has help? I'm having an interesting time figuring out how hook_perm, hook_menu, and system_admin_menu_block_access() interrelate. The new perm array thing seems fairly intuitive. Likewise access arguments aren't anything new, I've worked with them in D6. However if I (for example) add an array item to hook_perm() for 'administer site content' and change the access callback for $items['admin/content'] to array('admin/content','administer site content') the content menu doesn't display. What am I missing?

Nevermind, I think I see what I was doing wrong.

Maybe yes?

Err.

@afreeman: I was looking at your patch and think that I figured out why it kept failing testing. It seems to be because the tests needed to be updated to account for the new permission to "administer site theme." Here's a new version.

Awesome! Thanks Julia! Let's see if this dog hunts.

tagging

The permission title/name should be "Administer themes", in plural, instead of "Administer theme". See, for example, other permissions in system.module: "Administer files", "Administer actions", etc...

Rerolled for #20.
Also changed the description of the permission to.
"Choose the site theme and manage theme settings"
instead of just referring to the theme settings.

Subscribing.

Part of the original bug report is separating the "site info" from the "site configuration" (performance, site maintenance, file system, error reporting, etc..). This isn't included in the patch. It's probably more important from our point of view, as users are more likely to want to change the homepage/footer/mission than they are to change the theme.

needs reroll :S

Here's a re-rolled version that also breaks out site information from site configuration. Those with "Administer site information" can access "admin/settings/site-information."

Here's a re-rolled patch to account for the 404 functionality test. I wasn't sure whether to have the test user have only "administer site information" access or whether they should have "administer site configuration" access as well. Both are included now.

Here's a re-rolled patch.

Not sure why the bot didn't test the patch. Here it is again.

Here it is once again. Thanks Bojhan for the tip about renaming to .patch in IRC.

Patch performs as advertised.

A note to others who review this patch, be sure to clear caches after installing the patch. The menu system needs to be rebuilt in order for the changes to go into effect.

Just wondering but with the new IA, should this permission be called 'administer site appearance' instead?

Good point. Here's an updated patch.

Shawn

bot?

ICK! I used uppercase letters in the permissions string. Bad Shawn!

Let's do this again.

The #36 patch applies cleanly and performs as expected. It also incorporates the IA change to use "administer site appearance" instead of "administer site themes" from #33.

Not quite. We need an upgrade path. http://api.drupal.org/api/function/system_update_7011/7 could be used as a general guide I think.

Wow. This is one tricky novice issue. :) Here's a new patch with my attempt at a system_update function. Please review carefully.

The 3 fails are caused by the following tests in system.install:

    $this->assertText(t('The configuration options have been saved.'), t('Modules status has been updated.'));

    // Check that hook_modules_installed and hook_modules_enabled hooks were invoked and check tables.
    $this->assertText(t('hook_modules_installed fired for aggregator'), t('hook_modules_installed fired.'));
    $this->assertText(t('hook_modules_enabled fired for aggregator'), t('hook_modules_enabled fired.'));

What's interesting is that these fails appear to be independent of what I just added to system_update_7036.

I re-ran the tests locally and found that the 3 errors occur independent of my patch and are all connected to the Aggregator module. Advice?

The testing bot was possessed earlier today: http://testing.drupal.org/node/49

Marking back to needs review.

Here we go again. Re-rolled because previous version of the patch could no longer apply.

Here's a new re-rolled version. Keeping my fingers crossed that the end is in sight.

Julia

Please testing bot. Please find me in the next hour so that someone at our Triangle Drupal Hack Night can help.

There are some kinks to work out yet. Some of these belong on another issue, I'll sort that out in the morning and roll a new patch with some improvements.

* Permission 'Administer site configuration' is labeled 'Configure module settings' which is inaccurate. Not sure which is right yet.
* Warning: call_user_func_array(): First argument is expected to be a valid callback, 'admin_menu_flush_cache' was given in menu_execute_active_handler() (line 418 of /Applications/MAMP/d7/includes/menu.inc)
* The ability to access the page if you know the URL doesn't seem to be protected correctly
* The permission set needed to discover the URL is too heavy
* Site Information is under Reports/Site Configuration rather than the main config tab.

This patch now does what it set out to do (as long as testing passes). It can probably wait until after the lock to be committed, since it's not much of an API change, etc.

The most recent patch didn't take #292253: Remove the per-user themes selection from core into account, now merges correctly.

My 'administer site configuration' grep returned 85 results, so it's not accurate to describe it as t('Configure module settings.'). Change to t('Configure site-wide settings for modules and view system configuration warnings.') which is similar to the current HEAD text.

Menu item admin/settings/site-information wasn't using administer site information in this latest version, may have been lost in a merge.

Function system_update_7037 was only adding administer site appearance, needed to also add administer site information to users that have administer site configuration (since it was forked from that permission).

There is still a ton of things wrong with the menu / permission process, but I'm going to break them out into separate issues since it doesn't have direct bearing on this one.

The errors were 'Drupal error handlers' = in simpletest itself, which doesn't seem like a result of this patch.

Retesting, and if this doesn't work then I'll let it sit a few weeks.

The retest failed because of conflicts (passed previously). Just needs a re-roll against head some time after the code freeze.

@kwinters: Does that mean that this isn't going to get into 7.x?

Just wondering if I should update Site Info Lite to d7 as well as d6.

The freeze for that was some time in the fall, so it will have to wait for D8.

FYI, you probably don't need to add a separate form in your module in D6 / D7. See the approach in my last patch. With a hook_menu override and adding the new perm, I think you can accomplish the same thing.

Ok. I upgraded the Site Info Lite module to d7 and d6.

@kwinters: that's true, but I kind of like having them separate - so admins and editors have a separate view. I don't think many people use the module yet, but if they do, we're happy to accept feedback on the issue queue over there.

maybe http://drupal.org/project/config_perms should be pushed into core for drupal 8?

Maybe in concept, but probably not using any of its code directly. The D7 admin tree is already heavily reorganized so it's hard to tell if all the concepts even still apply. Who knows what D8 will bring.

In D7/D8 there's a new permission "Administer themes" that separates these permissions-

http://drupal.org/node/446666

Can we mark this as closed?

Actually, upon further thought, I'd really like to see the split permissions backported to D6. If there's no objections, I'll go ahead and roll a patch.

D6 backport

There's no way this is going to get into d6 core. Too much UI change, and probably bad for some module relying on existing permissions. Use http://drupal.org/project/config_perms for that.

Let's close as a duplicate of #446666: Split 'administer site configuration' into more granular permissions