Its impossible to re-use the Taxonomy term autocomplete without creating fields. I'm opening this thread to push to rewrite the taxonomy_autocomplete() function such that it will accept multiple vocabularies, rather than accept a single field to function from.
The current design is too closely tied with other core functionality and doesn't appear to sport Drupal's modular fashion.

For the sake of reference

line 79 and below of modules/taxonomy/taxonomy.pages.inc:

/**
 * Helper function for autocompletion
 */
function taxonomy_autocomplete($field_name, $tags_typed = '') {
  $field = field_info_field($field_name);

  // The user enters a comma-separated list of tags. We only autocomplete the last tag.
  $tags_typed = drupal_explode_tags($tags_typed);
  $tag_last = drupal_strtolower(array_pop($tags_typed));

  $matches = array();
  if ($tag_last != '') {

    // Part of the criteria for the query come from the field's own settings.
    $vids = array();
    $vocabularies = taxonomy_vocabulary_get_names();
    foreach ($field['settings']['allowed_values'] as $tree) {
      $vids[] = $vocabularies[$tree['vocabulary']]->vid;
    }

    $query = db_select('taxonomy_term_data', 't');
    $query->addTag('translatable');
    $query->addTag('term_access');

    // Do not select already entered terms.
    if (!empty($tags_typed)) {
      $query->condition('t.name', $tags_typed, 'NOT IN');
    }
    // Select rows that match by term name.
    $tags_return = $query
      ->fields('t', array('tid', 'name'))
      ->condition('t.vid', $vids)
      ->condition('t.name', '%' . db_like($tag_last) . '%', 'LIKE')
      ->range(0, 10)
      ->execute()
      ->fetchAllKeyed();

    $prefix = count($tags_typed) ? implode(', ', $tags_typed) . ', ' : '';

    $term_matches = array();
    foreach ($tags_return as $tid => $name) {
      $n = $name;
      // Term names containing commas or quotes must be wrapped in quotes.
      if (strpos($name, ',') !== FALSE || strpos($name, '"') !== FALSE) {
        $n = '"' . str_replace('"', '""', $name) . '"';
      }
      else {
        $term_matches[$prefix . $n] = check_plain($name);
      }
    }
  }

  drupal_json_output($term_matches);
}

How to improve it

Rather than try to capture which field we want, why not make the autocomplete callback accept vocabulary id's from the field generating the autocomplete URL? Then, just train the function to split up the vocabulary ID's by some common character.

Why on earth am I whining about this?

Well, discussing how to overcome the use of a Taxonomy term autocomplete started around here: http://drupal.org/node/1101432#comment-4436730 , http://drupal.org/node/1101432#comment-4520422

And after reading up on the code, I strongly urge it to be de-coupled from the field implementation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wjaspers’s picture

Category: bug » feature
RobLoach’s picture

Version: 7.x-dev » 8.x-dev
joachim’s picture

+1

This sounds like a more sensible way to do it, and more reusable as the OP says. taxonomy_autocomplete() should work on a vocabulary/ies, rather than a field.

xjm’s picture

Title: Autocomplete design needs rewrite » Taxonomy autocomplete:factor out helpers so other modules can use them as API
Assigned: Unassigned » xjm
Priority: Major » Normal

Was just about to create this exact issue.

joachim’s picture

Status: Active » Needs review
FileSize
2.75 KB

Here's a patch.

Some things to note:

> // Part of the criteria for the query come from the field's own settings.

This is true, but only to the extent that you select a vocab. The $field['settings'] array looks like this:

    allowed_values (Array, 1 element)
        0 (Array, 2 elements)
            vocabulary (String, 4 characters ) tags
            parent (Integer) 0

... but the 'parent' key does nothing at all.

-    foreach ($field['settings']['allowed_values'] as $tree) {
-      $vids[] = $vocabularies[$tree['vocabulary']]->vid;
-    }
+    $vid = $vocabularies[$vocabulary_name]->vid;

At first glance I've removed something here -- but I haven't, as term reference fields only support a single vocabulary -- the $vids array was completely pointless.

It *might* be nice to allow this autocomplete to operate on more than one vocab, but that's out of the scope of this patch I think. It's kind of beyond how taxonomy currently works, too, and it would also require a rethink of the path callback arguments.

