This is a really trivial change, but I needed to limit the content retrieved in the content section of the administration pages by using the db_rewrite_sql hook in a new module. I was surprised to find that the main query was not wrapped in db_rewrite_sql. You might wonder why one needs to change this query, one possible reason is to display content in a particular locale (with i18n enabled). AFAIK, this change shouldn't affect anything.

Original Code
$result = pager_query('SELECT n.*, u.name, u.uid FROM {node} n '. $filter['join'] .' INNER JOIN {users} u ON n.uid = u.uid '. $filter['where'] .' ORDER BY n.changed DESC', 50, 0, NULL, $filter['args']);

New Code
$result = pager_query(db_rewrite_sql('SELECT n.*, u.name, u.uid FROM {node} n '. $filter['join'] .' INNER JOIN {users} u ON n.uid = u.uid '. $filter['where'] .' ORDER BY n.changed DESC', 50, 0, NULL, $filter['args']));

CommentFileSizeAuthor
#14 node_query_wrapped_in_rewrite_sql_0.patch1.11 KBAshraf Amayreh
#3 node_query_wrapped_in_rewrite_sql.patch968 bytesAshraf Amayreh

Comments

Ashraf Amayreh’s picture

Ashraf Amayreh commented
Version: 5.1 » 5.x-dev
Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community
drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Active

A patch file needs to be attached. http://drupal.org/patch

This should be added to Drupal 6 first, then 5.

Ashraf Amayreh’s picture

Ashraf Amayreh commented
Status: Active » Needs review
StatusFileSize
new node_query_wrapped_in_rewrite_sql.patch968 bytes

Thanks for the the info :-)

Here's a patch file for the anticipated change. It simply wraps the administration content query with db_rewrite_sql so other modules may manipulate it. This may be beneficial for multi-lingual modules that wish to view the administrative content in one local or another.

Although it seems to me like a pretty simple one liner, I'd like to get some feedback in case there's something that I'm missing.

With regards,
Ashraf Amayreh

maherg’s picture

maherg commented

On a drupal 4.7 + i18n , the nodes still don't get filtered.

I assume this is an i18n bug since the mode in admin pages is off

Ashraf Amayreh’s picture

Ashraf Amayreh commented

This patch is unrelated to i18n module although the i18n module could definitely benefit from it. This patch will simply allow other modules to alter the administration query that retrieves the site's node listing.

Ashraf Amayreh’s picture

Ashraf Amayreh commented
Status: Needs review » Reviewed & tested by the community

I tested the code without finding any problems...

dries’s picture

dries commented

This looks like the right thing to do.

However, there are 43 other queries that query the node table, but not us db_rewrite_sql:

$ grep -r {node} * | grep SELECT | grep -v db_rewrite_sql | wc -l
      43

This might not be a problem, and probably belongs in another patch. It makes one wonder though.

Ashraf Amayreh’s picture

Ashraf Amayreh commented

That's what happens when you learn about issue subscription a little too late (like 1 month late lol)

You're right, there are a large number of queries that query the node table. Although we could go about changing all of them, I'll check if there's something else we can do to avoid this every time a change is required.

Initially, it strikes me as if all queries should be wrapped in db_rewrite_sql (or perhaps to integerate it within the db_query itself?) , but that's too much of an overhead unless we can largely minimize the footprint of the hook_rewrite_sql. I'll look into this more deeply, what do you think?

dries’s picture

dries commented

I'm mostly wondering what the security implications are of not wrapping these queries in db_rewrite_sql(). It might affect this patch.

Ashraf Amayreh’s picture

Ashraf Amayreh commented

I don't think there's a problem with this particular query at all, this query is very local in scope, it is simply used to display a result set. So it should not be affected by other queries.

We can work on other queries incrementally to get the desired result of being able to heavily influence the administration pages and customize them. But as it looks now, I think this patch is ready to roll.

Ashraf Amayreh’s picture

Ashraf Amayreh commented

Just reminding everyone about this patch :-)

I think it's too trivial to get thrown off till after drupal 6.

Gurpartap Singh’s picture

Gurpartap Singh commented

You could put the query in a separate variable like $sql and then: $result = db_rewrite_sql($sql); and: $result = pager_query($result); It's just a bit more clear, no?

Gurpartap Singh’s picture

Gurpartap Singh commented

Oops, like this:

$sql = 'SELECT n.*, u.name, u.uid FROM {node} n '. $filter['join'] .' INNER JOIN {users} u ON n.uid = u.uid '. $filter['where'] .' ORDER BY n.changed DESC';
$sql = db_rewrite_sql($sql);
$result = pager_query($sql, 50, 0, NULL, $filter['args']);

And, not sure but is that $filter['join'] and $filter['where'] addition to query safe enough?

Ashraf Amayreh’s picture

Ashraf Amayreh commented
StatusFileSize
new node_query_wrapped_in_rewrite_sql_0.patch1.11 KB

Yup, that's quite more readable. Rerolled against head.

The $filter variable shouldn't cause any problems, as long as the final string is a valid query, db_rewrite_sql should handle it properly. This is ready to roll.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

My vague assumption has been that we avoid db_rewrite_sql() for administrative display because we need admins to see all nodes rather than a filtered subset. Specifically, I've assumed that a user with 'administer nodes' permission should have admin access to all nodes.

Assume that example.module limits access to nodes of type 'example' by role. User x with administer nodes permission might not see example nodes in content administration. Is this the expected and desired outcome?

Ashraf Amayreh’s picture

Ashraf Amayreh commented

Actually, this is correct in most cases. But a couple of scenarios would spring to mind that have required this.

In one of these specific scenarios, I wanted to create a localized administration. The administration page viewed the nodes for all languages and this is not a configurable behavior, I had to wrap the query in db_rewrite_sql to modify it to view only Arabic nodes when viewing the Arabic interface and English nodes when viewing the English interface instead of showing a spaghetti mixture of all locales. Imagine the scenario with three or more languages!

The normal behavior would not change without explicitly enabling a module that explicitly states it alters that page, so no confusion should result from this as it will not effect most people, it's totally transparent.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

Node languages and language based filtering is built into Drupal 6, so this use case does not stand there. Maybe you have another use case? People accessing the node admin page should have admin rights for all nodes, shouldn't they?

Ashraf Amayreh’s picture

Ashraf Amayreh commented
Status: Needs review » Reviewed & tested by the community

hmm... what about a module that limits the content displayed per role?

Let's say role1 is allowed to see a listing of nodes of type "image" & "page" while role2 is allowed to see node listings of "story".

This is, of course, a hypothetical module. But adding this patch will allow such modules. Let's remember that this patch is transparent for all users, but without it, we'll be introducing an unnecessary limitation. How this may be used is better left to module developers.

moshe weitzman’s picture

moshe weitzman commented

This is a valid patch and is rightfully RTBC.

This page has plenty of valid use cases. For observer.com, editors use this page but they might not have administer nodes perm. Re.member, with fapi we can make pieces of node form available without administer nodes perm (like sticky, promoted, etc.).

db_rewrite_sql() is *NOT* needed whenvever you query node table. It is only needed when you retrieve a node listing (i.e. multiple nodes). This page does a node listing, so db_rewrite_sql() is appropriate here.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

OK, I have committed this fix, although simplified. We really don't need a comment saying "we apply db_rewrite_sql()" before we apply db_rewrite_sql. Similarly the intermediate variable was superfluous.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)