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.

CommentFileSizeAuthor
#63 1559506-63-query-alter-docs.patch993 byteslaranajim
#57 interdiff-1559506-53-57.txt1.01 KBanil280988
#57 query_alter-1559506-57.patch897 bytesanil280988
#53 query_alter-1559506-53.patch988 bytessnehi
#53 interdiff-41-53.txt1.06 KBsnehi
#48 interdiff-1559506-38-45.txt1.1 KBanil280988
#48 1559506-45.patch0 bytesanil280988
#45 interdiff-1559506-38-45.txt1.18 KBanil280988
#45 query_alter-1559506-45.patch1.04 KBanil280988
#41 query_alter-1559506-41.patch1.8 KBsnehi
#41 interdiff-41-39.txt1005 bytessnehi
#39 query_alter-1559506-38.patch1.11 KBsnehi
#39 interdiff-33-38.txt2.04 KBsnehi
#34 interdiff.txt1.76 KBpriya.chat
#33 interdiff.txt6.41 KBpriya.chat
#33 1559506-fixing-documentation-33.patch1.76 KBpriya.chat
#31 interdiff.txt1.88 KBfinnydobson
#31 1559506-31-fixing-documentation.patch1.6 KBfinnydobson
#31 interdiff.txt1.88 KBfinnydobson
#31 1559506-30-fixing-documentation.patch1.6 KBfinnydobson
#30 1559506-30-fixing-documentation.patch1.62 KBfinnydobson
#28 interdiff.txt1.87 KBfinnydobson
#28 1559506-28-updated-list-to-follow-listing-rules.patch3.2 KBfinnydobson
#24 1559506-24-add-list-to-the-documentation.patch1.95 KBfinnydobson
#9 drupal-query-1559506.patch1.67 KBJuliaKM
#7 nodedocs-1559506-v3.patch1.39 KBJuliaKM
#6 nodedocs-1559506-v3.patch1.4 KBJuliaKM
#3 nodedocs-1559506_v2.patch1.4 KBJuliaKM
#2 nodedocs-1559506.patch1.4 KBJuliaKM
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

I'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!

JuliaKM’s picture

Status: Active » Needs review
FileSize
1.4 KB

Attached is a patch for modules/node/node.module.

JuliaKM’s picture

FileSize
1.4 KB

Oops. Trailing space.

jhodgdon’s picture

Status: Needs review » Needs work

Hm. 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?

JuliaKM’s picture

Assigned: Unassigned » JuliaKM

D'oh! Will fix today.

JuliaKM’s picture

FileSize
1.4 KB

Here's a new version.

JuliaKM’s picture

FileSize
1.39 KB

Oops, trailing space.

jhodgdon’s picture

Ummm... I think it's still missing some context -- read the entire paragraph with your patch. Does it make sense?

JuliaKM’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

I rewrote the text a bit for clarity. Please verify that it is still accurate.

Thanks!
Julia

jhodgdon’s picture

Much 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 ...

jhodgdon’s picture

Status: Needs review » Needs work
mparker17’s picture

Assigned: JuliaKM » Unassigned

It doesn't look like this issue has been touched in two years, so unassigning...

ichionid’s picture

Status: Needs work » Needs review

9: drupal-query-1559506.patch queued for re-testing.

ichionid’s picture

7: nodedocs-1559506-v3.patch queued for re-testing.

ichionid’s picture

6: nodedocs-1559506-v3.patch queued for re-testing.

ichionid’s picture

3: nodedocs-1559506_v2.patch queued for re-testing.

ichionid’s picture

2: nodedocs-1559506.patch queued for re-testing.

The last submitted patch, 2: nodedocs-1559506.patch, failed testing.

The last submitted patch, 3: nodedocs-1559506_v2.patch, failed testing.

The last submitted patch, 6: nodedocs-1559506-v3.patch, failed testing.

The last submitted patch, 7: nodedocs-1559506-v3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: drupal-query-1559506.patch, failed testing.

mparker17’s picture

@ichionid, you only need to re-queue the most-recent patch.

finnydobson’s picture

Status: Needs work » Needs review
Issue tags: +CatalystAcademy
FileSize
1.95 KB

I am in high school and this is my first contribution to drupal. :) I updated the documentation to include list input according to comment #10.

mparker17’s picture

Status: Needs review » Reviewed & tested by the community

Awesome! 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!

mparker17’s picture

Just gonna hide those older patches...

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the patch!

The list format is not quite right:

+ * system. To have your query only retrieve nodes which the user has 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 make sure that your base query does not allow for the display of
...

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.

finnydobson’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
1.87 KB

Thank you for the comments. I have had another attempt at fixing up the list structure and removed the db_select information.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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 ...".

finnydobson’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

Thank you for your review. I removed the .gitignore from the patch and removed the list and information about query tags from the paragraph.

finnydobson’s picture

Please 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.

jhodgdon’s picture

Status: Needs review » Needs work