xjm’s picture

I think taxonomy_autocomplete_validate() should probably be refactored in the same way.

Edit: Just realized this patch is not actually what I thought. My plan was to start by taking the guts out of taxonomy_autocomplete() and taxonomy_autocomplete_validate() (which call drupal_json_output() and form_set_value() after logic) so that the logic could be reused by other modules wishing to alter the autocomplete behavior. That would address the problem in a different (and backportable) way.

joachim’s picture

Issue tags: -Needs backport to D7

I don't see how it can be, without providing a dedicated term autocomplete form element:

    '#element_validate' => array('taxonomy_autocomplete_validate'),

You can't pass parameters to the validator function. So it's dependent entirely on the form $element and the user values. So it has to do this to get valid vocabs:

    // Collect candidate vocabularies.
    $field = field_widget_field($element, $form_state);
    $vocabularies = array();
    foreach ($field['settings']['allowed_values'] as $tree) {
      if ($vocabulary = taxonomy_vocabulary_machine_name_load($tree['vocabulary'])) {
        $vocabularies[$vocabulary->vid] = $vocabulary;
      }
    }

If we take FieldAPI out of this, how does it know which vocabs are allowed? How would a custom module using the taxonomy autocomplete tell taxonomy_autocomplete_validate() which vocabs are allowed? This takes us to the point of needing a form element that has '#allowed_vocabs' = array($machine_name, ...) and that's a custom form element.

Definitely out of the scope of this particular issue I think.

joachim’s picture

Issue tags: +Needs backport to D7

Given this is a purely internal change, can we backport this to D7 to help contrib modules?

xjm’s picture

Issue tags: +Needs backport to D7

I am confused; I didn't say anything about taking Field API out of anything? It could be as simple as:

function taxonomy_autocomplete_validate($element, &$form_state) {
  $value = _taxonomy_autocomplete_validate($element, &$form_state);  // Probably needs a better name instead of _
  form_set_value($element, $value, $form_state);
}

And:


function taxonomy_autocomplete($field_name, $tags_typed = '') {
  $term_matches = _taxonomy_autocomplete($field_name, $tags_typed); // Ditto
  drupal_json_output($term_matches);
}

That way other modules can override form validation by using taxonomy.module's logic and just tweaking the results, rather than copying the whole thing. The problem is that these functions both contain logic and then do something at the end instead of returning something.

xjm’s picture

Category: feature » task

More of a task than a feature really, in both #6 and #10. No new functionality is being added, just reusability.

Status: Needs review » Needs work

The last submitted patch, 1169964.drupal.taxonomy-autocomplete-refactor.patch, failed testing.

joachim’s picture

  $term_matches = _taxonomy_autocomplete($field_name, $tags_typed); // Ditto
  drupal_json_output($term_matches);

Doing that wouldn't remove the tight coupling with FieldAPI which is what this issue is primarily about.

I can see what you're saying though -- split these functions so there is a pure API function that returns raw data, and then a wrapper function that handles the output.

Is that needed though? When would you want a list of term matches to a partial string in an array, rather than ready for an autocompleting form element? And as for taxonomy_autocomplete_validate, that's an #element_validate callback by definition -- it's meant to do form_set_error().

xjm’s picture

There is a good use case. TAC for example (the reason I am following this) needs to modify the autocomplete field in the following ways:

  1. Remove certain entries from default list
  2. Define its own autocomplete path
  3. Prevent a certain query alter from taking place at the autocomplete path
  4. Prevent certain entries from being returned by autocomplete
  5. Allow certain non-listed, non-autocompleting values to merge back in at validation without the user seeing them.

I am currently calling these functions directly within TAC's callbacks, but that adds the need for a lot of monkey business. If I could instead act on the results before doing form_set_value and drupal_json_output() in my own callbacks, it would save a ton of overhead and ugly workarounds.

I think I thought the original issue included this as part of the fix. :) But the two fit together.

wjaspers’s picture

@joachim, in response to #8 you'll know which Vocabs are allowed by passing the Vocab ID to the URL which is firing the autocomplete activity.

For instance,
/taxonomy/autocomplete/1,5,23

Would request terms from only vocabs 1, 5, and 23.

