Make it easier to add multiple fields in a dynamic SELECT

You can use addExpression('*'). But the true answer is that you should specify the column names one by one: even if it's "tedious and repetitive", it makes code more readable and less error-prone.

Also, note there is a feature request that would make this a little less repetitive (#316868: Default SelectQuery::addField() alias to $field, not $table_$field).

I like this: tableSelectQuery::addAllFields($table_alias). Internally, the method would do roughly as drewish suggests

foreach (drupal_schema_fields_sql('files') as $field) {
  $query->addField('f', $field, $field);
}

I agree that there is a DX issue here.

#3 would get the cleaner SQL, but would still not be at all self-documenting. I can't tell from the code what fields I'm going to end up with. I also want to minimize schema usage wherever possible for performance reasons.

As discussed in IRC, IMO the solution is not to make SELECT * work but to make it easier to add multiple fields in one shot. SELECT * is bad for SQL performance, with addFields() or similar.

@Crell - your critique in #4 does not make much sense to me. You don't know what fields you will get back in exactly the way with SELECT *. Thats a feature, not a bug. Sometimes you do not care. I agree that listing the fields is preferable to SELECT *, and the call to schema gets you that. Note that schema is in static cache so only the first call costs anything. If that first call is still bothersome, consider allowing SELECT * :)

I think this is just one of those conveniences like fetchAllAsssoc that you use when you need it and when you know what the tradeoffs are.

SELECT * is the Enemy of Decent Code. This is a documentation problem (ie I need to google together the articles on this) and not a code one. That said, I think menu has a SELECT * but that's rather easy to fix.

How many of those would actually require a dynamic query? :-)

Hey, I'm leading enough charges as is. :-)

However, attached patch adds an addFields() method, as discussed in IRC, that makes it much easier to queue up multiple fields from the same table at the same time. (Vis, it's one line.) It returns an array of aliases. (We probably do want to get #316868: Default SelectQuery::addField() alias to $field, not $table_$field in soon.)

If we're going to add "SELECT *" support to the dynamic query builder, my recommendation would be to allow NULL as the second parameter to addFields() which would result in an expression of "tablename.*", with no alias, and then no alias returned. (I'm sure I can come up with some way to make that work.) I defer to D&A on that one.

Could we consolidate addFields method and addField method? Seems like the one method could just behave differently if an array is passed. Seems like a gotcha to have both.

@13 something like the folllowing?

  public function addField($table_alias, $field, $alias = NULL) {
    if (is_array($field)) {
      foreach ($field as $a_field) {
        $this->addField($table_alias, $_afield);
      }
    }
    ....

I'm wary about consolidating too many things into the same method, especially when they will then be returning different things. (A string alias, an array of aliases, or nothing if we're doing a SELECT *.) Most of the rest of the DB API is atomic methods, at least on the return side.

Per discussion with webchick in IRC tonight, the following is the game plan (patch still to come):

- addField() remains as it is now.

- we add addFields(), which takes either addFields($table, $array_of_fields), which returns the query so it's chainable or addFields($table), which turns into $table.*. In this case, if you need to know the alias that was used you have to use getFields() to access the data structure directly and figure it out yourself.

- The aliasing logic will remain the same, modulo #316868: Default SelectQuery::addField() alias to $field, not $table_$field which we agreed was a good thing.

Revised patch for the changes to point 2 forthcoming.

Here we are again, as described in #16 with one change. Since we're now dealing with multiple fields, I went ahead and called the method fields() instead of addFields(). That also makes it not quite so annoyingly similar to addField(). A unit test with a fully chained query is also included. (Note that join() still cannot be chained, but we'll deal with that later.)

Note that this patch is dependent on #316868: Default SelectQuery::addField() alias to $field, not $table_$field, since you only get the default alias back. Since we know that one is going in I didn't bother to write the test for the old aliases. That means the unit test will not pass unless you apply the other patch first, or that gets committed. Committers, commit it, please! :-)

Oops. I forgot about SELECT *. :-)

Attached patch supports ->fields($tablename) to convert to SELECT $tablename.*. It does so by flagging the $tables array that the query should select everything from that table. That keeps the changes minimal and also keeps it exposed to alter hooks.

Of note, we are now iterating $tables twice in __toString(). While we could optimize that into a single loop, it would make the code less compact. It would also conflict with #299178: Add support for subqueries in FROM and JOIN clauses in dynamic query, which also mucks with that loop. For now let's keep the separate loops. Once all relevant patches are in we can decide if it's worth merging them in a follow-up.

The SELECT * unit test is *not* dependent on the field aliasing patch, as there is no aliasing at all for SELECT *. The one for explicit fields will still fail without the aliasing patch.

Are these test results right: http://testing.drupal.org/pifr/file/1/3 ?

8 fails, 8 exceptions with Select tests.

Checking testing.drupal.org.

That's correct, due to the fact that testing.drupal.org doesn't have the other dependent patch applied to it yet. :-) See comments in #16, 17, and 18.

Thanks.

The other patch just landed, so this one should pass all tests now. Can someone RTBC this so we can get back to converting core? :-)

I suspect it's your system. I just did a cvs update, applied this patch, and ran the entire suite of DB tests and everything passed perfectly. (Modulo a little offset.)

Code looks good. Confirmed that the tests pass. RTBC.

This is one of the prerequisites for progressing with #299176: Replace db_rewrite_sql() with hook_query_alter()..

OK looked this over, and apart from some double spacing in the comments which I already gave him crap about (and core is too inconsistent to rule one way or the other -- must make a crusade for that ;)), this looks good. I noticed that testSimpleSelectConditional() could use some minor love now after recent DBTNG commits, but Crell opted to handle this in a separate issue.

Committed to HEAD. Thanks! Marking CNW until the docs are updated.

Docs have been updated: http://drupal.org/node/310075

And there was much rejoicing!