Move query_alter() into a preExecute() method

#440310: howto limit result sets using hook_query_TAG_alter()? showed that only the SelectQuery object is passed to the hook implementations, without any extenders, which means that you can't call these methods. Maybe this change would help with that too, I'm not sure though...

It probably would, as extenders could override preExecute() which would then pass the extender object rather than the bare object. That could even be done in the base class, I think.

This is stalled on #481288: Add support for INSERT INTO ... SELECT FROM ... going in, though, which is currently RTBC...

#481288: Add support for INSERT INTO ... SELECT FROM ... is fixed now. Anyone with a patch to review?

subscribe.

This patch adds a preExecute() to SELECT and MERGE queries to match INSERT queries, and moves any validation and preprocessing to those. UPDATE and DELETE don't really have anything to move to a preExecute() to I left it out for now.

Looks like a tiny change and all the tests pass, I don't really get dbtng internals well enough to RTBC though.

I had the same idea as moshe in #8, a flag in preExecute() so that it can be called only once. That would make it possible to call preExecute() in every Extender::execute(), so that it is called as early as possible but only once.

Anyone want to try and roll that change before I get to it?

Ok, this was harder than I thought... the problem is that even if preExecute() is called in the extenders, only the inner SelectQuery object is passed to the query alter hooks.

There are two ways to solve it, I'm not sure which one is better:

a) Optionally allow to pass an SelectQueryInterface object to preExecute()

b) Make the inner SelectQuery object aware of its "most upper" extender and always use that for hooks and maybe other things (?)

Attached is a patch for both approaches...

There is also some other stuff in the patch, namely a pager tag and a apidoc comment for TableSort.

Hint: Both approaches are tested to fix #440310: howto limit result sets using hook_query_TAG_alter()?

What about overriding preExecute() in the SelectQueryExtender base class with the exact same code? That way by default an extender always passes itself to the alter hook.

And how to we control that preExecute is only executed once then? Because we don't want the hooks to be executed for every single extender....

Edit: Erm, why does the outer patch have lesser test assertions than the other one? looks wrong to me...

I dunno on the second question. :-)

On the first question, if preExecute() is overridden and does not delegate (eg, call $this->query->preExecute()), then only the outer-most one will get called. They'll all be identical, presumably, so there should be no issue there.

Well the "outer" patch results in a circular reference between the query object and extender objects, so that's a no-go.

As for the param patch, Hm. Let me play with this for a bit.

OK, after playing with this some more and running through scenarios in my head, I like the parameter mechanism. The attached patch makes a few changes to Berdir's #11, so here's the full rundown of what it does now:

1) Adds a preExecute() method to all query types that have validation of some form, for consistency. They are all public, although it should be extraordinarily rare to call it directly. Even then, only on select queries.

2) For the Select version of preExecute(), we also handle the extenders. Extenders call preExecute() with the optional query parameter that tells preExecute() what object to process. For a normal select query, that is $this. For an extended query, the outer-most extender will get first-dbs and will call preExecute() with itself, that is, with the outer-most extending object. That means alter hooks will get called on the outer-most extender.

So here's what happens:

- If you call execute() on a select query, it first calls preExecute(), then does its thing.
- If you call execute() on an extender, it first calls its preExecute(), which is defined in the base class. That will in turn call its parent's preExecute() with itself, forcing the wrapped query to validate the extender. That's what we want to happen. That will then return and the extender will then call its parent's execute() method. That will try to call preExecute(), see that it's already been done, and just return. Then execute() does its thing as normal.
- For really weird edge cases like Moshe's FAPI caching patch, you can call preExecute() on the query object or extender directly, then cast the object to a string to get its prepared statement string form. When you then call execute(), the preExecute() logic above has already happened so will simply return without doing anything all the way up to the actual query execution, and all is right with the world.
- If you call preExecute(), then do additional changes to the query, then call execute(), there's a good chance your query will break. Don't do that. It's incredibly stupid. :-)

3) I don't know why Berdir added a tag to the pager extender, but I renamed it and added one to the tablesort extender too, just kinda because. :-)

Crell spent a good 20 mins or so in IRC walking me though this since it's a bit tweaky. I feel like this ought to be documented... somewhere. But I don't think it really makes sense in the PHPDoc of any particular function. So archiving in the issue for now:

