Weird logic in SelectQuery::addField() [#851748]

  public function addField($table_alias, $field, $alias = NULL) {
    // If no alias is specified, first try the field name itself.
    if (empty($alias)) {
      $alias = $field;
    }

    // If that's already in use, try the table name and field name.
    if (!empty($this->tables[$alias])) {
      $alias = $table_alias . '_' . $field;
    }

    // If that is already used, just add a counter until we find an unused alias.
    $alias_candidate = $alias;
    $count = 2;
    while (!empty($this->tables[$alias_candidate])) {
      $alias_candidate = $alias . '_' . $count++;
    }
    $alias = $alias_candidate;

    $this->fields[$alias] = array(
      'field' => $field,
      'table' => $table_alias,
      'alias' => $alias,
    );

    return $alias;
  }

Shouldn't those !empty($this->tables[xxx]) be !empty($this->fields[xxx])??

Comment	File	Size	Author
#3	851748-change-tables-to-fields.patch	2.11 KB	Stevel
#3
#2	851748-change-tables-to-fields.patch	0 bytes	Stevel
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Crell CreditAttribution: Crell commented 12 July 2010 at 16:09

Um. It certainly looks like they should be. I'm amazed it hasn't broken by now. Perhaps a unit test will resolve the matter?

Comment #2

Stevel CreditAttribution: Stevel commented 20 July 2010 at 19:08

Status:

Active

» Needs review

File	Size
851748-change-tables-to-fields.patch	0 bytes

Attached a patch that makes the change DamZ described in the issue description, and added a small test that verifies that the alias is renamed when there are duplicates, which fails without the changes in addField().

I guess it's quite uncommon to explicitly add the same alias to more than one field, and if you select a field with identical names from multiple tables, it's easier to provide a unique alias yourself than using a generated one in the rest of your code. I think this could explain why this hasn't broken anything yet.

Perhaps it's worth revisiting this for D8 and just make the query writer responsible for using sensible aliases (and then it's possible to make the addField()-function chainable).

Comment #3

Stevel CreditAttribution: Stevel commented 20 July 2010 at 19:09

File	Size
851748-change-tables-to-fields.patch	2.11 KB

Hmm, zero-byte patch isn't good. This should be better...

Comment #4

Crell CreditAttribution: Crell commented 20 July 2010 at 19:14

The dynamic aliasing is necessary for highly dynamic queries such as Views and safety in alter hooks. I would like to make SelectQuery more chainable in D8, though.

Code looks good to me on visual inspection, but we'll wait for the bot before sending it upstairs.

Comment #5

Crell CreditAttribution: Crell commented 20 July 2010 at 22:22

Status:

Needs review

» Reviewed & tested by the community

Comment #6

Dries CreditAttribution: Dries commented 28 July 2010 at 01:51

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks.

Comment #7

Berdir

German

Switzerland

CreditAttribution: Berdir commented 28 July 2010 at 07:51

In most cases, we don't use that feature but there are a few occasions (even in core), where we do. AFAIK not on fields, but on joins. Filter queries are an option, see for example http://api.drupal.org/api/function/user_build_filter_query/7, there can be multiple joins to the permissions table.

Anyway, I agree that in most cases, this isn't required and it's also very inconsistent. Maybe we can find a way to make that consistent in D8 (Dunno, maybe all add* methods are not chainable and we introduce field(), expression() etc. in D8 that are. )

Comment #8

JacobSingh CreditAttribution: JacobSingh commented 28 July 2010 at 18:04

Hey, just FYI this patch broke comment notify yesterday.
#867534: Install broken since core change to DB API
Comment notify tried to add the same field twice (accidentally) like:

$query->field('c', 'cid');
// other stuff
$query->field('c', 'cid');

This manifested as a bizzare SQL error looking for the field c_cid which of course didn't exist. Not sure if the patch authors feel this deserves some error checking, docs or just RTFM, but it certainly wasn't easy to find the cause even having updated core and knowing it was one of 15 recent commits.

Comment #9

Damien Tournoud CreditAttribution: Damien Tournoud commented 28 July 2010 at 18:18

@JacobSingh: that looks like a legitimate mistake in Comment Notify. The second addition of the same field rightly generated an alias, which makes the db_insert() query below fail. That looks by design.

Comment #10

Crell CreditAttribution: Crell commented 28 July 2010 at 19:58

Damien is correct. That sounds like one of the cases where the auto-aliasing is needed, or would be. addField() was just not following its own documented API in the first place before this patch.

Sounds like a bug fix in the DB API found a bug in Comment Notify. :-)