The Field making the autocomplete request would assemble the URI properly based in its settings at that time. The reasoning behind taknig the FieldAPI out of the autcomplete is that fields, and ONLY fields can use it now--which limits the ability of modules to use it.

joachim’s picture

> 4. Prevent certain entries from being returned by autocomplete

I don't think that letting core find some terms and then removing the ones the user has no access to is the right way to go -- surely you'd then need your API _taxonomy_autocomplete() function to return tids rather than term names.

I think you're doing something sufficiently different from core that you warrant your own autocomplete callback :)

joachim’s picture

@wjaspers: you're saying pick the vocab machine name out of the $element's #autocomplete_path? Doable... bit ugly but definitely doable.

joachim’s picture

At any rate, here's new version of my patch but where the tests should pass :)

(See notes about changes in this patch in #6.)

xjm’s picture

Status: Needs work » Needs review

I don't think that letting core find some terms and then removing the ones the user has no access to is the right way to go -- surely you'd then need your API _taxonomy_autocomplete() function to return tids rather than term names.

I think you're doing something sufficiently different from core that you warrant your own autocomplete callback :)

I have my own autocomplete callback that calls taxonomy.module's. Writing my own from scratch would be duplicating it exactly, line for line, and then adding one additional function at the end. The module's specific internals are beside the point, though. The problem is that the logic of "what stuff" is not decoupled from "do stuff," and that is, inherently, not good. Simply adding wrappers to separate the "do stuff" part out makes the logic reusable.

wjaspers’s picture

RT #17, I would just use the Vocab ID to keep things short, but the machine name should be ok.

RT #18
$tag_last isn't running through check_plain (unless drupal_strtolower is doing it for you).
The original implementation allowed the field to request terms from multiple vocabs.
People might get frustrated that it only works with one vocab.

joachim’s picture

> The original implementation allowed the field to request terms from multiple vocabs.

No, it didn't -- see #6 above.

wjaspers’s picture

Whoops. my bad.

joachim’s picture

> $tag_last isn't running through check_plain (unless drupal_strtolower is doing it for you).

I'm not changing anything to do with $tag_last, so if it should be, it's another issue :)

xjm’s picture

Note that #15 actually is a feature request. However, #10 coupled with the working version of joachim's patch (coupling the decouplings!) would I believe provide a back-portable workaround for contrib modules to make multi-vocabulary autocomplete fields. Contrib would override the autocomplete path (as I have done), and then in its own callback, call the helper function I suggest for each vocabulary, merge the results, and then send it to drupal_json_output($term_matches). It wouldn't be optimized, but the number of vocabularies would presumably small, so it would provide the functionality when someone needed it in custom code.

joachim’s picture

> Note that #15 actually is a feature request.

Yup. And I'm not sure on the kitten status of this.

On the one hand, adding it to the current issue kills kittens. On the other hand, it could be argued that the existing code in the function, because is uses a $vids array, is *designed* for multiple vocabs even though there aren't ever any. So... at a push it could fly.

xjm’s picture

Title: Taxonomy autocomplete:factor out helpers so other modules can use them as API » Taxonomy autocomplete: factor out helpers so other modules can use them as API
FileSize
4.26 KB

Attached patch incorporates both the refactor of taxonomy_autocomplete() and the decoupling from FAPI to provide fully reusable autocomplete functionality. (As joachim has pointed out, my earlier question about the validator refactor is still related to fields and therefore a separate issue. Similarly, the multiple vocabularies question is a separate issue.).

xjm’s picture

Title: Taxonomy autocomplete: factor out helpers so other modules can use them as API » Taxonomy autocomplete: decouple from Field API and factor out a helper so that code is reusable

Sorry for noise; setting a more accurate title.

joachim’s picture