So... Apply the patch and read the whole text. ...

In node listings (lists of nodes generated from a select query, such as the default home page at path 'node', an RSS feed, a recent content block, etc.), the process above is followed except that hook_node_access() is not called on each node for performance reasons and for proper functioning of the pager system. See the @link entity_api Entity API topic @endlink for more information on entity queries. This will make sure that your base query does not allow for the display of unpublished nodes to users who should not have ...

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.

priya.chat’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
6.41 KB

Hello ,
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.

priya.chat’s picture

FileSize
1.76 KB

Hello , 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.

priya.chat’s picture

Assigned: Unassigned » priya.chat
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch! Sorry for the delay in reviewing -- I've been on vacation and no one else, unfortunately, ever reviews docs patches. :(

So....

+++ b/core/modules/node/node.module
@@ -878,12 +878,15 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
+ * system. So for node listing we can use db_select() query with tag of
+ * "node_access" having a check for published/unpublished status of the nodes
+ * with node access. See the @link entity_api Entity API topic @endlink for more

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.

snehi’s picture

Assigned: priya.chat » snehi
anil280988’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
905 bytes

Changed "db_select()" to "entityQuery".

snehi’s picture

Assigned: snehi » Unassigned
FileSize
2.04 KB
1.11 KB

Adding patch as said by Jhodgdon.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patches!

But...

+++ b/core/modules/node/node.module
@@ -883,7 +883,12 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
+ * queries. This will make sure that your base query does not allow for the
+ * display of unpublished nodes to users who should not have access to the node.
+ * Conditions need to be added to check the published status of the node.

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).

snehi’s picture

Status: Needs work » Needs review
FileSize
1005 bytes
1.8 KB

I hope i am making sense this time.

jhodgdon’s picture

Status: Needs review » Needs work
  1. --- a/core/lib/Drupal/Core/Entity/EntityInterface.php
    +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    

    This piece of the patch is out of scope for this issue... did it come from something else you were working on maybe?

  2. +++ b/core/modules/node/node.module
    @@ -883,7 +883,12 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
    + * queries. This will check that base query does not allow for the display of
    + * unpublished nodes to inappropriate users.
    

    This sentence is still incorrect. Completely backwards. Please see previous reviews.

snehi’s picture

@jhodgdon can you please give me a little hint what to replace instaed of this.

This will make sure that your base query does not allow for the display of unpublished nodes to users who should not have access to the node.

jhodgdon’s picture

Just remove it. The following lines explain the correct information already. Thanks!

anil280988’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
1.18 KB

Agreed with @jhodgdon, that line can be removed as same thing has been explained in next lines. Removing the line.

anil280988’s picture

Agreed with @jhodgdon, that line can be removed as same thing has been explained in next lines. Removing the line.

snehi’s picture

+++ b/core/modules/node/node.module
@@ -883,7 +883,11 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
+ * queries. display of unpublished nodes to users who should not have access to

Why D of display is small here. I am not sure what is the meaning of this line.
Thanks

anil280988’s picture

Hi snehi,
Sorry for the wrong patch, lines was not removed properly. Uploading the patch again.

Status: Needs review » Needs work

The last submitted patch, 48: 1559506-45.patch, failed testing.

snehi’s picture

Nothing in patch file.

snehi’s picture

Anyone up for this ?

snehi’s picture

Assigned: Unassigned » snehi
snehi’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
988 bytes

Done.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Finally, we have a patch that does what this issue summary said it should. THANKS!

snehi’s picture

I was just in mood of fixing it :p

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/node/node.module
    @@ -883,7 +883,10 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
    + * node. Tagging a query with "node_access" does not automatically check the
    

    I 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.

  2. +++ b/core/modules/node/node.module
    @@ -883,7 +883,10 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
    + * published/unpublished status of nodes, so your base query is responsible
    

    Let's replace your with the.

  3. Conditions need to be added to check the published status of the node. and so 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.
anil280988’s picture

Status: Needs work » Needs review
FileSize
897 bytes
1.01 KB

@alexpott, made the changes.

snehi’s picture

Assigned: snehi » Unassigned

Looks ok to me. +1 for RTBC.
Anil you are working so i unassigned myself from the issue.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me also, thanks! Much more concise indeed.

alexpott’s picture

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

Committed b1097a6 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed fff56d7 on 8.1.x
    Issue #1559506 by finnydobson, JuliaKM, snehi, anil280988, priya.chat,...

  • alexpott committed b1097a6 on
    Issue #1559506 by finnydobson, JuliaKM, snehi, anil280988, priya.chat,...
laranajim’s picture

Status: Patch (to be ported) » Needs review
FileSize
993 bytes

Ported a patch from Drupal 8 to Drupal 7.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 11e1856 on 7.x
    Issue #1559506 by finnydobson, JuliaKM, snehi, anil280988, priya.chat,...

Status: Fixed » Closed (fixed)

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