Hi

I run into a bug today.
When a block gets a class asigned using the "Advances options" fieldset gets then edited by a user withouth "access skinr classes" permission, the class is lost.

To reproduce:

  1. Create a user with "access skinr" but no "access skinr classes"
  2. As and admin, edit a block and add a class using "Advanced options" fieldset
  3. Login in as user created in step 1, and edit the block from step 2, do whatever you feel like, but remember to "Save"
  4. Login as admin again, and go to the edit page of the block, NO CLASS!

I Checked the code, but got a bit confused on how skinr does the magic, so no patch :(

Federico

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jm.federico’s picture

Title: class lost when user doesn't "access skinr classes" permission » class lost when user doesn't have "access skinr classes" permission

Change title

Jacine’s picture

Category: bug » support
Priority: Major » Normal
Status: Active » Postponed (maintainer needs more info)

The "access skinr classes" is for the advanced settings on the skinr settings form. It has nothing to do with the classes actually printing, and neither do any of the other permissions. They are all strictly for administering Skinr.

If you are still having this problem it's likely that your theme became disabled by this lovely core bug: #305653: Themes disabled during update. It's hard to catch because your theme doesn't actually change, but when you visit the theme listing page, you'll see that it's not checked. That is a common cause of this happening with Skinr.

It's also possible that you are using a module that is causing problems with Skinr, like SwitchTheme, which wreaks havoc with Skinr and should not be used with it.

I'm pretty sure this is not a bug with Skinr, but I'll wait to hear back from you for a bit before closing.

jm.federico’s picture

Hello, thanks for your reply.

I think I didn't make myself clear, the problem is not that the classes are printing or not, is that they get lost. This is the deal:

You have 2 users, both can edit blocks, one of them (User 1) has both "access skinr" and "access skinr classes" permissions, the other (User 2) only has the "access skinr".

When User 1 edits a block and sets a class using the advanced settings, the class does gets printed, all good so far.
Later, user 2 edits the same block as user 1, and after editing and saving the changes, the class that user 1 had set is lost.
User 1 has to go back to the block and set the class again.

Its seems to me the problem is that skinr detects user 2 does not have the permission to use advanced settings, so it does not saves the data from that field, even when there was something there previously set by user 1.

Hope this clarifies the problem.

Cheers

jm.federico’s picture

Title: class lost when user doesn't have "access skinr classes" permission » class lost when editing block and user doesn't have "access skinr classes" permission

Changing title ;)

vinoth.3v’s picture

Category: support » bug
Status: Postponed (maintainer needs more info) » Needs review

me too having the same issue.

ericduran’s picture

Status: Needs review » Active

There's no patch to review ;-)

ericduran’s picture

Status: Active » Needs review
FileSize
2.39 KB

Speaking of patches. Here's a patch to fix this issue. :)

The problem was that the skin was getting reset without checking the old values. So in the case where the user didn't have permissions to the classes options skinr would just set that option as empty without populating it with the old values.

This patches solves this issue by checking if the user has access to the classes options. If the user doesn't have access then it checks if the classes where set before, if they where it sets it again, if it wasn't then it just leave it alone.

ericduran’s picture

FileSize
2.39 KB

EDIT: REMOVED by ericduran.

ericduran’s picture

I love it my Comments are somehow getting submitted twice at the same exact time.

jm.federico’s picture

for #7 (and #8)
Worked for me!

nomonstersinme’s picture

Jacine perhaps this should be committed?

Jacine’s picture

Status: Needs review » Reviewed & tested by the community
ericduran’s picture

Status: Reviewed & tested by the community » Needs work

I know this is minimal, and I did write the patch but I'm reviewing this now and the coding standard is wrong. I'm missing a space in the "else".

Also I'm curious if we can have more than one person test this patch ? I just looked at the code and it seems right but I feel we might have miss a couple of situations. Maybe test what happens when someone has the other skinr permissions and make sure nothing breaks. I just don't remember. It was a while ago :-(

Jacine’s picture

Yes, well I would have tested and fixed the code style issue before committing anyway, given what's been going on lately, but you're right.

I would like to see tests written and SimpleTest used, TBH. I just have no clue how to begin with something like that.

ericduran’s picture

Ha, I was thinking simpletest too. :)

nomonstersinme’s picture

Trying to use the patch to test but i cant get it to patch.. :/

jm.federico’s picture

Been using it on a live site since it was released.

My setup:

Permissions enabled:
- access skinr

Other permissions are disabled. Only User 1 can do everything.

Cheers

Jacine’s picture

+++ includes/handlers.inc	10 Sep 2010 04:36:34 -0000
@@ -75,6 +75,7 @@ function skinr_submit_handler(&$form, $f
+        $pre_edit_skin = skinr_get($theme_name, $hook , $key);

I think $saved would be better here.

Powered by Dreditor.

Will be testing this shortly ;)

