[drupal-devel] [bug] generalizing node_rewrite_sql

chx drupal-devel at drupal.org
Sat Feb 5 00:04:34 UTC 2005 

 Project:      Drupal
 Version:      cvs
 Component:    database system
 Category:     bug reports
 Priority:     normal
 Assigned to:  chx
 Reported by:  chx
 Updated by:   chx
 Status:       patch

To my best knowledge, the biggest hurdle this patch faces is the sad
fact that the power supply of Dries' loved developer notebook died...
Have patience, please.


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.


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

January 30, 2005 - 19:17 : chx

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.


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

January 30, 2005 - 19:58 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_fix.patch (5.11 KB)

As Jose pointed out, node_db_rewrite_sql is wrong, it shall check for
primary_field not for primary_table. Also, primary_field is better name
than primary_key.
Previous patch is also included in this one.


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

January 31, 2005 - 20:29 : Dries

This patch does not apply against HEAD: 2 out of 3 hunks FAILED --
saving rejects to file includes/database.inc.rej.


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

January 31, 2005 - 20:40 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_fix_0.patch (5.12 KB)

:(


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

February 1, 2005 - 19:46 : Dries

Committed to HEAD.


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

February 3, 2005 - 16:01 : chx

Attachment: http://drupal.org/files/issues/db_rewrite_bugfix.patch (739 bytes)

Steef stumbled on another bug.


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

February 3, 2005 - 16:10 : stefan nagtegaal

Indeed, after applying the patch the node_* access modules works again
without spitting errors and other things...
Please apply this patch to HEAD...


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

February 4, 2005 - 23:46 : mathias

Indeed +1
It's difficult to do any node-level permissions work when Drupal's
throwing 'Fatal errors' because of syntax errors in database.inc.


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

More information about the drupal-devel mailing list