taxonomy_autocomplete produces SQL error for nonexistent field

I don't like this... I think it obfuscates the problem. At least now the user has a chance of debugging what is wrong. I know core doesn't really have an AJAX error handling framework, and can't handle an error gracefully in the autocomplete code. But for now, at least logging something would be good if we can't throw a big ol' alert.

-J

I don't understand the "random URL" argument... Why would a bot visit a URL which wasn't linked to anywhere? And if it did, why would we care if it got an ugly error? I must be missing the point :)

Logging to watchdog is not a bad step, although how hard would it be to return a more digestible error which showed up in an alert() like "The field $field does not exist, please check the configuration of this free tagging box" or something like that...

Generally I don't agree it is a good idea to run SQL queries we know will break and throw errors. If we know an SQL query will break, why attempt to execute it?

I totally agree, that's why I'm very pro-exception handling. Even if they do break, we shouldn't be passing that info directly back to users or developers really.

However, if we commit this patch, what is the difference between "didn't find any matching tags" and "your form field is totally broken and referencing a non-existant column in the database". It becomes almost impossible to debug.

I still don't understand how a bot would be trying to autocomplete a bad autocomplete field repetitively... OR are you saying that the watchdog will log when the field just shows? If that's the case, I still think some kind of logging / error handling is required. If a bot slams it, great, the developer will find the bug sooner.

In terms of re-architecting autocomplete.js, etc I agree and mentioned that we can't fix that right now in my first reply. What happens if you return a string instead of JSON though? It sounds to me like it is "handling" the SQL error by just dumping in an alert. What if we returned our digestible error in a string, would it end up in an alert as well?

I think so. Does it work for you still?

While you're there, you could check that the field is indeed a taxonomy field
and also that field_access('view') is true - since you're about to disclose information about the values in this field.

Like this?

re @ksenzee #9 : yup. As a nitpick, I'm not sure the various 'not OK' cases are worth generating specific error messages - the 403 for the specific case of !field_access('view', $field) is needed, other cases could just result in "Taxonomy field @field_name not found.", this is just safeguard for a case that's not supposed to happen anyway.

I'm not able to apply or test patches right now, so I'll leave others RTBC.

True. I was thinking I should check field access before checking field type, but it doesn't really matter, and it's less work for translators this way. New patch attached.

The latest patch here adds a translatable string, so I'm giving this issue the "string freeze" tag. We could probably do it without adding a new string if we absolutely had to though.

Tests, please.

The field_access() call in this patch also has the wrong syntax (it is missing the required third parameter).

Perhaps also it should get a code comment or something. It is not easy to understand why we want to check field access here, and it misled me for a bit (I thought maybe it was a security issue that we don't have this, but it turns out it isn't, since taxonomy terms in general are already possible for anyone with 'access content' permission to find out).

On further thought, I'm not sure the field_access() call belongs here at all. It seems like all it does is make the taxonomy autocomplete callback less generically useful. (I am writing some code for Views that uses this autocomplete callback, and adding the field access check as done in this patch would cause a bit of pain for that use case.)

The function takes a field as input, but only returns a list of terms as output; it does not return any content from the field itself. So I think the only access check that matters here is term_access (which the function already does).

I'd tend to agree. We're not displaying actual values of the field (for which you'd need 'view field' access), but a list of terms in a vocab. This has nothing to do with field_access().

#11: 814804-11-taxonomy-autocomplete-error.patch queued for re-testing.

Here is one w/oo the field_access

< function taxonomy_autocomplete($field_name, $tags_typed = '') {
---
> function taxonomy_autocomplete($field_name, $entity_type, $tags_typed = '') {

Why is there a new parameter? That looks like a mistake.

oops, absolutely right. Let me fix that.

shouldn't make patches before coffee. How's this one?

okay, for reals this time.

That looks good to me. Still needs a test per #14.

Added test to #27 per #14

Trying again.

Subscribe.

Test looks good. Made some spelling and other minor fixes. Also moving to 8.x - it's a backport candidate though.

Someone else gets this error when trying to patch? :

patch -p1 < taxonomy-error.patch
patching file modules/taxonomy/taxonomy.pages.inc
Hunk #1 succeeded at 74 (offset -2 lines).
patching file modules/taxonomy/taxonomy.test
Hunk #1 FAILED at 597.
1 out of 1 hunk FAILED -- saving rejects to file modules/taxonomy/taxonomy.test.rej

Thank you

Rerolled the patch.

Odd, the patch applied for me. I have rolled it again, testing on both D8 and D7.

Rerolled for core/.

Requires a reroll after #93854: Allow autocompletion requests to include slashes has been fixed.

It looks like #93854: Allow autocompletion requests to include slashes was rolled back, but the patch here needs a reroll for other reasons anyway.

Here are up-to-date versions for both D7 and D8.

Sorry for the issue queue noise, but for Drush Make purposes I need a patch against Drupal 7.10, and the above one doesn't apply there.... So in case anyone else needs to run this patch against Drupal 7.10, here it is :)

This is ready to go once again. :)

#40: autocomplete_query_error-814804-40.patch queued for re-testing.

Just a reroll to accomodate another addition to the autocomplete tests.

Reroll of David's patch in #41 for Drupal 7.12.

Thanks for the re-roll. Committed/pushed to 8.x, moving back to 7.x.

Renamed for the bot.

This is back to RTBC.

Committed and pushed to 7.x. Thanks!

I received this error on my 7.x website. The 7.x patch doesn't apply to the latest core. Any suggestions?

Different cause, same error message.