Jacine’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Ok, works like a charm. I can't believe no one has reported this sooner. Thank you @jm.federico and @ericduran!

Here's a re-roll, with better comments and $pre_edit_skin changed to $saved. Please review :)

Status: Needs review » Needs work

The last submitted patch, 891942.patch, failed testing.

moonray’s picture

The patch works as advertised. It just needs to be re-rolled to pass testing.

moonray’s picture

FileSize
6.07 KB

This patch makes the above adjustments in the panels and views plugins as well for version 1.x.

moonray’s picture

Status: Needs work » Needs review
FileSize
5.92 KB

And the same patch for 2.x version of skinr.

Status: Needs review » Needs work

The last submitted patch, skinr-2.x_891942-23.patch, failed testing.

moonray’s picture

Status: Needs work » Needs review

Testing failed due to additional wrong version patch. Please test the patch in #22.

moonray’s picture

Version: 6.x-1.5 » 6.x-1.x-dev
moonray’s picture

FileSize
5.62 KB

Here's a new patch that should apply cleanly to 6.x-1.x-dev.

vrajak@gmail.com’s picture

The patch in #27 applies cleanly and fixes the described issue for 6.x-1.x-dev.

Status: Needs review » Needs work

The last submitted patch, skinr_891942_27.patch, failed testing.

ericduran’s picture

Status: Needs work » Needs review

#27: skinr_891942_27.patch queued for re-testing.

nomonstersinme’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm that the patch in #27 works as described

Jacine’s picture

Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed ;)

6.x-2.x: http://drupal.org/cvs?commit=449004
6.x-1.x: http://drupal.org/cvs?commit=449002

Switching to 7.x, as this needs to be ported.

vrajak@gmail.com’s picture

Status: Patch (to be ported) » Needs review
FileSize
26.33 KB

Here is my stab at porting the D6 patch to D7. Probably need some revisions, but we'll see.

*edit* Since I redid my CVS just now it seems the patch is doing a lot of deleting then adding the same code back. I think its because of a way I edited the file. Will see and try to revise properly. Ignore this patch.

vrajak@gmail.com’s picture

FileSize
4.28 KB

This patch now has just the needed changes, hopefully it works.

nomonstersinme’s picture

I was going to test this issue, but before i even installed the patch has anyone confirmed this is a problem in 7? because i can't replicate the error before applying the patch.... :/ my custom class is not getting lost when edited by a user with just access skinr nor does the custom class field appear on the block configuration page....

Jacine’s picture

Status: Needs review » Needs work

There's no reason the problem shouldn't exist. It's on the old codebase, which definitely had a bug. I have no idea why you wouldn't be able to reproduce that, but of course that is the first step that should happen in testing.

Also, the indentation is all off in this patch and should be 2 spaces, not tabs. Try running the patch through the coder module.

nomonstersinme’s picture

does this problem happen for you?

vrajak@gmail.com’s picture

I tested this on my current D7 setup and was unable to reproduce the bug. However I would like to install a fresh copy of D7 which has been updated anyway since I setup my local environment. Will do that soon and post back.

vrajak@gmail.com’s picture

Tried testing with latest versions of Skinr & D7 but the edit skin and advanced options from config menu have vanished. Possible issues with update code in Skinr, (http://drupal.org/node/956990#comment-3812874) pending a fix before I can report on the class lost bug being an issue with D7 Skinr or not.

vrajak@gmail.com’s picture

Okay thanks to a patch by Moonray things are working again. I tested with latest version of Skinr (2010-Dec-09) and current D7 (2010-Dec-01) and following the steps does not reproduce the bug. The class remains fine even after editing. This was a bug in D6 Skinr that was never patched in D7 Skinr so why the bug is not present is mysterious.

moonray’s picture

With #995080: Permissions are confusing fixed, the bug should now become apparent.

moonray’s picture

Vraja: Have you been testing with an account other than the primary admin account?
I can reproduce this bug without fail.

moonray’s picture

Priority: Normal » Major
moonray’s picture

Assigned: Unassigned » moonray
FileSize
3.64 KB

Here are some tests for this issue.

moonray’s picture

last patch had a redundant test. Removed and updated.

moonray’s picture

And of course, it fails. Let's try again.

EDIT: #1082842: Update storage of skin configurations to give more granular control already fixes this bug and passes these tests.

vrajak@gmail.com’s picture

Re #42: "Vraja: Have you been testing with an account other than the primary admin account?"

I don't think I tried this outside of the primary admin account, maybe that was why I couldn't get it. Your latest patches fix this it seems, but i'll test today and see what happens.

moonray’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.