[drupal-devel] [feature] generalizing node_rewrite_sql

Sun Jan 30 18:17:16 UTC 2005

 Project:      Drupal
 Version:      cvs
 Component:    database system
 Category:     feature requests
 Priority:     normal
 Assigned to:  chx
 Reported by:  chx
 Updated by:   chx
-Status:       fixed
+Status:       patch
 Attachment:   http://drupal.org/files/issues/db_rewrite_empty_fix.patch (2.88 KB)

While writing term_node and vocabulary rewrites for taxonomy module
(patch is under testing, will post this evening), I've unearthed a
nasty bug with this.
If $where is empty -- which did not happened with n type rewrites, 'cos
node_access_where_sql always returns at least '1' -- then the resulting
SQL will be invalid, there will be an empty AND clause.
While I was at it, I made the whole thing faster -- replacements are
only done when they are needed.

chx

Previous comments:
------------------------------------------------------------------------

January 24, 2005 - 19:58 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_sql.patch (7.66 KB)

As I was thinking on the various usages of node_rewrite_sql , it came to
my mind that with very little changes it can be generalized so that any
queries against any table -- not just node -- can be rewritten. I have
moved node_rewrite_sql to database.inc, renamed it db_rewrite_sql,
introduced another necessary parameter, which does not affect the
current (light) usage of node_rewrite_sql. I think the usage is even
clearer now, 'cos I think nid_alias caused some confusion. Now it is
called $primary_table , while the new parameter is called $primary_key.
What this would be good for? One may want to have permission and
languages for taxonomy terms and vocabularies.

------------------------------------------------------------------------

January 24, 2005 - 23:08 : Dries

I guess it makes sense to go with a generic approach, though it is
difficult to predict how many queries will end up being wrapped in a
call to db_rewrite_sql() ...  Comments?

------------------------------------------------------------------------

January 24, 2005 - 23:28 : Jose A Reyero

I like it!, though I still miss some more info about what the query is
about -the 'hints' stuff- , but it's fine... :-)
But also think there should be some kind of 'guidelines' about wrapping
specific queries or not. For the moment, my candidates are:
- node queries, which means 'queries that select lists of nodes', not
any query which contains 'node'
- taxonomy queries, same here, 'queries that select lists of
vocabularies/terms'
Both types of queries above are good candidates for joining permissions
conditions, and maybe language conditions (i18n).
And maybe more in the future.... we'll see.

------------------------------------------------------------------------

January 24, 2005 - 23:41 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_sql_0.patch (7.68 KB)

A through test showed that introducing the $primary_key parameter was
done incorrectly, thus the new version.

------------------------------------------------------------------------

January 24, 2005 - 23:43 : moshe weitzman

I'm confusd about who is responsible for calling the rewrite function.
Does it not make sense to run every query through this system (and thus
insert a call into db_query). We already do this for prefixes. perhaps
the hook is too performance draining?
Note that the rewrite sql() hook is not documented on the hooks page:
http://drupaldocs.org/api/head/group/hooks. One would document it by
editing
http://cvs.drupal.org/viewcvs/contrib/contributions/docs/developer/hooks/

------------------------------------------------------------------------

January 25, 2005 - 00:00 : chx

There have been talks on #drupal about automatizing, and even the
simpler node_rewrite_query was found to be better called by hand. But
this? Automatic mechanism for this -- I think -- is almost impossible.

------------------------------------------------------------------------

January 25, 2005 - 20:54 : chx

I feel some cold coming... yes, it's the code freeze approaching. Please
help this patch into core :)

------------------------------------------------------------------------

January 27, 2005 - 20:34 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_sql_1.patch (37.83 KB)

Here is the patch with search-and-replace done.

------------------------------------------------------------------------

January 29, 2005 - 18:49 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_sql_2.patch (38.67 KB)

And here is a version against current CVS.

------------------------------------------------------------------------

January 29, 2005 - 23:03 : Dries

Committed to HEAD.

------------------------------------------------------------------------

January 30, 2005 - 10:41 : chx

Attachment: http://drupal.org/files/issues/database.inc.docfix.patch (1.77 KB)

Documentation fix (and the doc is a bit nicer, too). There is no code
change.

------------------------------------------------------------------------

January 30, 2005 - 14:20 : Goba

Set to patch then...

------------------------------------------------------------------------

January 30, 2005 - 17:51 : Dries

Because of a bug in the project module, you can't use '.inc' in the name
of your patch.  Please rename and resubmit your patch.

------------------------------------------------------------------------

January 30, 2005 - 17:57 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_sql_docfix.patch (1.77 KB)

So here it is.

------------------------------------------------------------------------

January 30, 2005 - 18:49 : Dries

Committed to HEAD.

-- 
View: http://drupal.org/node/16111
Edit: http://drupal.org/project/comments/add/16111