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.Follow-up to #2506345: Unpublished Nodes accessible
Problem/Motivation
When trying to save grants on a new, unpublished node, the checkboxes won't save.
Proposed resolution
Save grants on unpublished nodes too.
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | unpublished_nodes_grants_checkboxes_wont_save-2541588-6.patch | 538 bytes | Nick Dewitte |
| #1 | unpublished_nodes-2541588-1-tests-only.patch | 2.32 KB | joelpittet |
Comments
Comment #1
joelpittetHere's some tests, they should fail:) I don't have a solution to the problem yet but this should help ensure it works going forward.
First time writing tests before I know the answer:)
Comment #3
joelpittetThose two fails are expected;)
Comment #4
joelpittetActually I should leave this as needs-work because the fix needs to be added to these tests.
Comment #5
joelpittetBump, there are tests to show this is broken, bumping priority.
Comment #6
Nick Dewitte CreditAttribution: Nick Dewitte commentedAttached is a patch that resolves this specific issue in a very naive way.
When this module saves grants, this is done via nodeaccess_set_grants().
This function calls node_access_acquire_grants(), which calls all implementations of hook_node_access_records().
The nodeaccess_node_access_records() implementation (which is the file I patched) checked if the node was unpublished,
and if so, it returned no grants (NULL).
After reading https://api.drupal.org/api/drupal/modules%21node%21node.module/group/nod...
it appears to me that the access based on the published state should be managed in hook_node_access().
Once grants are being checked, I don't think it's appropriate to flat out deny access to all unpublished nodes without any condition.
But perhaps ignoring the published state as this patch does isn't the way to go either?
Adding the "Needs committer feedback" tag since I think this is dependant on how the module should behave.
Comment #8
joelpittetI wrote tests in #1 see if they pass with that fix.
Comment #9
alisonThank you @nick.dewitte and @joelpittet!
So, we need a version of the patch that has the fix and the tests, ya?
More broadly, though:
@nick.dewitte Are you saying that the patch you provided,
(1) allows the grants settings form to be saved (yay), and
(2) means that the grants settings from nodeaccess module will not have any impact on whether a user an see the node if it is unpublished, that determination is left up to whatever unpublished content perms exist on the site?
(Is that an accurate explanation, or?)
I think I'm fine with that concept. I'm ok with hearing other opinions, too, but it seeeeems logical to me that, unless additional nodeaccess-based permissions are added to account for viewing unpublished content (i.e. "view" + "edit" + "delete" + a new "view unpublished" perm), nodeaccess should not impact access to unpublished content. And, that feeeeeels like a safer setup -- I don't think people would want users with "view" access to automatically be able to view unpublished content -- right?