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.

CommentFileSizeAuthor
#22 db_rewrite_bugfix.patch739 byteschx
#20 db_rewrite_fix_0.patch5.12 KBchx
#18 db_rewrite_fix.patch5.11 KBchx
#17 db_rewrite_empty_fix.patch2.88 KBchx
#15 db_rewrite_sql_docfix.patch1.77 KBchx
#12 database.inc.docfix.patch1.77 KBchx
#10 db_rewrite_sql_2.patch38.67 KBchx
#9 db_rewrite_sql_1.patch37.83 KBchx
#5 db_rewrite_sql_0.patch7.68 KBchx
db_rewrite_sql.patch7.66 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Dries CreditAttribution: Dries commented

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?

Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
Title: generalizing node_rewrite_sql » +1

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.

JonBob’s picture

JonBob CreditAttribution: JonBob commented
Title: +1 » generalizing node_rewrite_sql
Jose Reyero’s picture

Jose Reyero CreditAttribution: Jose Reyero commented
chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_sql_0.patch7.68 KB

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

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

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/

chx’s picture

chx CreditAttribution: chx commented

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.

chx’s picture

chx CreditAttribution: chx commented

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

chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_sql_1.patch37.83 KB

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

chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_sql_2.patch38.67 KB

And here is a version against current CVS.

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD.

chx’s picture

chx CreditAttribution: chx commented
FileSize
database.inc.docfix.patch1.77 KB

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Set to patch then...

Dries’s picture

Dries CreditAttribution: Dries commented

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.

chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_sql_docfix.patch1.77 KB

So here it is.

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD.

chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_empty_fix.patch2.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’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_fix.patch5.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.

Dries’s picture

Dries CreditAttribution: Dries commented

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

chx’s picture

chx CreditAttribution: chx commented
FileSize
db_rewrite_fix_0.patch5.12 KB

:(

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to HEAD.

chx’s picture

chx CreditAttribution: chx commented
Category: feature » bug
FileSize
db_rewrite_bugfix.patch739 bytes

Steef stumbled on another bug.

Stefan Nagtegaal’s picture

Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commented

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

matt westgate’s picture

matt westgate CreditAttribution: matt westgate commented

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.

chx’s picture

chx CreditAttribution: chx commented

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.

Steven’s picture

Steven CreditAttribution: Steven commented

Applied to CVS.

Anonymous’s picture

(not verified) CreditAttribution: commented