Replace db_rewrite_sql() with hook_query_alter().

Here they are:
http://api.drupal.org/api/function/db_rewrite_sql/7/references

subscribe

New plan, in the correct order:

1) Implement node_query_alter() (a DISABLED_node_query_alter() is in, but is marked as "don't work yet")
2) Convert all db_rewrite_sql() that rewrite node queries using the new query builder, and tag them with 'node_access'
3) Remove node_db_rewrite_sql() (yeah!!)
4) Think of a plan for other rewritten queries (taxonomy, ...)
5) Execute the plan defined in 4
6) Remove db_rewrite_sql() (re-yeah!!)

7) Profit! ;)

I greatly look forward to committing this patch when it's ready. :D

Subscribing.

OK, I'm marking this needs review to get some attention. :-) I have what I think is a working node_query_alter() implementation, at least from reading the existing code. However, I really don't know how best to test it. I've taken a stab at starting the relevant unit tests, but this will absolutely require input from one of the 4-5 people who actually grok node access in the first place. If you are one of them, please jump in and see if you can confirm (with tests) that my code actually does something useful.

I was just thinking about this. i will review and give some feedback.

In order to test this, it is best IMO to upgrade http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/exam... to use the new API.

I committed an update for that node_access_example module so it can be used in HEAD to generate sensible node access records ... I hope to look at this patch soon.

Is there an example out there of of a query that used to do pager_query() but now does dynamic select? If not, could you give some pointers or even just paste in a converted query from node_page_default() ... I am assuming that we must do dynamic in order to add a tag.

Also, I would like to remove the generic node_query_alter hook and instead go to the specific ones that form_alter does like hook_form_alter_[form_id]. we would do hook_query_alter_[tag].

All pager queries will eventually end up dynamic, but I need to take care of #299267: Add "Extender" support to SELECT query builder first as that's how I plan to handle pager and tablesort queries. Yes, tagging only works on dynamic queries. However, paging and node_access should be independent of each other, shouldn't they?

I considered splitting out query_alter a la FAPI, but we would have to keep at least the common one. If not, then it would be impossible (or at least very very difficult) to operate on queries based on multiple tags (which are supported). I left the split version out of the original patch mostly for simplicity, but I'm fine with adding it, just not removing the base one.

Yes paging and node access are independant but node access queries are always node listings by definition. Each one that I can find is either a pager_query or a db_query_range(). I don't know how to add a tag to those.

With Crell's help, I have worked though the bugs in the previous patch and converted one forum block to use the new node access system. It is going to be a biggish job to convert all the other node listing queries to be dynamic queries so I'd prefer to have that happen in a follow up patch. Also, there are some patches in the queue that will make those queries simpler (e.g. addFields method).

The forum block does not use node_title_list() since I could not see how to use it now that the query builder can alias our fields, willy nilly.

There is a small bug fix to the DB API - added missing parens.

Actually, we can use node_title_list() as is. Simpler patch attached.

This patch has no dependencies so feel free to commit it once it is RTBC :)

In order to easily convert the rest of the node listing queries, we ought to get these in:

• #320591: Tag specific alter hook
• #299267: Add "Extender" support to SELECT query builder
• #318440: Make it easier to add multiple fields in a dynamic SELECT

subscribe

The bug fix for nested conditions was committed separately (#331737: Nested conditions don't work), so this won't apply now.

Reroll

I think we are good to go, the patch is rather simple. Yay db_rewrite_sql dies.

While this patch is relatively easy indeed, I'd still like to get a better handle on how other queries would get converted. Therefore, I'm going to investigate #320591: Tag specific alter hook and #299267: Add "Extender" support to SELECT query builder some more first to see if that helps me wrap my head around that question.

@Dries: Here's the short version, if I can manage that... :-)

hook_query_alter() is intended, primarily, to replace db_rewrite_sql() and hook_views_query_alter(). Anywhere we previously would use db_rewrite_sql() you'd now use a dynamic query and add a node_access tag to it. node_query_alter() does almost exactly the same thing as node_rewrite_sql(), but in a structured fashion rather than a hacky string parsing. We can then potentially do other things elsewhere if people come up with them, but that's the main use case. (And I will honestly defer to Moshe on the exact implementation, as I still don't fully grok node_access. :-) )

The tag-specific alter hook makes no change to that. It's just a minor performance/usability improvement with the exact same concept as hook_form_*_alter(). All arguments for that apply here as well.

Extenders are an entirely different animal. They're for cases where you need additional functionality on a select query that is controlled at the point you're creating the query, whereas hook_query_alter() is for modifying a set of queries in some standard way from elsewhere in Drupal. They're really unrelated sorts of functionality.

Committed to CVS HEAD. Thanks.

Woohoo! Thanks, Dries!

Moshe, can you update the handbook pages if necessary? I know we had discussed what the tag structure would be, and as I said you know the implementation better than I.

http://drupal.org/node/310077

Those handbook docs need no further updates IMO. The upgrade docs need to mention this but we really need to commit #320591: Tag specific alter hook and #299267: Add "Extender" support to SELECT query builder to do a good job explaining the conversion process.

I've added an item to the upgrade docs. Once we have all of the various subystems in place we can add a dedicated handbook page for how to update the most complex case (db_rewrite_sql(), paged, with tablesort) to Drupal 7. Let's continue those threads in their own threads.

Automatically closed -- issue fixed for two weeks with no activity.

hook_query_alter() needs an example in the docs.

OK, fine, here's your docs. :-)

The last submitted patch failed testing.

What syntax error? Stupid bot...

The last submitted patch failed testing.

Third time's the charm.

Reposting for bot's sake.

Committed to CVS HEAD. Thanks.

Don't know if you noticed but there's a typo in this patch

Seems like this should be td.tid.

@RandalK The original patch that has the typo was committed quite a bit ago. Can you check core and make sure the typo still exists and open up a new issue maybe referencing this issue?

It is still present in forum_block_view():

I'm assuming that there are no tests for that block, or it would have been caught. Probably not as part of this patch, but adding a test would be nice :)

Committed to CVS HEAD. I'm marking this 'code needs work' because we don't have a test yet for the forum block.

Created a separate issue in forum.module for the followup and now we can mark this as fixed.
#440344: Tests needed for forum blocks

Automatically closed -- issue fixed for 2 weeks with no activity.