The documentation for query alters needs to be clarified. This is in the Node Access topic/group in the API docs. It currently says:
When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved, through the use of hook_query_TAG_alter().
I think it should say something like this:
When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved, through the use of hook_query_TAG_alter(). However, note that tagging with "node_access" does not automatically check the published/unpublished status of nodes, so your base query is responsible for ensuring that unpublished nodes are not displayed to inappropriate users.
--- As a somewhat separate issue, but I'm putting it here because it is related: I think we need to add to the documentation on the query alteration docs page and make it clear what the node_access tag does and doesn't do. Right now, it just says:
This query should have node access restrictions placed on it.
I think it should say:
This query should have node access restrictions placed on it; all queries that retrieve a list of nodes (or node IDs) for display to users should have this tag. However, note that when the Node module alters queries with this tag, it does not check the published/unpublished status of nodes, so your base query is responsible for ensuring that unpublished nodes are not displayed to inappropriate users.
I will take care of that edit shortly.
Comment | File | Size | Author |
---|---|---|---|
#63 | 1559506-63-query-alter-docs.patch | 993 bytes | laranajim |
#48 | 1559506-45.patch | 0 bytes | anil280988 |
| |||
#45 | interdiff-1559506-38-45.txt | 1.18 KB | anil280988 |
#45 | query_alter-1559506-45.patch | 1.04 KB | anil280988 |
#41 | query_alter-1559506-41.patch | 1.8 KB | snehi |
Comments
Comment #1
jhodgdonI've taken care of the docs page edit. If someone could make a patch for the @defgroup node_access in modules/node/node.module, that would be helpful!
Comment #2
JuliaKM CreditAttribution: JuliaKM commentedAttached is a patch for modules/node/node.module.
Comment #3
JuliaKM CreditAttribution: JuliaKM commentedOops. Trailing space.
Comment #4
jhodgdonHm. I think you took the text that I suggested for (and added to) http://drupal.org/node/310077 instead of the text I suggested for the node access API docs. I'm not sure it quite makes sense in the context here?
Comment #5
JuliaKM CreditAttribution: JuliaKM commentedD'oh! Will fix today.
Comment #6
JuliaKM CreditAttribution: JuliaKM commentedHere's a new version.
Comment #7
JuliaKM CreditAttribution: JuliaKM commentedOops, trailing space.
Comment #8
jhodgdonUmmm... I think it's still missing some context -- read the entire paragraph with your patch. Does it make sense?
Comment #9
JuliaKM CreditAttribution: JuliaKM commentedI rewrote the text a bit for clarity. Please verify that it is still accurate.
Thanks!
Julia
Comment #10
jhodgdonMuch better!
I'm wondering if this could be made even clearer by using a list. Something like:
... proper functioning of the pager system. To ensure that your query only retrieves nodes the user should have access to:
- Use a db_select() dynamic query, rather than writing the SQL for your query directly.
- Use SelectQuery::addTag() to add the 'node_access' tag to the query. This will ...
- Add conditions to check the published status of the node ...
Comment #11
jhodgdonComment #12
mparker17It doesn't look like this issue has been touched in two years, so unassigning...
Comment #13
ichionid CreditAttribution: ichionid commented9: drupal-query-1559506.patch queued for re-testing.
Comment #14
ichionid CreditAttribution: ichionid commented7: nodedocs-1559506-v3.patch queued for re-testing.
Comment #15
ichionid CreditAttribution: ichionid commented6: nodedocs-1559506-v3.patch queued for re-testing.
Comment #16
ichionid CreditAttribution: ichionid commented3: nodedocs-1559506_v2.patch queued for re-testing.
Comment #17
ichionid CreditAttribution: ichionid commented2: nodedocs-1559506.patch queued for re-testing.
Comment #23
mparker17@ichionid, you only need to re-queue the most-recent patch.
Comment #24
finnydobson CreditAttribution: finnydobson commentedI am in high school and this is my first contribution to drupal. :) I updated the documentation to include list input according to comment #10.
Comment #25
mparker17Awesome! Welcome to the Drupal community! :)
Considering how long this issue has sat around in the queue, it'll be like reviewing a whole new patch anyway, so an
interdiff.txt
file is probably unnecessary, but for the future, I find it easier submit a straight re-roll of a patch, then submit a second patch with any changes plus an interdiff. Again, it's not a big deal, so don't sweat it :)I reviewed your code: the wording makes sense and it conforms to coding standards. I'm going to be brave/controversial and mark this as Reviewed and Tested By the Community (RTBC) — although the Documentation team may want to make some more changes. IMO, even if it gets committed as-is, it'll be better than the documentation we have now.
Thanks for your hard work!
Comment #26
mparker17Just gonna hide those older patches...
Comment #27
jhodgdonThanks for the patch!
The list format is not quite right:
See https://www.drupal.org/node/1354#lists
The lines starting "directly" and "will make" (etc.) should be indented two spaces to line up under the "Use".
I also think that before this list, we should tell people to use an entity query instead of making that suggestion after the list. That is really what people should be doing, not writing their own queries with db_select.
I'm also wondering whether we should even be putting in this information about how to use db_select. Really they should *always* use an entity query, right?
I realize I filed this issue... but it was more than 2.5 years ago before entity queries were really fully usable to do everything you might want to do.
Comment #28
finnydobson CreditAttribution: finnydobson commentedThank you for the comments. I have had another attempt at fixing up the list structure and removed the db_select information.
Comment #29
jhodgdonThanks for the new patch, getting better!
A few things:
a) Patch has file .gitignore in it and shouldn't.
b) We should really just tell people to use the entity query and NOT have any other information about query tags. The note about making sure the node is published is good, though. So probably we do not need a list. Just tell people to use entity queries and then add a note about maybe needing to add a condition for the node being published.
c) If we keep the existing text, there's also a missing space here: "... on entity queries.To have your query ...".
Comment #30
finnydobson CreditAttribution: finnydobson commentedThank you for your review. I removed the .gitignore from the patch and removed the list and information about query tags from the paragraph.
Comment #31
finnydobson CreditAttribution: finnydobson commentedPlease ignore the previous comment! I posted the comment too quickly. In this patch I removed the .gitignore from the patch and removed the list and information about query tags from the paragraph. Thank you.
Comment #32
jhodgdonSo... Apply the patch and read the whole text. ...
This just doesn't make sense. It needs some more words to link what was there to the new text, a sentence getting the point across that you need to use an entity query in order to make a listing.
Comment #33
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedHello ,
I have added some more text to give little more clear view about using db_query() with tags "node_access" , this will help people to understand the concept.
Comment #34
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedHello , Please ignore the last interdiff.txt file
I have added some more text to give little more clear view about using db_query() with tags "node_access" , this will help people to understand the concept.
Comment #35
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedComment #36
jhodgdonThanks for the patch! Sorry for the delay in reviewing -- I've been on vacation and no one else, unfortunately, ever reviews docs patches. :(
So....
No. To list nodes we should always use an entity query. We should never tell people to use a db_select. Ever. The text in the original issue here is more accurate than the replacement.
Comment #37
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #38
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedChanged "db_select()" to "entityQuery".
Comment #39
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAdding patch as said by Jhodgdon.
Comment #40
jhodgdonThanks for the patches!
But...
Um, I think this first sentence is incorrect, or at least when I read it, it seems to contradict the second sentence (which is correct and was the whole point of this issue).
Comment #41
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedI hope i am making sense this time.
Comment #42
jhodgdonThis piece of the patch is out of scope for this issue... did it come from something else you were working on maybe?
This sentence is still incorrect. Completely backwards. Please see previous reviews.
Comment #43
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@jhodgdon can you please give me a little hint what to replace instaed of this.
Comment #44
jhodgdonJust remove it. The following lines explain the correct information already. Thanks!
Comment #45
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAgreed with @jhodgdon, that line can be removed as same thing has been explained in next lines. Removing the line.
Comment #46
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAgreed with @jhodgdon, that line can be removed as same thing has been explained in next lines. Removing the line.
Comment #47
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedWhy D of display is small here. I am not sure what is the meaning of this line.
Thanks
Comment #48
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi snehi,
Sorry for the wrong patch, lines was not removed properly. Uploading the patch again.
Comment #50
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedNothing in patch file.
Comment #51
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAnyone up for this ?
Comment #52
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #53
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone.
Comment #54
jhodgdonFinally, we have a patch that does what this issue summary said it should. THANKS!
Comment #55
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedI was just in mood of fixing it :p
Comment #56
alexpottI think that the word
automatically
is superfluous and makes me a little confused since it implies that there is a way that tagging a query with "node_access" might in some situations check this.Let's replace
your
withthe
.Conditions need to be added to check the published status of the node.
andso your base query is responsible for ensuring that unpublished nodes are not displayed to inappropriate users
seem repetitive and also odd since it will probably only take one condition to check.Comment #57
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commented@alexpott, made the changes.
Comment #58
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedLooks ok to me. +1 for RTBC.
Anil you are working so i unassigned myself from the issue.
Comment #59
jhodgdonLooks good to me also, thanks! Much more concise indeed.
Comment #60
alexpottCommitted b1097a6 and pushed to 8.0.x and 8.1.x. Thanks!
Comment #63
laranajim CreditAttribution: laranajim as a volunteer commentedPorted a patch from Drupal 8 to Drupal 7.
Comment #64
jhodgdonLooks good, thanks!
Comment #65
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!