Status: Needs review » Needs work
+++ b/modules/taxonomy/taxonomy.pages.inc
@@ -74,10 +74,34 @@ function taxonomy_term_feed($term) {
+function taxonomy_autocomplete_term_matches($vocabulary_name, $tags_typed = '') {

If you're really uncoupling, 'typed' doesn't really come into it... and the function name shouldn't mention autocomplete. It's just taxonomy_get_matching_terms(), maybe?

Powered by Dreditor.

xjm’s picture

If you're really uncoupling, 'typed' doesn't really come into it... and the function name shouldn't mention autocomplete. It's just taxonomy_get_matching_terms(), maybe?

Hm, I am not sure I agree. taxonomy_get_matching_terms() is pretty vague, and the matching behavior is still designed for autocomplete. For example, the data format is autocomplete-specific--the keys include the full field content with previous tags, and the names of the matched terms are the values. So, it is still for taxonomy autocompletion; it is just available to be altered.

joachim’s picture

> the keys include the full field content with previous tags, and the names of the matched terms are the values

Ah good point.

xjm’s picture

Maybe taxonomy_autocomplete_get_matches() is a better name? I think the rest (about the data format) is clear in the doxygen comment in #26.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.26 KB

Attached is identical to #26 except with the better function name taxonomy_autocomplete_get_matches().

andypost’s picture

Status: Needs review » Needs work
+++ b/modules/taxonomy/taxonomy.testundefined
@@ -629,7 +629,7 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
-    $this->drupalGet('taxonomy/autocomplete/taxonomy_' . $this->vocabulary->machine_name . '/' . $input);
+    $this->drupalGet('taxonomy/autocomplete/' . $this->vocabulary->machine_name . '/' . $input);

It looks like API change, maybe better to fix with parameter 'taxonomy_' . $this->vocabulary->machine_name

auto complete just needs to remove a prefix

-12 days to next Drupal core point release.

dawehner’s picture

Issue tags: +VDC

This is sort of helpful to replace the custom hack in views.

andypost’s picture

Patch needs re-roll, having standard auto-complete in core for terms could be very helpful for contrib
Suppose no reason to backport this to D7 because it probably breaks contrib

xjm’s picture

@andypost, the idea was to provide a helper, and not change the existing API, which would be backportable.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.55 KB

Rerolled against HEAD, hopefully not that many failed tests.

Wim Leers’s picture

Patch bingo. Patch looks fine, I just have nitpicks and comments + naming suggestions.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ *  The machine name of the vocabulary to autocomplete on.

Delete "to autocomplete on"?

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ *   Returns the json response object containing the autocomplete results.

s/json/JSON/

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ * Helper function to generate taxonomy autocomplete suggestions.

Generates taxonomy autocomplete suggestions.

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ *   The machine name of the vocabulary to autocomplete on.

Delete "to autocomplete on"?

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ *   A comma-separated list of tags. We only autocomplete the last tag.

"We only …" -> "Only the last tag is autocompleted."

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+ *   An associative array of possible matches, keyed by the corresponding

s/possible matches/suggestions/

+++ b/core/modules/taxonomy/taxonomy.pages.incundefined
@@ -97,37 +97,43 @@ function taxonomy_term_feed(Term $term) {
+function taxonomy_autocomplete_get_matches($vocabulary_id, $tags_typed) {

s/matches/suggestions/

?

dawehner’s picture

FileSize
6.59 KB

Oh nice!

I'm not 100% sure about the suggestions changes, but yeah here is a patch.

dawehner’s picture

FileSize
2.49 KB

Forgot the interdiff

Wim Leers’s picture

"Matches" is fine too, as long as it's consistent everywhere. And if you use "matches", don't say "possible matches", but "matches".

Status: Needs review » Needs work

The last submitted patch, drupal-1169964-39.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
8.66 KB

I'm not absolute sure about the last change, but I don't think it is really useful to print out some debug messages once someone calls it with a wrong vocabulary-id?

xjm’s picture

Assigned: xjm » Unassigned
dawehner’s picture

So everyone who subscribed to that issue: How do we want to solve it, once we have entity reference using that?

Maybe we should still define our own controller, but pass in some field for views?
We could also help with a additional wrapper for the entity reference one, so we can get the feature required in this issue.

dawehner’s picture

I guess this will not happen, so we could move it directly to d7?

jibran’s picture

Issue tags: -Needs backport to D7, -VDC

#43: drupal-1169964-43.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1169964-43.patch, failed testing.

tkoleary’s picture

Please follow progress on: #2346973: Improve usability, accessibility, and scalability of long select lists

Changing to Select2 may render this issue obsolete

amateescu’s picture

Status: Needs work » Closed (duplicate)

We now have a generic 'entity_autocomplete' form api element: https://www.drupal.org/node/2418529