Sample query:

$query = db_select('node', 'n')
  ->extend('PagerDefault')
  ->extend('TableSort')
  ->execute();

When you call execute(), Tablesort::execute() calls $this->preExecute($this); That is defined in the extender base class. SelectQueryExtenderBase::preExecute() checks that it has never run before. It hasn't, so it runs all of the validation and query_alter and such stuff, on the TableSort object.

It then returns, and TableSort::execute() does its thing, which ends with calling $this->query->execute(), which happens to be PagerDefault::execute(); PagerDefault::execute() runs $this->preExecute($this), which is again SelectQueryExtenderBase::preExecute(). It has already run, so it just returns and does nothing.

PagerDefault::execute() then does its thing, which ends with $this->query->execute(), which happens to be the SelectQuery object. SelectQuery::execute() calls $this->preExecute(), which as before has already run, so it does nothing. It then goes through the normal query execution process, and we're done.

Net result: query_alter hooks get passed whatever the outermost extender is, which is what we want.

Here's a review of the code itself, mostly minor stuff and some comment clarifications.

+++ includes/database/query.inc	2009-08-23 04:51:04 +0000
@@ -721,13 +721,27 @@ class MergeQuery extends Query {
+  public function execute() {
+    if (!$this->preExecute()) {
+      return NULL;
+    }

Hunks of code like this really make no sense, because that function is called preExecute() (which sounds like an optional thing) rather than something like preValidate() which does not sound like an optional thing. I know we already made this call in some other issue because this does more than validation, but we should add comments above these t the effect of "Do not proceed if validation fails." because otherwise it's super confusing.

+++ includes/database/select.inc	2009-08-23 04:39:21 +0000
@@ -501,7 +514,25 @@ class SelectQueryExtender implements Sel
+  public function preExecute(SelectQueryInterface $query = NULL) {
+  	// If no query object is passed in, use $this.

Indentation is off on the comment.

+++ includes/database/select.inc	2009-08-23 04:39:21 +0000
@@ -501,7 +514,25 @@ class SelectQueryExtender implements Sel
+    // Call preExecute to allow access to the Extender in hook_query_alter().
+    if (!$this->preExecute($this)) {
+      return NULL;
+    }
+++ includes/pager.inc	2009-08-23 04:35:55 +0000
@@ -52,6 +60,11 @@ class PagerDefault extends SelectQueryEx
+    // Call preExecute to allow access to the Extender in hook_query_alter().
+    if (!$this->preExecute($this)) {
+      return NULL;
+    }

I believe this comment refers to the 20 minutes you spent in IRC talking me through this code. ;) It'd be nice to give the next person a little bit more of a clue what's happening, since this comment by itself doesn't give me enough context to understand why you thought it was important enough to leave such a comment.

+++ includes/pager.inc	2009-08-23 04:35:55 +0000
@@ -43,6 +43,14 @@ class PagerDefault extends SelectQueryEx
+    // Add pager tag. Do this here to ensure that it is always added before
+    // preExecute() is called.
+    $this->addTag('pager');
+++ includes/tablesort.inc	2009-08-23 04:36:23 +0000
@@ -22,10 +22,20 @@ class TableSort extends SelectQueryExten
+    // Add tablesort tag. Do this here to ensure that it is always added before
+    // preExecute() is called.
+    $this->addTag('tablesort');

Um. Why? :)

+++ includes/tablesort.inc	2009-08-23 04:36:23 +0000
@@ -22,10 +22,20 @@ class TableSort extends SelectQueryExten
+   *   Table header array, @see theme_table()

Move @see to its own line.

I'm on crack. Are you, too?

Hi from Vancouver Island :)

Um. Why? :)

dynamic hook_query_TAG_alter hooks are called based on these tags, so they need to be set before preExecute() is called. However, if we are in execute() it is possible that it has already been called by another extender. So we add the Tag in the constructor. It would also be possible to make this generic and add a $this->addTag(strtolower($extender_name)) in SelectQueryExtender::__construct(), I think...

Tweaked various comments per #18. No code changes, so setting back to RTBC.

Ok, awesome. Thanks for the improved documentation. Committed to HEAD.