Function node_configure() (http://api.drupal.org/api/function/node_configure/6) generates the Post settings page and decides whether or not to display the Rebuild permissions button, which it does so if either
1. a node access module is active
2. the {node_access} table doesn't contain precisely 1 row
In 6.x/7.x, the 2nd test should be node_access_needs_rebuild().
In 5.x, the 2nd test should also check whether the 1 existing row is the default grant (it is possible for a node access module to leave a single row in the table which is not the default grant). Whether this check would also be useful for 6.x/7.x I'm not sure - it might catch edge cases but probably should not be necessary in theory (because in 6.x+ when all node access modules are disabled the permissions are automatically rebuilt).
Also the frequent absence of the Rebuild permissions button is a significant usability problem (witness the large number of forum topics on this). Therefore a suitable message should be output when the Rebuild permissions function is not available, explaining that the node access permissions do not need rebuilding. Haven't we all at some time been confused by the comings and goings of the Rebuild button?
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 226479-update-rebuild-permissions-check-rev2-d6.patch | 2.87 KB | brianV |
| #10 | 226479-update-rebuild-permissions-check-rev2.patch | 2.8 KB | brianV |
| #1 | post-settings-rebuild-message.png | 42.08 KB | brianV |
| #1 | 226479-update-rebuild-permissions-check.patch | 2.09 KB | brianV |
Comments
Comment #1
brianV commentedHere is a patch to change the check as suggested for D7. It also includes a message informing the user if the permissions cache does not need to be rebuilt (see screenshot).
Comment #2
brianV commentedtagging for D7 User Experience
Comment #3
brianV commentedComment #4
Bojhan commentedCan we get a before and after screenshot for this, and places other then this page where this button occurs.
Comment #5
brianV commentedBojhan,
Without this patch, the 'Node Access Settings' fieldset, with the message inside, doesn't display. Just picture it gone, and there is your 'before' shot.
Also, this is the only place the button occurs.
Comment #6
Bojhan commentedOke, so we might concider making this a drupal_set_message rather then a fieldset.
Comment #7
brianV commentedWe could, but I see a benefit in the message being exactly where the user would expect to find the form if it had generated. Of course, this is just one man's opinion.
Comment #8
dries commentedI was actually going to suggest that we can _remove_ the fieldset. If nothing needs to be done, we don't need to tell the user anything. I'm tempted to mark this patch "won't fix".
Comment #9
catchI don't think we need a message saying that permissions are fine. It seems like if you had a batch update of node access interrupted, the proposed patch would leave your table in a half-built state with no button to push - similar to if it timed out in D5. I think like the search index button we should just leave this there all the time - if you press it when you don't need to, you waste some cycles.
Comment #10
brianV commentedcatch,
You raise some good points.
Here is a new revision removing the check altogether, so the field set and button display all the time, regardless of whether or not the permissions actually need to be rebuilt.
Comment #11
catchPatch looks good to me. Going to let yoroy or Bojhan (or anyone else interested) rtbc it though.
Comment #12
yoroy commentedyoroy: does the patch make it show even if no node acces is happening?
ah yes, so it seems. remove the check altogether
catch: yeah it shows it all the time. there's a chance you could remove a node access module, leave the rows in your table, and end up locking your users out. pressing the button should set it back to normal.
yoroy: right. well, if the check is untrustworthy and you might need it to fix something horrible, better always be able to access it
Comment #13
yoroy commentedAnd then get to relocating the button asap, we want to get rid of this Post Settings screen entirely: #361277: Remove the 'post settings' admin screen and relocate contents
Comment #14
dries commentedCommitted to CVS HEAD. Thanks!
Comment #15
gpk commentedI've not checked whether this applies to D6, otherwise would have set to RTBC.
For D5 I think some sort of status message would still be needed (probably on the post settings page in the permissions fieldset) for the avoidance of confusion. In D6 you get the status message when a permissions rebuild is needed and so the absence of this message is probably sufficient indication of whether Drupal thinks there is likely to be a problem with the current permissions cache. If we suddenly make this fieldset appear permanently in D5 then I think it needs a bit of explanation.
But first, D6.
Comment #16
brianV commentedD6 patch attached.
What text should we have for the d5 version?
Comment #17
brianV commentedComment #18
gábor hojtsyCommitted yesterday in http://drupal.org/cvs?commit=222608 but forgot to make note here. Thanks!
Comment #19
gpk commented@16: the consensus here might well actually be for the same approach as in 6.x, i.e. just show the fieldset at all times. I only hesitate to agree with this approach because it is adding an extra control that on simple sites (with no node access considerations) adds nothing except another button to confuse the site admin and a couple of paras of text that are basically irrelevant/largely meaningless -> more confusion. If the permissions definitely don't need rebuilding then suppose I'd do something like showing the fieldset with the Rebuild button disabled and the simple text "The permissions cache does not need to be rebuilt."
A slightly separate point - per the original issue post, in http://api.drupal.org/api/function/node_configure/5 the test
count(module_implements('node_grants')) > 0also needs to check that the single row *is* the default grant. If not then a rebuild is *definitely* needed.Comment #20
marcingy commentedMarking as won't fix as d5 is end of life.