[drupal-devel] node_access_where_sql()
I'm debugging an issue in the project module on Drupal cvs. Most node queries use the node_access_where_sql() function like so. "SELECT foo FROM bar WHERE " . node_access_where_sql() . " AND criteria = true" However, the first test in the function returns nothing which will obviously result in an invalid query. Shouldn't the function return a value of "0" or something so that queries will always be valid? http://drupaldocs.org/api/head/function/node_access_where_sql
On Sun, 20 Feb 2005, Chris Cook wrote:
I'm debugging an issue in the project module on Drupal cvs.
Most node queries use the node_access_where_sql() function like so.
"SELECT foo FROM bar WHERE " . node_access_where_sql() . " AND criteria = true"
However, the first test in the function returns nothing which will obviously result in an invalid query. Shouldn't the function return a value of "0" or something so that queries will always be valid?
http://drupaldocs.org/api/head/function/node_access_where_sql
MAybe some docs need to be updated. We now use db_rewrite_query. See current code for examples. Cheers, Gerhard
On Feb 21, 2005, at 1:47 AM, Gerhard Killesreiter wrote:
On Sun, 20 Feb 2005, Chris Cook wrote:
Most node queries use the node_access_where_sql() function like so.
"SELECT foo FROM bar WHERE " . node_access_where_sql() . " AND criteria = true"
However, the first test in the function returns nothing which will obviously result in an invalid query. Shouldn't the function return a value of "0" or something so that queries will always be valid?
MAybe some docs need to be updated. We now use db_rewrite_query. See current code for examples.
That function used to return "1", but the node access performance patch changed this since db_rewrite_query doesn't need this. It was an accident that it breaks older modules. The question is: do I roll a patch to revert this behavior (so older modules would work, but be using a deprecated and slower API), or do we just document it as a necessary 4.5 -> 4.6 module change?
On Mon, 21 Feb 2005 08:36:32 -0500, Jonathan Chaffer <jchaffer@structureinteractive.com> wrote:
The question is: do I roll a patch to revert this behavior (so older modules would work, but be using a deprecated and slower API), or do we just document it as a necessary 4.5 -> 4.6 module change?
We could just rename the other functions _node_accesss_*_sql if they are not intended to be used by external functions anymore. The function calls will have to be updated anyway unless the return is reverted.
We could just rename the other functions _node_accesss_*_sql if they
Sounds like a good idea to me. db_rewrite_sql had its own problem with empty WHEREs in the past, the second and last (I hope at least) of this kind was fixed yesterday. Just sent a handbook update to Killes... at least I wanted to, instead I sent it to the whole list, sorry. NK
Jonathan Chaffer wrote:
On Feb 21, 2005, at 1:47 AM, Gerhard Killesreiter wrote:
On Sun, 20 Feb 2005, Chris Cook wrote:
Most node queries use the node_access_where_sql() function like so.
"SELECT foo FROM bar WHERE " . node_access_where_sql() . " AND criteria = true"
However, the first test in the function returns nothing which will obviously result in an invalid query. Shouldn't the function return a value of "0" or something so that queries will always be valid?
MAybe some docs need to be updated. We now use db_rewrite_query. See current code for examples.
That function used to return "1", but the node access performance patch changed this since db_rewrite_query doesn't need this. It was an accident that it breaks older modules.
The question is: do I roll a patch to revert this behavior (so older modules would work, but be using a deprecated and slower API), or do we just document it as a necessary 4.5 -> 4.6 module change?
Actually this broke the node.module's hook_search as it calls _db_rewrite_sql() directly. I already patched this, but perhaps we should consider going back to the old behaviour. Steven Wittens
Actually this broke the node.module's hook_search as it calls _db_rewrite_sql() directly. I already patched this, but perhaps we should consider going back to the old behaviour.
Calling _db_rewrite_sql is a big no-no! I did it 'cos I was unable to do solve node_search rewrite by the actual wrapper, but it is was a rare exception. In the future, if anyone else does so, will see that after both calls to this helper we have a check for empty($where) so he can act accordingly. So this shall not set move us back to the old behaviour. Still my vote is on renaming to _node_access_*_sql.
participants (5)
-
Chris Cook -
Gerhard Killesreiter -
Jonathan Chaffer -
Negyesi Karoly -
Steven Wittens