Fixed: Make `path_autocomplete()` alterable queries more robust against column ambiguity.

[prevoj]

path_autocomplete fails when acl, forum, and forums_access are installed. You get the following error on any field where path_autocomplete is called to provide suggestions. You receive the following log error:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'name' in where clause
is ambiguous:
SELECT DISTINCT taxonomy_term_data.`tid` AS `tid`, taxonomy_term_data.`vocabulary`
AS `vocabulary`, taxonomy_term_data.`name` AS `name` FROM {taxonomy_term_data} taxonomy_term_data
LEFT OUTER JOIN {forum_access} fa ON fa.tid = taxonomy_term_data.tid LEFT OUTER JOIN {acl} acl
ON acl.number = taxonomy_term_data.tid AND acl.module = 'forum_access' LEFT OUTER JOIN {acl_user} aclu
ON aclu.acl_id = acl.acl_id WHERE (`name` LIKE :db_condition_placeholder_0 ESCAPE '\\')
AND( (`fa`.`rid` IS NULL ) OR (`fa`.`grant_view` >= :db_condition_placeholder_1)
OR (`aclu`.`uid` IS NOT NULL ) )
ORDER BY tid DESC LIMIT 8 OFFSET 0; Array ( [:db_condition_placeholder_0] => %community%
[:db_condition_placeholder_1] => 1 ) in path_autocomplete()
(line 4226 of backdrop/core/modules/system/system.module).

The problem is that taxonomy_term_data and acl are joined in the query, they both have a column named 'name', and an unqualified 'name' is used in the where clause.

To reproduce the behavior:

1. Install and enable acl, forum, and forum_access on Backdrop.
2. Go to Configuration > URL Handling > URL Aliases > Add URL Alias.
3. Type a character in the "System existing path" input and wait.
4. The autocomplete will trigger and you get an error.

I was able to fix this problem in Backdrop 1.22.0 by editing my core/modules/system/system.module and changing line 4222 from:

->condition('name', '%' . db_like($string) . '%', 'LIKE') to: ->condition('taxonomy_term_data.name', '%' . db_like($string) . '%', 'LIKE')

After adding the table name, it prevents the ambiguous SQL 'name' column error and I get suggestions like you would expect for the path.

I know acl, forum, and forum_access appear to be pretty old and I plan long term to look at doing it the forum.backdropcms.org way. I like simple and using mostly core features when I can but I'm porting a Drupal 7 site and need to rely on the old forum module for now. Plus 'name' is a pretty generic column name so it would probably be good to qualify it in the where clause anyway.

Additional information

• Backdrop CMS version: 1.22.0
• ACL version: 1.x-1.0.0
• Forum version: 1.x-1.0.2
• Forum Access version: 1.x-1.4.2

[indigoxela]

Hi @prevoj and welcome to Backdrop.

Nice catch! Both tables, taxonomy_term_data and acl have a "name" column, leading to the ambiguity in the query. No such overlap with, for example, node or file. Only terms seem to be affected.

The suggestion to simply add the table name to the core query to prevent such overlaps seems reasonable to me, but I'm no expert with the database abstraction layer.

[indigoxela]

I took the time to reproduce and can confirm the problem. It doesn't exist in Drupal, as the "Existing system path" field doesn't have autocomplete turned on. I opened a separate issue for that.

However, I see a remaining problem here. The queries in function path_autocomplete(), that use addTag - which provides the ability to alter the query - should take extra care, as other tables might have same column names.

[indigoxela]

A PR is available for testing and review.

It simply adds table aliases, which prevents the problem, for both queries in function path_autocomplete() which use addTag.

[indigoxela]

PR rebased.

@prevoj maybe you want to test?

[klonos]

I haven't tested yet - just quickly reviewed the code, and it LGTM 👍🏼 ...thank you @indigoxela 🙏🏼

[klonos]

...now also tested:

1. on a tugboat demo site (currently 1.22.2), I've installed and enabled acl/forum/forum_access (also auto-installed chain_menu_access as a dependency)
2. navigated to /admin/config/urls/path/add
3. typed a character in the "Existing system path" autocomplete field -> 👎🏼
4. navigated to /admin/reports -> saw the error reported by @prevoj

I then tested this on the PR sandbox:

1. I've installed and enabled acl/forum/forum_access
2. navigated to /admin/config/urls/path/add
3. typed a character in the "Existing system path" autocomplete field -> no visible errors, no errors in the log either, and I was able to add an alias successfully 👍🏼

[klonos]

...this is RTBC 👍🏼 ...now we only need a milestone set @indigoxela (or anyone else).

[argiepiano]

I am a bit concerned about the sudden introduction of a new alias for the taxonomy_term_data in path_autocomplete. My concern is that some contrib module that implements hook_query_alter or hook_query_TAG_alter may be expecting to use the default alias (taxonomy_term_data) to alter the query. The new alias may actually make that alteration difficult or buggy.

Instead of using ttd as the alias in the db_select, I propose that we use the default one, taxonomy_term_data when adding the fields, condition and the orderBy

The same concern applies to using n as alias for the node table, instead of using the default.

I'll make these suggestions directly in the PR code.

(* When I say default I mean the one created by db_select when no alias is specified. for example db_select('node'); will use node as the default alias for the node table.)

[klonos]

Good catch @argiepiano 👍🏼 ...I wasn't aware that db queries could be altered via hooks. The requested changes do make sense to me. @indigoxela unless you have objections, can you please review and accept the changes?

[prevoj]

I would definitely be willing to do so!

[indigoxela]

@argiepiano many thanks for catching that possible BC problem.

Although the GitHub UI can be handy, things get convoluted quickly when more than one or two lines change, so I decided to not simply merge your suggestions, but adapted my local branch as suggested. That way I was also able to test things properly before pushing.

[argiepiano]

Thanks @indigoxela. LGTM 👍🏽

[quicksketch]

I merged https://github.com/backdrop/backdrop/pull/4137 into 1.x and 1.22.x.

https://github.com/backdrop/backdrop/commit/e20dd2ce97094296166914c9d98623a35f779e1d by @indigoxela, @argiepiano, @prevoj, and @klonos.

Great work here team! WELCOME to @prevoj! Thank you for both filing the issue and follow-up on it!