Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
- Create a user with "access skinr" but no "access skinr classes"
- As and admin, edit a block and add a class using "Advanced options" fieldset
- Login in as user created in step 1, and edit the block from step 2, do whatever you feel like, but remember to "Save"
- 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
Comment | File | Size | Author |
---|---|---|---|
#46 | patch_commit_2bf38e0f20c3.patch | 3.49 KB | moonray |
#45 | patch_commit_2bf38e0f20c3.patch | 3.49 KB | moonray |
#44 | patch_commit_2bf38e0f20c2.patch | 3.64 KB | moonray |
#34 | skinr_891942_35.patch | 4.28 KB | vrajak@gmail.com |
#33 | classlostD7.patch | 26.33 KB | vrajak@gmail.com |
Comments
Comment #1
jm.federico CreditAttribution: jm.federico commentedChange title
Comment #2
JacineThe "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.
Comment #3
jm.federico CreditAttribution: jm.federico commentedHello, 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
Comment #4
jm.federico CreditAttribution: jm.federico commentedChanging title ;)
Comment #5
vinoth.3v CreditAttribution: vinoth.3v commentedme too having the same issue.
Comment #6
ericduran CreditAttribution: ericduran commentedThere's no patch to review ;-)
Comment #7
ericduran CreditAttribution: ericduran commentedSpeaking 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.
Comment #8
ericduran CreditAttribution: ericduran commentedEDIT: REMOVED by ericduran.
Comment #9
ericduran CreditAttribution: ericduran commentedI love it my Comments are somehow getting submitted twice at the same exact time.
Comment #10
jm.federico CreditAttribution: jm.federico commentedfor #7 (and #8)
Worked for me!
Comment #11
nomonstersinme CreditAttribution: nomonstersinme commentedJacine perhaps this should be committed?
Comment #12
JacineComment #13
ericduran CreditAttribution: ericduran commentedI 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 :-(
Comment #14
JacineYes, 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.
Comment #15
ericduran CreditAttribution: ericduran commentedHa, I was thinking simpletest too. :)
Comment #16
nomonstersinme CreditAttribution: nomonstersinme commentedTrying to use the patch to test but i cant get it to patch.. :/
Comment #17
jm.federico CreditAttribution: jm.federico commentedBeen 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
Comment #18
JacineI think $saved would be better here.
Powered by Dreditor.
Will be testing this shortly ;)
Comment #19
JacineOk, 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 :)
Comment #21
moonray CreditAttribution: moonray commentedThe patch works as advertised. It just needs to be re-rolled to pass testing.
Comment #22
moonray CreditAttribution: moonray commentedThis patch makes the above adjustments in the panels and views plugins as well for version 1.x.
Comment #23
moonray CreditAttribution: moonray commentedAnd the same patch for 2.x version of skinr.
Comment #25
moonray CreditAttribution: moonray commentedTesting failed due to additional wrong version patch. Please test the patch in #22.
Comment #26
moonray CreditAttribution: moonray commentedComment #27
moonray CreditAttribution: moonray commentedHere's a new patch that should apply cleanly to 6.x-1.x-dev.
Comment #28
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedThe patch in #27 applies cleanly and fixes the described issue for 6.x-1.x-dev.
Comment #30
ericduran CreditAttribution: ericduran commented#27: skinr_891942_27.patch queued for re-testing.
Comment #31
nomonstersinme CreditAttribution: nomonstersinme commentedI can confirm that the patch in #27 works as described
Comment #32
JacineThanks! 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.
Comment #33
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedHere 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.
Comment #34
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedThis patch now has just the needed changes, hopefully it works.
Comment #35
nomonstersinme CreditAttribution: nomonstersinme commentedI 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....
Comment #36
JacineThere'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.
Comment #37
nomonstersinme CreditAttribution: nomonstersinme commenteddoes this problem happen for you?
Comment #38
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedI 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.
Comment #39
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedTried 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.
Comment #40
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedOkay 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.
Comment #41
moonray CreditAttribution: moonray commentedWith #995080: Permissions are confusing fixed, the bug should now become apparent.
Comment #42
moonray CreditAttribution: moonray commentedVraja: Have you been testing with an account other than the primary admin account?
I can reproduce this bug without fail.
Comment #43
moonray CreditAttribution: moonray commentedComment #44
moonray CreditAttribution: moonray commentedHere are some tests for this issue.
Comment #45
moonray CreditAttribution: moonray commentedlast patch had a redundant test. Removed and updated.
Comment #46
moonray CreditAttribution: moonray commentedAnd 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.
Comment #47
vrajak@gmail.com CreditAttribution: vrajak@gmail.com commentedRe #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.
Comment #48
moonray CreditAttribution: moonray commentedCommitted tests to http://drupalcode.org/project/skinr.git/commit/9b7803e
#1082842: Update storage of skin configurations to give more granular control fixes the bug itself.