As discussed in #314532: DBTNG comment.module, should we change the auto-aliasing logic that addField() uses? Currently if no alias is specified the default is $table_$field. Regardless, it will then check for a duplicate and append a counter to the alias name if necessary.
Since in most cases the field name is not a duplicate, should we make the default just $field, and if that is already in use then use $table_$field? That would simplify conversion as we wouldn't have to rename every single field when referenced and/or alias every single field when specified.
Thoughts?
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | select_alias.patch | 2.28 KB | Crell |
Comments
Comment #1
merlinofchaos commentedThe $table_$field bit was initially modelled after Views, but the situation here is a little different. Views queries really expect to have a lot of different unrelated hands poking at them, so $table_$field helps prevent collisions. In Drupal core, $table_$field does seem like a burden. Also, Views 2 cares a lot less about what the alias actually is, since it's a lot better at saying "add a field and tell me what the alias was". That's the critical part from the Views perspective: 1) it will always tell me the actual alias used and 2) it will always alias.
Comment #2
CorniI commentedI'm definitly for defaulting the alias to $field, as I wrote in #314532: DBTNG comment.module. The return of addField can't be used really in core, as eg lot's of different places just expect the layout of a $comment-object, and they don't mind if there are aliases set, they'll just access a (maybe nonexistant) variable.
Comment #3
Crell commentedWell, as the docs say in that case it's better for them to not presume what the alias will be but simply use the value returned from addField() as the field name. That way even if the alias handling changes they will continue to work. Code that's doing that will work fine with a shorter default, however, so yeah, let's go ahead and change the default logic as described in the OP. ($field, then $table_$field, then $table_field_n)
Comment #4
Crell commentedAnd here's the patch to do that, including unit test updates for it.
Comment #5
drewish commentedsubscribing
Comment #6
dave reidPatch makes sense, looks good, and applies with just a little fuzz. Had a few node revision/path translation test failures and exceptions, but they all happened on current HEAD as well, no fault to this patch.
Comment #7
webchickCommitted to HEAD. Thanks!
Comment #8
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.